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

Create a unified editor search provider #13884

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Jan 29, 2023

References

Fixes #13283 and provides a tentative fix for #13885

Also fixes #13282

Code changes

Simplifies searching in file editor by removing CodeMirrorSearchProvider and replacing it with abstract EditorSearchProvider which expects subclasses to define editor and model properties. CellSearchProvider and FileEditorSearchProvider now inherit from the new EditorSearchProvider which is essentially identical to the old CellSearchProvider.

User-facing changes

Searching in editor works reliably again.

Backwards-incompatible changes

@krassowski krassowski added the bug label Jan 29, 2023
@krassowski krassowski added this to the 4.0.0 milestone Jan 29, 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 krassowski marked this pull request as ready for review January 29, 2023 23:05
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 @krassowski

packages/codemirror/src/searchprovider.ts Show resolved Hide resolved
packages/fileeditor/src/searchprovider.ts Outdated Show resolved Hide resolved
packages/fileeditor/src/searchprovider.ts Show resolved Hide resolved
packages/fileeditor/src/searchprovider.ts Show resolved Hide resolved
packages/codemirror/src/searchprovider.ts Show resolved Hide resolved
packages/codemirror/src/searchprovider.ts Outdated Show resolved Hide resolved
packages/codemirror/src/searchprovider.ts Show resolved Hide resolved
packages/codemirror/src/searchprovider.ts Show resolved Hide resolved
packages/cells/src/searchprovider.ts Show resolved Hide resolved
packages/cells/src/searchprovider.ts Show resolved Hide resolved
@krassowski krassowski marked this pull request as draft February 7, 2023 13:28
@krassowski krassowski changed the title Create a united editor search provider Create a unified editor search provider Feb 12, 2023
For example, if we had text: `test test test` and replaced first `test`
with `a` it would now be `a tes|t test` where `|` indicates cursor
position set by `_lastReplacementPosition` (at fifth character)
which would lead to the last `test` being highlighted rather
than the second-to-last one) and effectively one result being
omitted in consecutive replacements.
@krassowski krassowski force-pushed the create-shared-search-provider-for-editors branch from 0d78d40 to 3b40b0f Compare February 13, 2023 20:16
@krassowski
Copy link
Member Author

krassowski commented Feb 13, 2023

I've increased timeout on galata update workflow, let's see if this helps.

bot please update galata snapshots

@fcollonval
Copy link
Member

bot please update galata snapshots

@krassowski
Copy link
Member Author

bot can I very kindly ask to please update snapshots

@fcollonval
Copy link
Member

@krassowski I have the impression there is something about this PR breaking the update job. I ran the update job successfully on a recent PR yesterday: https://github.com/jupyterlab/jupyterlab/actions/runs/4172386115/jobs/7223418464

Did you try running the update for your new test locally? Hopefully that may give us some idea.

@krassowski
Copy link
Member Author

@fcollonval no, it is not specific to this PR, see that the Galata update job also failed on another PR after this comment #13964 (comment)

link to the cancelled (timed out) job:
https://github.com/jupyterlab/jupyterlab/actions/runs/4171947506/jobs/7222458570

I ran the update job successfully on a recent PR yesterday: https://github.com/jupyterlab/jupyterlab/actions/runs/4172386115/jobs/7223418464

The link you posted only shows documentation update. That works fine. The main galata one does not.

Did you try running the update for your new test locally?

Yes, it works locally.

@fcollonval
Copy link
Member

Thanks for the additional cases; so it seems the latest job that succeed if from 5 days ago: https://github.com/jupyterlab/jupyterlab/actions/runs/4135509044/jobs/7148065705 - it uses the same Playwright version; so let's rule that out as sources for now.

The jobs for this PR and the one you referenced have the same strange timeouts in the few jobs that got executed:

·/home/runner/work/jupyterlab/jupyterlab/galata/test/documentation/customization.test.ts-snapshots/default-terminal-position-single-documentation-linux.png does not match, writing actual.
·××T/home/runner/work/jupyterlab/jupyterlab/galata/test/documentation/customization.test.ts-snapshots/default-menu-bar-documentation-linux.png does not match, writing actual.
·××T/home/runner/work/jupyterlab/jupyterlab/galata/test/documentation/customization.test.ts-snapshots/customized-terminal-position-single-documentation-linux.png does not match, writing actual.
·××T/home/runner/work/jupyterlab/jupyterlab/galata/test/documentation/customization.test.ts-snapshots/customized-menu-bar-documentation-linux.png does not match, writing actual.
·××T

So many timeouts are unexpected when updating snapshots. But the worst is that the log is telling us it is updating documentation snapshots 🤯

I'll look to that locally.

@fcollonval
Copy link
Member

fcollonval commented Feb 14, 2023

Ok this is my fault - in #13909 I updated the default playwright config and forget to update the test:update script in galata that is used by default. So it is now trying to execute the three subprojects documentation, galata and jupyterlab.

@fcollonval
Copy link
Member

bot please update galata snapshots

@fcollonval
Copy link
Member

@krassowski I gave a try to rebase this PR to fix the snapshots. But I was not able to get a proper branch 😕 I let you do it as I don't want to mess your repo. Sorry to not be able to help you more on this.

@krassowski
Copy link
Member Author

bot please update galata snapshots

@krassowski
Copy link
Member Author

Thanks for the help here, it is greatly appreciated! I am not sure if this will do it, if it still fails I will look into robustifying the completer test.

@github-actions
Copy link
Contributor

Galata snapshots updated.

@krassowski krassowski closed this Feb 15, 2023
@krassowski krassowski reopened this Feb 15, 2023
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 @krassowski

The UI test is due to flakiness but the JS test is legit.

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 for the patience Mike, let's merge this.

CI failure is unrelated.

@krassowski krassowski merged commit a90edc8 into jupyterlab:master Feb 16, 2023
76 of 78 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.