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

Support find widget in notebooks #13982

Merged
merged 4 commits into from
Aug 5, 2024
Merged

Support find widget in notebooks #13982

merged 4 commits into from
Aug 5, 2024

Conversation

msujew
Copy link
Member

@msujew msujew commented Aug 1, 2024

What it does

Adds support for a monaco-like find widget to the notebook widget.

How to test

The search widget can be brought up by pressing Ctrl+F inside of any area of the notebook widget. Having the Jupyter Keymaps extension installed also allows to bring it up by just pressing F outside of an editor area (while the notebook widget is focused).

Multiple test scenarios:

  • In general: Test that the widget behaves similarly to the existing find widget for text editors.
  • Select text in a monaco editor and press Ctrl+F. The selected text should be placed as the initial search value of the find widget.
  • Search for text in code. The code should be highlighted (with the current selection getting a special highlighting) and jumping to different matches should work.
  • Search using match case, word boundaries and regex should work as expected.
  • Search for text in rendered markdown cells. The text should be highlighted directly in the HTML. Jumping should work as well.
  • Replacing code in the code editors should work as expected.
  • Replacing code in the rendered markdown cells should not be possible (button greyed out).
  • Upon changing the editing mode of a markdown cell, the search should update and highlight the now rendered code editor instead (also in reverse).

Follow-ups

Known issues:

  • Cannot find in webviews
  • Cannot replace on rendered markdown
  • There is no global undo for the global replace yet. Each cell change needs to be undone individually.
  • Performance for huge rendered markdown is quite bad

The current API is pretty suboptimal (especially for finding matches inside of rendered markdown cells). I plan to get back to this later to revise the API and extract a common FindWidget that can be used by any widget.

Review checklist

Reminder for reviewers

@msujew msujew added the notebook issues related to notebooks label Aug 1, 2024
@msujew msujew requested a review from jbicker August 1, 2024 13:45
Copy link
Contributor

@jbicker jbicker left a comment

Choose a reason for hiding this comment

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

  • After step by step replacement => 'NaN of [number]' appeared in find widget, then 'find' is not working at all anymore.
    FandR-NaN
    Reproduce: find and replace all occurrences step by step until there is finally 'No Results' in counter field. Change to the find input again and begin to type. 'NaN of [number of occurrences]' appears and highlighting does not work anymore.
  • If the last occurrence has been replaced (if available, for example if "great" is replaced with "super great"), it must jump back to the first occurrence. At the moment the search remains at the last occurrence.
  • pasting a search term from clipboard into find input does not work
  • When playing around with markdown: if markdown is rendered it sometimes doesn't find occurrences when using match case, word boundaries and/or regex, (unfortunately I can't find a reliable way to reproduce it but maybe it has something to do with the currently pretty suboptimal API you've mentioned)
  • - [ ] CMD-Option-F resp. Strg-Alt-F does not work (should open 'find and replace')

@msujew
Copy link
Member Author

msujew commented Aug 3, 2024

Thanks @jbicker, I believe I fixed every discovered issue except for one:

CMD-Option-F resp. Strg-Alt-F does not work (should open 'find and replace')

This one isn't supported in VS Code (in notebooks) and I'm not sure adding a keybinding for this is a good idea. It might accidentally break some keybindings that I'm not aware of.

Copy link
Contributor

@jbicker jbicker left a comment

Choose a reason for hiding this comment

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

LGTM. Everything works as expected.

@msujew msujew merged commit 8d247e0 into master Aug 5, 2024
11 of 14 checks passed
@msujew msujew deleted the msujew/notebook-find branch August 5, 2024 13:13
@github-actions github-actions bot added this to the 1.53.0 milestone Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notebook issues related to notebooks
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants