[PATCH] Edit Bookmarks name

Yosef Hamza jo.adam.93 at gmail.com
Tue Apr 1 12:44:39 PDT 2014


Switching the pixmap to flag because custom name bookmarks are still indeed
bookmarks just with different name that user have chosen for what suit
him/her.

here's a test logbook I have edited that shows you can use to check
saving/loading. and about effecting the code, while testing it I found that
it works just as bookmarks, YET there's some issues with the bookmarks
itself -like with editing then saving all the bookmarks will be gone-.

Planned to work on it after being done with this one.

PS' I replied with that patch and that test after like 2 hours of receiving
those issues, but I guess there was persona/ML conflict.



On Tue, Apr 1, 2014 at 9:26 PM, Dirk Hohndel <dirk at hohndel.org> wrote:

>
> I'll respond to the last feedback that I had given you - but this is to
> the patch you sent 15 minutes ago:
>
> On Tue, 2014-03-25 at 09:49 -0700, Dirk Hohndel wrote:
> > On Tue, 2014-03-25 at 17:55 +0200, Yosef Hamza wrote:
> >
> > big parts of your patch have "4 space" indentation.
>
> Still true
>
> > can you explain in the commit message why you switched the pixmap to be
> > a flag?
>
> Still missing
>
> > 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?
>
> fixed
>
> > the comment "so we know what to remove" makes no sense in your case (it
> > made sense for the code which you copied).
>
> fixed
>
> > 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
>
> I don't see any indication that you have done that. Should be in the
> commit message
>
> > And of course you can ONLY do the name change if the event is of type
> > SAMPLE_EVENT_BOOKMARK
>
> Fixing the third comment above fixed this as well
>
> Please address the remaining issues.
>
> /D
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20140401/cf0a0d33/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test.ssrf
Type: application/octet-stream
Size: 828 bytes
Desc: not available
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20140401/cf0a0d33/attachment.obj>


More information about the subsurface mailing list