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 search coming back in notebook and editor #15443

Merged
merged 13 commits into from Dec 13, 2023

Conversation

krassowski
Copy link
Member

References

Builds on the test proposed in #15259 (thanks @parmentelat)
Fixes #14871

Code changes

Track the search on/off state and only announce state changes/act on them if the search is on.

User-facing changes

Search does not come back in notebook and file editor

Backwards-incompatible changes

None

@krassowski krassowski added this to the 4.0.x milestone Nov 27, 2023
Copy link

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

@parmentelat
Copy link
Contributor

I just gave this a manual test
as binder was taking 10 minutes+, I went for a local install and did this

  • went to commit 17b52c5
  • jlpm install
  • jlpm build
    with that in place I kicked a new jupyter lab

however I could not see any improvement, the searched terms keeps on being highlighted even when the search dialog is closed

so I take it the way I am trying to test is not right, is it ?

@krassowski
Copy link
Member Author

jupyter lab --dev-mode (do you see the orange dev mode indicator at the top)?

If you still see it in the dev mode, can you record a screencast?

@parmentelat
Copy link
Contributor

stupid me, I am more into developing extensions these days and I forgot about the --dev-mode (mental slap on the forehead ;)

all seems to be in order now; I'll try to give it a more thorough shot later on then :)

@parmentelat
Copy link
Contributor

just to confirm, I have been able to test this manually with larger notebooks and so far I have not seen the search dialog reappearing unintentionally, so it's all good as far as I am concerned; at the very least it is way better than before :)

thanks a lot @krassowski for fixing this, it is a huge burden off heavy users who use Search all the time

ps1. I tried to analyse the outcome of CI that has many failures, it does not seem to relate to these changes but I could not say for sure

ps2. just as a comment, not quite related to the topic at hand, but
I think I remember having seen issues where people were not at ease with the search feature switching cells in edit mode
I think I understand the need to do that in cases where the search pattern would be present in the cell source but not in the rendered version; however in the vast majority of cases, it's not the case, so after all these manual tests, it's my feeling that this issue is maybe next thing that could be improved :)

@krassowski
Copy link
Member Author

ps1. I tried to analyse the outcome of CI that has many failures, it does not seem to relate to these changes but I could not say for sure

No worries, I will take care of these. Briefly there are two issues: (a) we need to update snapshots as new PRs were merged since and the UI looks slightly different (b) parallel execution of tests on CI is breaking some test cases which is being worked on elsewhere.

I think I remember having seen issues where people were not at ease with the search feature switching cells in edit mode
I think I understand the need to do that in cases where the search pattern would be present in the cell source but not in the rendered version; however in the vast majority of cases, it's not the case, so after all these manual tests, it's my feeling that this issue is maybe next thing that could be improved :)

I agree from the UX point of view. Having it not touch render state and showing a tooltip "3 more matches in source code" could be a good solution IMO. If you cannot find an issue about this, please create a new one.

@krassowski
Copy link
Member Author

krassowski commented Nov 30, 2023

bot please update galata snapshots again

Copy link
Contributor

Galata snapshots updated.

and revert spurious snapshot updates
@jupyterlab jupyterlab deleted a comment from github-actions bot Nov 30, 2023
@krassowski krassowski marked this pull request as ready for review November 30, 2023 20:13
@parmentelat
Copy link
Contributor

@krassowski
thanks for youor invitation to review this PR; I can't say that I feel exactly relevant, but FWIW I do not see anything sticking out in the code changes

in any case I'd like to thank you a million times again for taking care of this nagging issue, I am eager to see this released so I can stop writing junk in the search dialog only to keep the issue from happening ;-)

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 at @krassowski

@krassowski krassowski merged commit 746ce2d into jupyterlab:main Dec 13, 2023
78 of 81 checks passed
@krassowski
Copy link
Member Author

@meeseeksdev please backport to 4.0.x

Copy link

lumberbot-app bot commented Dec 13, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 4.0.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 746ce2d75652a207fa1cdac07dd05854ac15eafb
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #15443: Fix search coming back in notebook and editor'
  1. Push to a named branch:
git push YOURFORK 4.0.x:auto-backport-of-pr-15443-on-4.0.x
  1. Create a PR against branch 4.0.x, I would have named this PR:

"Backport PR #15443 on branch 4.0.x (Fix search coming back in notebook and editor)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

krassowski added a commit to krassowski/jupyterlab that referenced this pull request Dec 26, 2023
…editor

* a test case to illustrate issue jupyterlab#14871

* cleanup unused code

* rename outline into highlight as it sounds more proper

* fix typo in variable name _unrenderedByHighligh → _unrenderedByHighlight

* Fix search coming back in notebook and editor

* Add missing return

* Rename snapshots and limit screenshot to notebook area

Remove no-op await locator line

* Update Playwright Snapshots

* Improve async test implementation to avoid early closing

* Update Playwright Snapshots

* Remove visual snapshots, use locator counts instead

and revert spurious snapshot updates

---------

Co-authored-by: Thierry Parmentelat <thierry.parmentelat@inria.fr>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit 746ce2d)
krassowski added a commit that referenced this pull request Dec 27, 2023
…5562)

* a test case to illustrate issue #14871

* cleanup unused code

* rename outline into highlight as it sounds more proper

* fix typo in variable name _unrenderedByHighligh → _unrenderedByHighlight

* Fix search coming back in notebook and editor

* Add missing return

* Rename snapshots and limit screenshot to notebook area

Remove no-op await locator line

* Update Playwright Snapshots

* Improve async test implementation to avoid early closing

* Update Playwright Snapshots

* Remove visual snapshots, use locator counts instead

and revert spurious snapshot updates

---------

Co-authored-by: Thierry Parmentelat <thierry.parmentelat@inria.fr>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit 746ce2d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Find/replace highlighting keeps coming back up after find prompt closed
3 participants