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

Improve focus behaviour in search #14387

Merged
merged 14 commits into from
May 4, 2023

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Apr 16, 2023

References

Fixes #14308

Depends on codemirror/view@f49f2ee included in @codemirror/view 6.9.5, therefore required #14413

Code changes

  • only create DOM event (i.e. focus) CM extension once, and only add it during editor setup phase
  • support arbitrary nesting of highlight markers fixing current highlight not shown in some cases
  • preserve current match when switching cells if possible
  • if there is no current match, try to assign one
  • do not ever set selection when searching in selection, instead just scroll to the match
  • introduces (non-public) SearchStartAnchor type to disambiguate the search start behaviour allowing to fix highlighting issues
  • introduces IHighlightAdjacentMatchOptions interface to specify whether highlighting should scroll/select new match
  • do not scroll/select new match when starting search
    • this can be improved in follow-up: we could scroll/select if this is a new query (but not restart of query after document change)

User-facing changes

Extending selection after changing to command mode now works smoothly (does not revert to edit mode, which was caused by CodeMirror bug, and is faster - which was previously slowed down by needless cleaning of highlights due to both activeCellChanged and selectionCellChanged handlers being used - now the shared job will only be performed once).

Initial match is set consistently across various scenarios, e.g.:

Before After
image image

Including, in file editor, and when populating search box from selection (regardless of selection direction):

image

Backwards-incompatible changes

N/A

@krassowski krassowski added the bug label Apr 16, 2023
@krassowski krassowski added this to the 4.0.0 milestone Apr 16, 2023
@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@krassowski
Copy link
Member Author

Merged master to clean update-links job failure that was addressed earlier.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks a lot @krassowski for working on this.

galata/test/jupyterlab/notebook-search.test.ts Outdated Show resolved Hide resolved
packages/codemirror/src/searchprovider.ts Outdated Show resolved Hide resolved
@krassowski
Copy link
Member Author

@andrii-i can we merge this one or did you have any specific feedback here?

@fcollonval
Copy link
Member

Merging now to get it in the RC

@fcollonval fcollonval merged commit 6b1069c into jupyterlab:master May 4, 2023
@andrii-i
Copy link
Contributor

andrii-i commented May 4, 2023

@krassowski Good to merge, no feedback on my end, it was mentioned as needing a review last week but I have not came around to reviewing. Thanks for reviewing @fcollonval.

krassowski added a commit to krassowski/jupyterlab that referenced this pull request May 8, 2023
which included jupyterlab#14387 enabling to finish the tests off
fcollonval pushed a commit that referenced this pull request Jul 17, 2023
* Draft for automatically toggling search in selection

* Implement `jp-mod-search-active` class, add `await`s

* Draft unit tests for `getSelectionState` (currently failing)

* Harmonise setting values to end with `-selected`

* Interpret having an active cell as not having any selected cells

for purposes of auto-selection toggling, as this would always
toggle search in cell selection (which we can revisit if we get
user feedback).

* Add integration tests for auto-selection and toggle shortcut

* Fix typos, polish docs

* Attempt to fix flaky test

* Update test after merge with master

which included #14387 enabling to finish the tests off

* Update shortcuts snapshot (not sure why it changes)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search "in selected" sometimes reverts to edit mode preventing cell selection
3 participants