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.

Sign up for GitHub

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

Jump to bottom

Fix replace when replacement text matches source text multiple times #16258

Merged
merged 14 commits into from May 6, 2024
Merged

Fix replace when replacement text matches source text multiple times #16258

merged 14 commits into from May 6, 2024

Conversation

JasonWeill
Copy link
Contributor

References

Fixes #16242.

Code changes

Advances to the next match when the replacement text matches the source text at least twice.

(This is a draft PR with tests created first.)

User-facing changes

After replacing a search term (e.g., foo) with replacement text that contains the search term multiple times (foofoofoo) the highlight advances to the next match after the replacement text, including if the next match is in a later cell.

Backwards-incompatible changes

None.

@JasonWeill JasonWeill added the bug label Apr 26, 2024
@jupyterlab-probot jupyterlab-probot
Copy link

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

@JasonWeill JasonWeill changed the title Search and replace when replacement text matches source text multiple times WIP: Search and replace when replacement text matches source text multiple times Apr 26, 2024
@JasonWeill JasonWeill changed the title WIP: Search and replace when replacement text matches source text multiple times Search and replace when replacement text matches source text multiple times May 1, 2024
@JasonWeill JasonWeill marked this pull request as ready for review May 1, 2024 00:39
@JasonWeill
Copy link
Contributor Author

JasonWeill commented May 1, 2024 •
edited

With Firefox and Chrome, I see the expected behavior when I replace the search text (e.g., test) with a replacement that matches the search text 0 times (foo), 1 time (tester), 2 times (testtest), and 3 times (testtesttest), but the UI tests in the pipeline fail reliably. Not sure what's causing this, but I'm promoting this from a draft to ready for review.

(Update: After a rebase, the 2x and 3x cases are not working again :( )

@JasonWeill JasonWeill marked this pull request as draft May 1, 2024 17:09
@JasonWeill
Copy link
Contributor Author

The most recent commits fix the 0-match, 1-match, 2-match, and 3-match cases on my desktop.

@JasonWeill
Copy link
Contributor Author

Still seeing unit test failures; I might have to test again after a rebase.

@JasonWeill JasonWeill marked this pull request as ready for review May 3, 2024 23:19
@JasonWeill
Copy link
Contributor Author

All tests related to search and replace are now passing; this PR is now ready for review.

krassowski
krassowski approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Looks good locally, thank you @JasonWeill!

@krassowski krassowski added this to the 4.2.0 milestone May 6, 2024
@krassowski krassowski changed the title Search and replace when replacement text matches source text multiple times Fix replace when replacement text matches source text multiple times May 6, 2024
@krassowski krassowski merged commit 6922310 into jupyterlab:main May 6, 2024
82 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krassowski krassowski krassowski approved these changes

@fcollonval fcollonval Awaiting requested review from fcollonval

Assignees

@JasonWeill JasonWeill

Labels
bug pkg:codemirror pkg:notebook tag:Testing
Projects
None yet
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.

Search highlight does not move to next cell when replacing a with aa
2 participants
@JasonWeill @krassowski