jumplist: browser-style (or 'tagstack') navigation #11530

butwerenotthereyet · 2019-12-07T20:57:55Z

Traditionally, when navigating to a specific location from the middle of the jumplist results in shifting the current location to the bottom of the list and adding the new location after it. This behavior is not desireable to all users--see, for example https://vi.stackexchange.com/questions/18344/how-to-change-jumplist-behavior.

Here, another jumplist behavior is introduced. When g:jumplistmode is set to "history", the jumplist behaves like history of a web browser. When navigating to a location from the middle of the jumplist

2 first
1 second
0 third <-- current location
1 fourth
2 fifth

to a location that is different from the location that is currently next in the jumplist

new != fourth

the locations after the current location in the jump list are discarded

2 first
1 second
0 third

The result is that when moving forward from that location, the new location will be appended to the jumplist:

3 first
2 second
1 third
0 new

If the new location is the same

new == second

as some previous (but not immediately prior) entry in the jumplist,

2 first
1 second
0 third <-- current location
1 fourth
2 fifth

both occurrences preserved

3 first
2 second
1 third
0 second (new)

when moving forward from that location.

It would be desireable to go farther and, when the new location is the same as the location that is currently next in the jumplist,

new == fourth

make the result of navigating to the new location by navigating directly (e.g. 50gg) be the same as moving forward in the jumplist

2 first
1 second
0 third
1 new <-- current location
2 fifth

and simply increment the jumplist index. That change is NOT part of this patch because it would require passing the new cursor location to the function (setpcmark) from all of its callees. That in turn would require those callees to know before calling what the new cursor location is, which they do not currently.

justinmk · 2019-12-08T02:25:30Z

Looks good except:

instead of g:foo variable, introduce a new jumpoptions option, which takes a list of strings. See 'wildoptions' for example.
src/nvim/testdir/test_jumplist.vim is a Vim old test. Do not add new tests there, it makes merging upstream patches more difficult. Add new tests to test/functional.... See test/functional/example_spec.lua for an example.
- You can run the test like this:
```
TEST_FILE=test/functional/example_spec.lua make functionaltest
```

butwerenotthereyet · 2019-12-08T21:29:31Z

@justinmk Thank you!

There's now jumpoptions and this behavior occurs when jumpoptions includes history rather than when g:jumplistmode equals history: a0740dc . The tests are now in functionaltest/ui: e5505c8.

Is the next step to document on jumpoptions?

src/nvim/mark.c

src/nvim/option_defs.h

test/functional/ui/jumpoptions_spec.lua

src/nvim/mark.c

justinmk · 2019-12-08T21:57:43Z

Is the next step to document on jumpoptions?

yes.

Also, calling this "history" is pretty misleading since that could describe the old behavior as well. Perhaps "stack" is a better term? Browser-like behavior pops items off the stack, whereas Vim-like behavior is always additive. In fact, this essentially makes ctrl-o/ctrl-i behave like the 'tagstack', which (as mentioned in https://vi.stackexchange.com/q/18344/578 ) is what ctrl-t already does.

butwerenotthereyet · 2019-12-09T00:00:34Z

calling this "history" is pretty misleading since that could describe the old behavior as well. Perhaps "stack" is a better term? Browser-like behavior pops items off the stack, whereas Vim-like behavior is always additive.

Agreed, "stack" fits much better with existing Vim terminology. The option is now ( 3aca2ea ) stack.

In fact, this essentially makes ctrl-o/ctrl-i behave like the 'tagstack', which (as mentioned in https://vi.stackexchange.com/q/18344/578 ) is what ctrl-t already does.

The first-pass on the documentation now mentions that adding stack to jumpoptions makes the jumplist behave like the tagstack.

justinmk · 2019-12-09T03:05:51Z

src/nvim/mark.c

@@ -1204,16 +1214,27 @@ void cleanup_jumplist(win_T *wp, bool checktail)
        break;
      }
    }
-    if (i >= wp->w_jumplistlen) {  // no duplicate
+    bool mustfree = false;


this is a "dead assignment", static analysis will flag it.

Suggested change

bool mustfree = false;

bool mustfree;

I would probably lean towards a ternary or some other way to be less verbose in the logic below, but I suppose that could turn out worse.

Fixed the dead store in 7f29497 . The less verbose versions I've been able to come up with so far have been less readable (highly subjective), but if there's a better way to write this let's do it.

runtime/doc/motion.txt

runtime/doc/options.txt

runtime/doc/motion.txt

src/nvim/option.c

justinmk

LGTM. Nice attention to detail--thanks for taking care about formatting the tests and other details :)

todo: mention this in vim_diff.txt

test/functional/normal/jump_spec.lua

Traditionally, when navigating to a specific location from the middle of the jumplist results in shifting the current location to the bottom of the list and adding the new location after it. This behavior is not desireable to all users--see, for example https://vi.stackexchange.com/questions/18344/how-to-change-jumplist-behavior. Here, another jumplist behavior is introduced. When jumpoptions (a new option set added here) includes stack, the jumplist behaves like the tagstack or like history in a web browser. That is, when navigating to a location from the middle of the jumplist 2 first 1 second 0 third <-- current location 1 fourth 2 fifth to a new location the locations after the current location in the jump list are discarded 2 first 1 second 0 third <-- current location The result is that when moving forward from that location, the new location will be appended to the jumplist: 3 first 2 second 1 third 0 new If the new location is the same new == second as some previous (but not immediately prior) entry in the jumplist, 2 first 1 second 0 third <-- current location 1 fourth 2 fifth both occurrences preserved 3 first 2 second 1 third 0 second (new) when moving forward from that location. It would be desireable to go farther and, when the new location is the same as the location that is currently next in the jumplist, new == fourth make the result of navigating to the new location by jumping (e.g. 50gg) be the same as moving forward in the jumplist 2 first 1 second 0 third 1 new <-- current location 2 fifth and simply increment the jumplist index. That change is NOT part of this patch because it would require passing the new cursor location to the function (setpcmark) from all of its callees. That in turn would require those callees to know *before* calling what the new cursor location is, which do they do not currently.

butwerenotthereyet · 2019-12-09T04:14:16Z

@justinmk Thank you for all the guidance.

mention this in vim_diff.txt

There's now a mention of jumpoptions and jumplist-stack in vimdiff in the Changed Feature section under "motion". Is that what you had in mind?

MaskRay · 2020-01-19T19:11:35Z

Is the following the traditional behavior?

first
second
fourth
fifth
third  # shifting the current location to the bottom of the list 
new    # and adding the new location after it

epheien · 2020-01-20T03:13:15Z

Is the following the traditional behavior?

first
second
fourth
fifth
third  # shifting the current location to the bottom of the list 
new    # and adding the new location after it

Almost like this

Problem: not possible to use the jumplist like a stack Solution: Add the 'jumpoptions' setting to make the jumplist a stack. Add an option for using jumplist like tag stack related: #7738 closes: #13134 ported from NeoVim: - https://neovim.io/doc/user/motion.html#jumplist-stack - neovim/neovim@39094b3 - neovim/neovim#11530 - https://vi.stackexchange.com/questions/18344/how-to-change-jumplist-behavior Based on the feedback in the previous PR, it looks like many people like this option. Signed-off-by: Christian Brabandt <cb@256bit.org> Co-authored-by: Yegappan Lakshmanan <yegappan@yahoo.com> Co-authored-by: butwerenotthereyet <58348703+butwerenotthereyet@users.noreply.github.com>

butwerenotthereyet changed the title ~~jumplist: enable browser-history-style navigation~~ [RFC] jumplist: enable browser-history-style navigation Dec 7, 2019

marvim added the RFC label Dec 7, 2019

butwerenotthereyet force-pushed the jumplist-history branch from 40916de to 5cc2e5d Compare December 7, 2019 22:07

justinmk added core Nvim core functionality or code enhancement feature request labels Dec 8, 2019

justinmk added the needs:response waiting for reply from the author label Dec 8, 2019