jupyterlab-contrib / jupyterlab-vim

Vim notebook cell bindings for JupyterLab
https://jupyterlab-contrib.github.io/jupyterlab-vim.html
MIT License
689 stars 43 forks source link

`gg` and `G` no longer working in `jupyterlab 4.1.0` #134

Closed lukashergt closed 9 months ago

lukashergt commented 9 months ago

Description

After updating from jupyterlab 4.0.11 to jupyterlab 4.1.0, the commands gg and G to navigate to the top/bottom cell of the notebook in jupyter mode no longer seem to work. Going back to jupyterlab 4.0.11 both gg and G work as expected.

Can anybody else confirm this?

Context

ianhi commented 9 months ago

edit: I was able to replicate i misread the issue at first


I was not able to replicate on:

python : 3.12.1 jupyterlab-vim: 4.1.0 jupyterlab : 4.1.0 firefox: 112 (on linux)

Was this in a totally fresh environment?

lukashergt commented 9 months ago

I was not able to replicate on:

python : 3.12.1 jupyterlab-vim: 4.1.0 jupyterlab : 4.1.0 firefox: 112 (on linux)

Huh, weird.

Was this in a totally fresh environment?

Yes, new virtualenv...

lukashergt commented 9 months ago

I tried again with a fresh virtualenv of python version 3.11.5. Same thing, gg and G work in jupyterlab==4.0.11, but do not work in jupyterlab==4.1.0...

petergthatsme commented 9 months ago

I can confirm, and for me other shortcuts stopped working in 4.1.0 as well e.g,. shift+O. I downgraded to 4.0.11 for now, and things are ok. thx.

ianhi commented 9 months ago

ahhh sorry i misread the original issue, i thought you were talking about inside of a cell. I agree I see the same

ianhi commented 9 months ago

ahhh I suspect we need to update the CSS selectors: https://jupyterlab.readthedocs.io/en/latest/extension/extension_migration.html#change-of-notebook-focus-handling-impacting-command-mode-shortcut-selectors

krassowski commented 9 months ago

Yes, but JupyterLab 4.1 was supposed to handle the transition by transparent substitution in the background. I must have missed some selectors, sorry :facepalm:

krassowski commented 9 months ago

So https://github.com/jupyterlab/jupyterlab/pull/15639 should have caught .jp-Notebook:focus usage as in:

https://github.com/jupyterlab-contrib/jupyterlab-vim/blob/fc42ef650142f95aa8f8f5e3b4f82b2fef03a996/schema/plugin.json#L173-L177

krassowski commented 9 months ago

Ok, the selector substitution rules are good but only first shortcut gets fixed (for me Y Y):

image

instead there is a bug in the logic in:

if (
  oldSelector.includes(change.old) &&
  !selectorsAlreadyWarnedAbout.has(oldSelector)
) {
  newSelector = oldSelector.replace(change.old, change.new);
  selectorDeprecationWarnings.add(
    `"${change.old}" was replaced with "${change.new}" in ${change.versionDeprecated} (present in "${oldSelector}")`
  );
  selectorsAlreadyWarnedAbout.add(oldSelector);
}

selectorsAlreadyWarnedAbout should not be checked in the outer if as it skips subsequent shortcuts. I will open a PR against JupyterLab,

krassowski commented 9 months ago

This is getting fixed in:

reviews for both pull requests would be greatly appreciated!

lukashergt commented 9 months ago

I can confirm that with #135 gg and G now jump to the top and bottom cell of the notebook, respectively.

However, it seems the view does not jump. For example, if I am at the bottom of the notebook viewing the bottom cell which is currently highlighted, and I then press gg, then the highlight vanishes (and presumably jumps to the top cell), but I am still looking at the bottom cell. Only after pressing j or k or zz (at least twice actually...) does the view follow and jump to the selected cell...

krassowski commented 9 months ago

Thank you @ianhi for the quick release and @lukashergt for testing. The scroller node has also changed in lab 4.1 (https://jupyterlab.readthedocs.io/en/latest/extension/extension_migration.html#css-class-name-change-in-the-windowedlist-superclass-of-staticnotebook), and instead of using the public API of notebook, this extension was doing scrolling manually:

https://github.com/jupyterlab-contrib/jupyterlab-vim/blob/a6520efded5415c0bdcb4beffab5b77dd3550d51/src/labCommands.ts#L248-L253

I can take a look tomorrow.

ianhi commented 9 months ago

ahhh sorry I didn't catch this. I did not make enough cells in my test notebook to trigger scrolling. I will add that to my mental checklist of things to check.

lukashergt commented 9 months ago

Wow, completely resolved as far as I can tell, thanks for these quick fixes @krassowski, @ianhi!