On Wed, Dec 2, 2015 at 9:41 PM, Dirk Hohndel <[email protected]> wrote: > >> On Dec 2, 2015, at 9:20 PM, K. pestophagous Heller <[email protected]> >> 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 <[email protected]> >> --- >> >> 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 _______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
