Re: [Nano-devel] [PATCH V6] Implement bookmarks

[Marco Diego Aurélio Mesquita]

On Wed, Dec 05, 2018 at 08:26:12PM +0100, Benno Schulenberg wrote:
> 
> Op 04-12-18 om 16:55 schreef Marco Diego Aurélio Mesquita:
> > Differences from V5:
> >  - It is now possible to move a bookmark on the
> >    current line.
> >  - Next and previous no longer ignores bookmarks
> >    on the current line.
> 
> Why are you making things more complicated?  Any complication you
> add is a no-no and reduces your chances of getting the patches
> accepted to zero.  Try to make things simpler, as lean and simple
> as you can make it.
> 

Because of the previous review. Don't worry, V7 fixes it.

> You've already found a way to reduce the overhead, otherwise I
> would have proposed to drop storing the x position in the line.
> Geany's bookmarks just remember a line; when you jump to such
> a bookmark, the cursor jumps to the start of the line.  Does
> nano need more precision than that?
> 

Fixed in V7.

> >  - Relevant parts are excluded from NANO_TINY.
> 
> Not yet all.  See below.
> 

Fixed in V7.

> > Subject: [PATCH 1/2] new feature: ability to toogle and jump to bookmarks
> 
> Please fix the spello: toggle.  Also elsewhere.
> 

Fixed in V7.

> > For now, bookmarks are not visible in any way.
> 
> Do you have any plans to make them visible?  In the space between line
> number and text maybe?
> 

Yes I have plans for this. I tried not highlighting the number of
a bookmarked line and liked it. What do you think about it?

> > +   size_t bookmark_x;
> > +           /* Bookmark position for this line. */
> 
> This should be ifdeffed.
> 
> > +/* Toggles bookmark. */
> 
> The function comments in nano use imperative style.
> 

Fixed in V7.

> > +void bookmark(void)
> > +{
> > +   if (is_bookmarked(openfile->current) &&
> > +           openfile->current->bookmark_x != openfile->current_x){
> > +           /* Handle the case of moving a bookmark on current line. */
> > +           openfile->current->bookmark_x = openfile->current_x;
> > +           statusbar(_("Bookmark moved."));
> 
> If hitting "Toggle bookmark" somewhere else in an already bookmarked
> line *moves* the bookmark, you cannot call it toggling any more.
> 
> The moving the bookmark is an unneeded complication.  Get rid of it.
> 

Fixed in V7.

> > +   statusbar(is_bookmarked(openfile->current) ?
> 
> Now you've called is_bookmarked() three times.  Do it just once, and
> thus clearly separate the different situations.
> 

Gone in V7.

> > +   refresh_needed = TRUE;
> 
> A bookmark is not visible, so setting or removing has no visual effect,
> so there is no need to refresh the edit window.
> 

It is needed for the case when the screen must be scrolled.

> > +           /* Special case when current line is bookmarked. */
> 
> No.  When people have such long lines that they need a bookmark in order
> to navigate inside it, they... deserve their misery.  They will have to
> type <Alt+PageUp> and then <Alt+PageDown> to jump to that mark.  Most
> people, when they type <Alt+PageUp>, want to jump to an earlier bookmark,
> so that is what the function should do.
> 

Fixed in V7.

> > +           if (current == NULL) {
> > +                   statusbar(next ?
> > +                                           _("No bookmark after this 
> > point.") :
> > +                                           _("No bookmark before this 
> > point."));
> 
> Searching for a bookmark should not stop at top or bottom of file,
> it should wrap around, like ^W does.  Most likely I will ever use
> only one bookmark, and when I move around in the file, I do not
> want to need to realize where I moved in the file, I just want to
> type <Alt+PageUp> and be moved to that single bookmark, wherever
> it is.
> 

Fixed in V7.

> > +   go_to_book_mark(TRUE);
> > +   go_to_book_mark(FALSE);
> 
> Use FORWARD and BACKWARD instead; they are defined in nano.h.
> 

Fixed in V7.

> >  #define ALT_DOWN 0x424
> > +#define ALT_INSERT 0x42C
> >  #define ALT_DELETE 0x42D
> > +#define ALT_PAGEUP 0x427
> > +#define ALT_PAGEDOWN 0x428
> 
> The order should be sensible: ascending codes.  So move the
> last two lines to before ALT_INSERT.
> 

Fixed in V7.

> >  extern int altleft, altright;
> >  extern int altup, altdown;
> > -extern int altdelete;
> > +extern int altinsert, altdelete, altpageup, altpagedown;
> 
> These should be grouped in pairs.
> 

Fixed in V7.

Thanks for the review! I think V7 is now commit-ready.