[PATCH] Prevent gaschange tank icons from using garbage coords.

K. "pestophagous" Heller pestophagous at gmail.com
Thu Dec 3 10:58:34 PST 2015


On Wed, Dec 2, 2015 at 9:41 PM, Dirk Hohndel <dirk at hohndel.org> wrote:
>
>> On Dec 2, 2015, at 9:20 PM, K. pestophagous Heller <pestophagous at gmail.com> wrote:
>>
>> The DiveEventItem(s) need not store a pointer to memory
>
> "need not" is odd. "should not" maybe?

haha. oddness seems to be a side-effect as i try to internalize the
discipline of using imperative tense in commit messages.  i never
developed any habits about verb tense in commit messages, and now that
i am hyper-aware of this concept, it's making me write real weird.
getting afraid to use verbs in general. help!

>> Fixes #968
>>
>> Signed-off-by: K. Heller <pestophagous at gmail.com>
>> ---
>>

>> When 'internalEvent' is a pointer to freed memory,
>> internalEvent->time.seconds can have all kinds of crazy values, which
>> get used in DiveEventItem::recalculatePos to place the tank at weird x
>> coordinates.
>
> Some of this explanation could even migrate up to the commit message.
> It's always nice to have more context there.

good idea.

>> DiveEventItem::recalculatePos is a slot.  As many have said, one faces
>> an uphill battle (a losing battle) to try to reason cleanly about
>> ordering and dependency graphs and such between all the various
>> signals and slots.
>
> That's the beauty and the curse of it. It makes things presumably
> easier when writing code - but then it comes back to bite you.

i am open to the beauty of it.  it offers the promise of some great
potential decoupling of components, but it is hindered (for now)
because many subsurface widgets that subclass Qt classes don't act
"object oriented" in terms of encapsulating (hiding) and protecting
their own state.

>> +     // logic for copying ev partially copied from copy_events.
>> +     // maybe dive.c should have a 'copy_event' used here and by copy_events?
>
> Yes, I'd like that better
>
>> +     int size = sizeof(*ev) + strlen(ev->name) + 1;
>> +     internalEvent = (struct event*) malloc(size);
>> +     memcpy(internalEvent, ev, size);
>> +     internalEvent->next = NULL;
>
> that way this code would be in dive.c and we'd just call that helper.

glad you agree. copy_event will be created.

>
>>       setupPixmap();
>>       setupToolTipString();
>>       recalculatePos(true);
>> diff --git a/profile-widget/diveeventitem.h b/profile-widget/diveeventitem.h
>> index f358fee..9d6ad5d 100644
>> --- a/profile-widget/diveeventitem.h
>> +++ b/profile-widget/diveeventitem.h
>> @@ -11,6 +11,7 @@ class DiveEventItem : public DivePixmapItem {
>>       Q_OBJECT
>> public:
>>       DiveEventItem(QObject *parent = 0);
>> +     virtual ~DiveEventItem();
>>       void setEvent(struct event *ev);
>>       struct event *getEvent();
>>       void eventVisibilityChanged(const QString &eventName, bool visible);
>
> The rest looks good to me.
>
> Would you re-edit this and re-submit?

will do tonight. my pleasure.

/K


More information about the subsurface mailing list