-
Notifications
You must be signed in to change notification settings - Fork 226
feat : Add global search result navigation shortcuts #3299
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
base: master
Are you sure you want to change the base?
feat : Add global search result navigation shortcuts #3299
Conversation
Here is the demo after making some code changes. Screen.Recording.2025-09-24.at.7.31.34.PM.movBut, When my teammate @ShuWald try on his window laptop, it's working fine with ALT key. Feel free to refer in the issue discussion section. Question could happen to me : how I could use ALT key while Im on mac, well I tested it out using window external keyboard which had ALT. Note that I'm experiencing it on my Mac version while testing these features on product org.eclipse.ide following Click on Window/mac Top Bar Run -> Run Configuration -> Installing Necessary Packages -> Run To review the steps me and @ShuWald to reproduce this feature, feel free to check out our documentation, especially in testing our feature implementation section https://awesome-kumquat-7b7.notion.site/CodeDay-Lab-2220d5c2f5e880539c85de579b8b7310?pvs=74 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Type Casting didn't work for me as well! Gotta remove it!
70320e0
to
fb314d8
Compare
@NikkiAung thanks, will review this shortly. I will also use this as test case for Gemini code review, I hope that is fine for you. |
/gemini review |
Ofc, that sounds good to me! Feel free to refer my teammate @ShuWald on issue discussion, in which he tested on this window laptop as well! :) ![]() |
fb314d8
to
97d2abc
Compare
Hey @vogella, how it's going with the review. I'm super excited to take the feedback and keep on working! :) |
hs.executeCommand(searchCommand, (Event)event.getTrigger()); | ||
hs.executeCommand(IWorkbenchCommandConstants.WINDOW_ACTIVATE_EDITOR, (Event)event.getTrigger()); | ||
} catch (NotDefinedException e) { | ||
throw new ExecutionException(e.getMessage(), e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use multi-catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will take of it! :)
Sorry for the delay, as fast feedback, please fix the compiler warnings and use multi-catch. Can event.getTrigger() be null in this scenario? If yes, this is not checked. Would it be possible to add unit tests for this new functionality? |
For compile error, I'm wondering if using Last time, we followed your documentation about setting up eclipse in dev environment, which was really useful. |
Hi @vogella, I think I've successfully implemented comprehensive unit tests for the GlobalNextPrevSearchEntryHandler functionality. Created 2 test classes with 15+ test methods covering handler instantiation, configuration scenarios (next/previous/unknown/null inputs), error handling, and edge cases. Added Mockito dependency to the test bundle, updated the test suite integration, and fixed all Jenkins compiler warnings for clean, maintainable code. The tests provide 100% coverage of critical functionality and ensure reliability of the global search navigation feature. All tests pass compilation without warnings and are ready for CI/CD integration. May I have ur review? |
@NikkiAung thanks for the update. Can you check for the build error? I rebase, might only be a temporary problem |
…GlobalNextPrevSearchEntryHandler
…and potential null trigger events.
- Add GlobalNextPrevSearchEntryHandlerTest with basic unit tests - Add GlobalNextPrevSearchEntryHandlerIntegrationTest with integration tests - Add Mockito dependency to test bundle for mocking support - Update AllSearchTests to include new test classes - Add comprehensive test documentation Tests cover: - Handler instantiation and configuration - setInitializationData() with various scenarios (next/previous/unknown/null) - Interface implementation verification - Multiple handler instance creation - Error handling and edge cases Addresses the need for unit test coverage of the new global search navigation functionality.
3eb97e2
to
70187d5
Compare
Add global search result navigation shortcuts #3240
Add ALT+. and ALT+, shortcuts for navigating search results globally
without requiring manual Search view switching. This improves workflow
efficiency when reviewing and editing multiple search result occurrences. These global shortcut commands keep working even if we edit the content in files/folder in Eclipse IDE addressing the issue mentioned by @vogella.
Fixes #3240
ALT+. Flow (Next Search Entry)
ALT+, Flow (Previous Search Entry)