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

Find/Replace Logic: Allow reset of Incremental Base Position #2104

Merged
merged 1 commit into from
Jul 19, 2024
Merged

Find/Replace Logic: Allow reset of Incremental Base Position #2104

merged 1 commit into from
Jul 19, 2024

Conversation

Wittmaxi
Copy link
Contributor

Allow the client to reset the base position for incremental search without enabling/disabling the Incremental search.

fixes #1913

@Wittmaxi Wittmaxi requested a review from HeikoKlare July 18, 2024 12:19
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 18, 2024 •
edited
Loading

Test Results

 1 815 files  +  601   1 815 suites  +601   1h 35m 54s ⏱️ + 19m 14s
 7 678 tests +    1   7 450 ✅ +  251  228 💤  -   5  0 ❌ ±0 
24 195 runs  +7 043  23 446 ✅ +7 072  749 💤 +226  0 ❌ ±0 

Results for commit ecb803a. ± Comparison against base commit 8534560.

♻️ This comment has been updated with latest results.

@Wittmaxi
Copy link
Contributor Author

@HeikoKlare I am not sure what is failing here. Can you please have a look?

HeikoKlare
HeikoKlare reviewed Jul 19, 2024
View reviewed changes
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

The change is almost good to go. It provides a dedicated way of resetting the incremental base position, which removes a workaround in the find/replace overlay. I only have two minor comments regarding documentation.

Just one further thought on the design of the FindReplaceLogic: In the long term, I hope that we find a design which does not require the consumer of the FindReplaceLogic to think about when to reset the incremental base position, but that the logic provides a proper level of abstraction in it's functionality so that resetting the incremental base position can be hidden within it. But let's see if that will be possible and reasonable, or if that would require to increase the level of abstraction of the FindReplaceLogic too far. With this change, we can first make the behavior of incremental base position updates consistent and provide regression proper tests before maybe improving the design again.

...pse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogic.java Show resolved Hide resolved
...se.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/IFindReplaceLogic.java Outdated Show resolved Hide resolved
@HeikoKlare
Copy link
Contributor

HeikoKlare commented Jul 19, 2024 •
edited
Loading

@HeikoKlare I am not sure what is failing here. Can you please have a look?

The failure was because of this issue: eclipse-pde/eclipse.pde#1310

Note that this kind of failure makes the build for that OS fail immediately, so you should restart the job as you might miss broken tests or other failures specific to that OS otherwise.

@Wittmaxi Wittmaxi mentioned this pull request Jul 19, 2024
Merged
@Wittmaxi
Copy link
Contributor Author

@HeikoKlare once the tests run through and you give me your OK, I will merge this

HeikoKlare
HeikoKlare approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

Thank you for the final improvement! This is good to go from my side.

Feel free to merge once the builds/checks have succeeded.

@Wittmaxi @HeikoKlare
Find/Replace Logic: Allow reset of Incremental Base Position
c6db852 
Allow the client to reset the base position for incremental search
without enabling/disabling the Incremental search.

fixes eclipse-platform#1913
@Wittmaxi Wittmaxi merged commit 56bca9c into eclipse-platform:master Jul 19, 2024
15 checks passed
@HeikoKlare HeikoKlare mentioned this pull request Jul 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeikoKlare HeikoKlare HeikoKlare approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[find/replace logic] cannot easily reset the Incremental Base-Location
2 participants
@Wittmaxi @HeikoKlare