[PATCH] Edit Bookmarks name

Dirk Hohndel dirk at hohndel.org
Tue Mar 25 09:49:50 PDT 2014


On Tue, 2014-03-25 at 17:55 +0200, Yosef Hamza wrote:

big parts of your patch have "4 space" indentation.

can you explain in the commit message why you switched the pixmap to be
a flag?

you always add the option to "Edit name" to the context menu if there is
any event - but this only makes sense for bookmark events, right?

the comment "so we know what to remove" makes no sense in your case (it
made sense for the code which you copied).

changing the event->name is an interesting approach. Subsurface has not
been super consistent about the way in which we handle events. The
libdivecomputer design specifies the event->type, yet the subsurface
code often uses the event name to track things which likely is a mistake
in what was implemented in the very early code and was never fixed.

So in a way I'm OK with overwriting the name, but there's a price to be
paid:
- you need to make sure that this doesn't break anything else in the
code
- you need to make sure that you can save and load such events and
things behave correctly
- you need to add a test dive that has such a modified event in it

And of course you can ONLY do the name change if the event is of type
SAMPLE_EVENT_BOOKMARK

Linus - on a more fundamental level... we should change the code that we
have that scans for things like "gaschange" and have it scan for
SAMPLE_EVENT_GASCHANGE and SAMPLE_EVENT_GASCHANGE2 instead. I'm sure
there are other places where we trigger on the name instead of the type
as I think we should...

/D

/D



More information about the subsurface mailing list