Add dismiss command for the completer #13771
Conversation
…triggered by ESC on completer widget
…dget ('dismiss' in manager)
|
Thanks for making a pull request to jupyterlab! |
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
…triggered by ESC on completer widget
…dget ('dismiss' in manager)
typo Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
…ole; added ovverideEsc to fileeditor (to avoid documentsearch:end on Esc).
# Conflicts: # packages/completer/src/manager.ts # packages/fileeditor-extension/schema/completer.json # packages/fileeditor-extension/src/commands.ts
|
I added the controller:dismiss-* for all 3 cases, notebook, console and text editor. For the first two I left the selector very specific, jp-mod-completer-active, because I did not want it to pass by the completer if no completer is yet invoked (say you press esc on the console line, I want it to just do what it did before, default of console, not go involve the completer commands). For the case of the fileeditor, the default was the problem (going to documentsearch:end) so I created a new Esc binding which does nothing, leaving the completer:dismiss-file to touch only active completer widgets. |
…n-linux.json; added label for override esc.
| /** | ||
| * Override Esc that causes documentsearch:end, doing nothing | ||
| */ | ||
| commands.addCommand(CommandIDs.overrideEsc, { | ||
| execute: () => { | ||
| return; | ||
| }, | ||
| label: trans.__('Override Esc') | ||
| }); |
There was a problem hiding this comment.
Instead of adding this command we probably should modify how documentsearch:end behaves, e.g. by modifying selector.
There was a problem hiding this comment.
I'm not sure we want to change the documentsearch:end... Its selector is jp-mod-searchable, which includes the container of the whole file editor content page (as it should be, I think).
That is, if I remove the overrideEsc, then pressing esc anywhere else on the sheet (when neither completer or search widget is opened) will go back to the documentsearch:end warning. The warning comes up because indeed no search widget is enabled. One could get rid of the warning :D
I can remove the completer-active part of the selector, that way Esc will always pass through the completer:dismiss-file, even when no completer has been invoked (but maintain the end command if a search widget is opened).
Override Esc or always use completer (or the Escape binding from some other package), I can't come up with a third option...
|
I tried changing the selector of documentsearch:end to .jp-DocumentSearch-input, but then my search widget was broken in dev-mode: if any letter typed into search is found in the text, focus changes to the match in the text and one can only overwrite the found result. Cannot get the focus back into the find-view after that and even after closing the widget, matches stay highlighted and focus can't be moved. |
Was that in the file editor? There are two bugs in the latest version:
which are mostly affecting the file editor. You can safely ignore the file editor for now. |
…tSearch-input; removed all completer:dismiss-* from notebook, console and fileeditor as no longer necessary.
github-actions
bot
added
pkg:documentsearch
and removed
tag:Testing
pkg:console
pkg:completer
labels
…s jp-mod-searchable, so only when search widget is active.
|
I have nothing to add. Every time i update the branch, it generates new errors, different from the previous ones. I don't know how to fix them. |
|
I'm sure you all have seen this, but in my last push the dismiss command for the completer had been removed. The default key binding to Escape, that was already in the completer, is used. I'm just mentioning it because of the branch name change which makes sense if you use one of the previous commits - consider these had some not so nice workarounds as mentioned in previous comments. |
| // Only bind Esc in case the document is currently searchable, | ||
| // else it will clash with Esc to close completer on fileeditor | ||
| app.commands.addKeyBinding({ | ||
| command: CommandIDs.end, | ||
| keys: ['Escape'], | ||
| selector: '.jp-mod-searchable' | ||
| }); |
There was a problem hiding this comment.
I don't believe it is doing what you think it does. Once the command is added it will remain there forever. Hence this will only "solve" the problem for the very dismissals of completer which happen before first opening of search panel. Otherwise, it is the same as defining it in schema, but prevents user customisation, so we should not do it.
There was a problem hiding this comment.
I guess one could remove the command each time the search widget is closed, each time the texteditor widget loses the "searchable" class... But I guess it's not coherent with the overall style.
I'm guessing you can use or not use whatever commit of mine that you find useful, right, or do you want me to do a rollback to the version with the dismiss command for the completer?
|
Thank you for opening PR and perseverance! The issue it was trying to solve was closed because another PR addressed the problem using a different approach (#13659 (comment)) and a number of changes on key binding handling were since made. I need to admit that there was an error of judgement on my side when I labelled the original issue as "good first issue" - it clearly was a difficult one - apologies. Since the relevant issue was resolved and this PR has conflicts/is not ready I will close it for now , but please feel welcome to open another PR if you would like to contribute in the future! |
|
Thanks for the message, I had completely forgotten about this (apologies), as I had probably just given up. Doesn't mean it wasn't a good first issue, just that I had not understood it. Thanks for all your feedback! |
References
Issue #13659
Code changes
Added dismiss-widget command to the completer package that gets invoked by Escape in file-editor.
Before, Escape went to document-search:end without there being a search view opened, so the command was not enabled. Now the dismiss command is enabled with the completer.
Dismiss-widget uses dispose() from the completer widget.
User-facing changes
console warning gone.
Backwards-incompatible changes