Discussion:
[Nano-devel] [PATCH V6] Implement bookmarks
Marco Diego Aurélio Mesquita
2018-12-04 15:55:57 UTC
Permalink
Hi Devs!

Attach patches implement bookmarks in nano.

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.
- Relevant parts are excluded from NANO_TINY.
- Removed do_ prefixes from function names.
- Added features are rebindable.


Please review it.
Benno Schulenberg
2018-12-05 19:26:12 UTC
Permalink
Post by Marco Diego Aurélio Mesquita
- 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?
Post by Marco Diego Aurélio Mesquita
- Relevant parts are excluded from NANO_TINY.
Not yet all. See below.
Post by Marco Diego Aurélio Mesquita
Subject: [PATCH 1/2] new feature: ability to toogle and jump to bookmarks
Please fix the spello: toggle. Also elsewhere.
Post by Marco Diego Aurélio Mesquita
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?
Post by Marco Diego Aurélio Mesquita
+ size_t bookmark_x;
+ /* Bookmark position for this line. */
This should be ifdeffed.
Post by Marco Diego Aurélio Mesquita
+/* Toggles bookmark. */
The function comments in nano use imperative style.
Post by Marco Diego Aurélio Mesquita
+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.
Post by Marco Diego Aurélio Mesquita
+ statusbar(is_bookmarked(openfile->current) ?
Now you've called is_bookmarked() three times. Do it just once, and
thus clearly separate the different situations.
Post by Marco Diego Aurélio Mesquita
+ 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.
Post by Marco Diego Aurélio Mesquita
+ /* 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.
Post by Marco Diego Aurélio Mesquita
+ if (current == NULL) {
+ statusbar(next ?
+ _("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.
Post by Marco Diego Aurélio Mesquita
+ go_to_book_mark(TRUE);
+ go_to_book_mark(FALSE);
Use FORWARD and BACKWARD instead; they are defined in nano.h.
Post by Marco Diego Aurélio Mesquita
#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.
Post by Marco Diego Aurélio Mesquita
extern int altleft, altright;
extern int altup, altdown;
-extern int altdelete;
+extern int altinsert, altdelete, altpageup, altpagedown;
These should be grouped in pairs.

Benno
Marco Diego Aurélio Mesquita
2018-12-06 01:37:05 UTC
Permalink
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
- 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.
Post by Benno Schulenberg
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.
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
- Relevant parts are excluded from NANO_TINY.
Not yet all. See below.
Fixed in V7.
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
Subject: [PATCH 1/2] new feature: ability to toogle and jump to bookmarks
Please fix the spello: toggle. Also elsewhere.
Fixed in V7.
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
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?
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
+ size_t bookmark_x;
+ /* Bookmark position for this line. */
This should be ifdeffed.
Post by Marco Diego Aurélio Mesquita
+/* Toggles bookmark. */
The function comments in nano use imperative style.
Fixed in V7.
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
+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.
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
+ 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.
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
+ 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.
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
+ /* 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.
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
+ if (current == NULL) {
+ statusbar(next ?
+ _("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.
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
+ go_to_book_mark(TRUE);
+ go_to_book_mark(FALSE);
Use FORWARD and BACKWARD instead; they are defined in nano.h.
Fixed in V7.
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
#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.
Post by Benno Schulenberg
Post by Marco Diego Aurélio Mesquita
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.
Brand Huntsman
2018-12-06 02:44:33 UTC
Permalink
On Wed, 5 Dec 2018 23:37:05 -0200
Post by Marco Diego Aurélio Mesquita
Post by Benno Schulenberg
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?
Would be better if you colored a '>', or other character, with function color and put it in the blank column. Unless it will only appear when line numbers are enabled, which isn't useful for users who don't use line numbers.
Post by Marco Diego Aurélio Mesquita
I think V7 is now commit-ready.
You didn't rename go_to_book_mark() as David suggested.

The cursor jumping is back. Go to end of line above bookmarked line, jump to bookmark, then use up arrow, it jumps to the end of line. It should reset cursor to start of line.
Benno Schulenberg
2018-12-06 19:12:56 UTC
Permalink
Post by Marco Diego Aurélio Mesquita
Post by Benno Schulenberg
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?
When I use line numbers, they are colored. When the number for a
bookmarked is uncolored... don't know if that is nice. But it's
probably better than putting a star or some other character in the
single column between line number and text.
Post by Marco Diego Aurélio Mesquita
Post by Benno Schulenberg
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.
But toggling a bookmark does not move the cursor at all, so why
would the screen scroll?
Post by Marco Diego Aurélio Mesquita
Thanks for the review! I think V7 is now commit-ready.
Commit-ready... Oh, how eager you are. :)

Have a look at https://savannah.gnu.org/bugs/?55054. It has had
fifteen revisions already and still I'm not ready to commit it.
In other words: patience.

Benno

Brand Huntsman
2018-12-05 22:15:30 UTC
Permalink
On Tue, 4 Dec 2018 13:55:57 -0200
Post by Marco Diego Aurélio Mesquita
- It is now possible to move a bookmark on the
current line.
- Next and previous no longer ignores bookmarks
on the current line.
- Added features are rebindable.
I haven't tested the patch yet but changing bookmark_x to a bool and only jumping to start of line, as Benno suggested, would solve the need to not ignore current line or move a bookmark. Less useful for softwrapped paragraphs but it eliminates all confusion.

go_to_book_mark() and is_bookmarked() should be static, but changing bookmark_x to a bool would eliminate the need to have is_bookmarked().

You need doc/nano.texi and doc/nanorc.5 documentation for the bindable function names in first patch. And the second patch needs to mention the hard binds in those two files as well. Find "zap" for an example.

A user-defined bind wouldn't show up in help without the second patch. So the add_to_funcs and gists should be in the first patch.
David Ramsey
2018-12-06 00:04:27 UTC
Permalink
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?
<snip>
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.
...changing bookmark_x to a bool and only jumping to start of line, as
Benno suggested, would solve the need to not ignore current line or
move a bookmark. Less useful for softwrapped paragraphs but it
eliminates all confusion.
Thirded on both counts. Jumping to the start of the line is precise
enough for me, and if a single softwrapped line is long enough to need
multiple bookmarks on it, that's what search is for.

(Alternatively, you could just justify overly long lines with --fill=72
to break them up into pieces, edit the pieces using bookmarking, and
then justify the pieces again with --fill=100000 to turn them back into
overly long lines. Assuming that the lines are no longer than 100000
columns, of course.)

Having multiple bookmarks is fine and, in many cases, necessary; having
multiple bookmarks *per line* and having to move them around is overly
complex.

----------
Do you have any plans to make them visible?
I wonder about this too; seeing where bookmarks are would be handy.

<snip>
Searching for a bookmark should not stop at top or bottom of file,
it should wrap around, like ^W does.
Seconded; that would make it consistent.

----------

I've noticed one oddity in the first patch: relevant functions are named
bookmark(), bookmark_next(), bookmark_previous(), etc., but there's one
that breaks the pattern: go_to_book_mark(). Shouldn't it be named
go_to_bookmark() instead, so that searching for "bookmark" doesn't miss
it?
Brand Huntsman
2018-12-06 00:25:31 UTC
Permalink
On Wed, 5 Dec 2018 18:04:27 -0600
Post by David Ramsey
Post by Benno Schulenberg
Do you have any plans to make them visible?
I wonder about this too; seeing where bookmarks are would be handy.
The blank column after line numbers is a good place but you wouldn't be able to see bookmarks if line numbers aren't enabled. So the presence of any bookmarks should add that blank column if not already there, and removing all bookmarks should hide the blank column if line numbers are disabled. If not feasible, it doesn't make sense to only display them if line numbers are enabled.
Loading...