Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix modifyCell or modifyEditor not called after text editor is opened after notebook #105

Conversation

firai
Copy link
Collaborator

@firai firai commented Sep 3, 2023

Fixes #103.

On ILabShell currentChanged(), check if currentWidget matches one tracked by editorTracker or notebookTracker, and call modifyEditor() or modifyCell() accordingly.

@krassowski, can you please help review whether this is right approach?

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

Binder 👈 Launch a Binder on branch firai/jupyterlab-vim/modifyCell-when-changing-active-widget

@firai firai closed this Sep 3, 2023
@firai firai reopened this Sep 3, 2023
@firai firai changed the title Connect notebookTracker currentChanged to editorManager Fix modifyCell or modifyEditor not called after text editor is opened after notebook Sep 3, 2023
Copy link
Collaborator

@krassowski krassowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in principle it looks right, though I would avoid console.warn in final version to keep the dev console usable. Does it fix the problem?

@krassowski krassowski added the bug Something isn't working label Sep 3, 2023
@firai
Copy link
Collaborator Author

firai commented Sep 3, 2023

Yes, as far as I can tell. The code block and the console.warn were copied from here:

app.commands.addCommand('vim:enter-normal-mode', {
label: 'Enter Normal Vim Mode',
execute: () => {
const current = app.shell.currentWidget;
if (!current) {
console.warn('Current widget not found');
} else if (editorTracker.currentWidget === current) {
editorManager.modifyEditor(editorTracker.currentWidget.content.editor);
} else if (notebookTracker.currentWidget === current) {
cellManager.modifyCell(
notebookTracker.currentWidget.content.activeCell
);
} else {
console.warn('Current widget is not vim-enabled');
}
},
isEnabled: () => enabled
});

Do you have any suggestions on what the console.warn should be replaced with? Also, I suppose I'm supposed to refactor this out into a function so that it isn't duplicated?

@krassowski
Copy link
Collaborator

Well, the difference is that when vim:enter-normal-mode is run we expect the current widget to be vim-enabled, but shell.currentChanged will fire for any kind of a widget. I would say you could use console.debug or just remove it (if you want to leave the logic for clarity you can add // no-op comment).

IMO duplication is fine as these are slightly different contexts.

@firai
Copy link
Collaborator Author

firai commented Sep 3, 2023

The build process fails without the if (!current) code path, so I'll replace with no-op comments

@firai firai marked this pull request as ready for review September 3, 2023 16:35
@krassowski krassowski changed the title Fix modifyCell or modifyEditor not called after text editor is opened after notebook Fix modifyCell or modifyEditor not called after text editor is opened after notebook Sep 3, 2023
Copy link
Collaborator

@krassowski krassowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @firai. Tested on binder, works well.

@krassowski krassowski merged commit 2078dc0 into jupyterlab-contrib:master Sep 3, 2023
6 checks passed
@firai firai deleted the modifyCell-when-changing-active-widget branch September 4, 2023 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opening text editor after notebook causes normal mode j & k in cells to be stuck within a cell
2 participants