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

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

Merged
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
Copy link
Contributor

github-actions bot commented Jul 18, 2024

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?

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.

@HeikoKlare
Copy link
Contributor

HeikoKlare commented Jul 19, 2024

@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
Copy link
Contributor Author

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

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.

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
lathapatil pushed a commit to lathapatil/eclipse.platform.ui that referenced this pull request Oct 21, 2024
…-platform#2104)

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

fixes eclipse-platform#1913
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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