[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Nano-devel] [PATCH V6] Implement bookmarks
From: |
Benno Schulenberg |
Subject: |
Re: [Nano-devel] [PATCH V6] Implement bookmarks |
Date: |
Wed, 5 Dec 2018 20:26:12 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 |
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