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

[Benno Schulenberg]

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.

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?

>  - Relevant parts are excluded from NANO_TINY.

Not yet all.  See below.

> Subject: [PATCH 1/2] new feature: ability to toogle and jump to bookmarks

Please fix the spello: toggle.  Also elsewhere.

> 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?

> +     size_t bookmark_x;
> +             /* Bookmark position for this line. */

This should be ifdeffed.

> +/* Toggles bookmark. */

The function comments in nano use imperative style.

> +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.

> +     statusbar(is_bookmarked(openfile->current) ?

Now you've called is_bookmarked() three times.  Do it just once, and
thus clearly separate the different situations.

> +     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.

> +             /* 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.

> +             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.

> +     go_to_book_mark(TRUE);
> +     go_to_book_mark(FALSE);

Use FORWARD and BACKWARD instead; they are defined in nano.h.

>  #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.

>  extern int altleft, altright;
>  extern int altup, altdown;
> -extern int altdelete;
> +extern int altinsert, altdelete, altpageup, altpagedown;

These should be grouped in pairs.

Benno

signature.asc
Description: OpenPGP digital signature