-
Notifications
You must be signed in to change notification settings - Fork 201
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
Improve memory search results page #516
Conversation
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.
This looks reasonable and I will run it up in my IDE and provide any further feedback.
I can squash this PR when I merge it, or if you make further changes you can feel free to squash it so the PR has just one commit.
memory/org.eclipse.cdt.debug.ui.memory.search/META-INF/MANIFEST.MF
Outdated
Show resolved
Hide resolved
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.
Thanks for the PR @LizzMre - I am reviewing an earlier version as I am just about to hit Submit review I see there are new changes - I'll comment on them in a moment.
Overall this looks great, mostly style changes.
There is one blocking problem though. By always activating the memory view on selection change the search view cannot be navigated well with a keyboard. Imagine you have the search view and memory view open in two different tab groups, you cannot press up/down arrow to scroll through all your matches anymore. Can you check if the memory view and the search view are in different tab groups, or perhaps check if the memory view is already visible.
...bug.ui.memory.search/src/org/eclipse/cdt/debug/ui/memory/search/MemorySearchResultsPage.java
Show resolved
Hide resolved
...bug.ui.memory.search/src/org/eclipse/cdt/debug/ui/memory/search/MemorySearchResultsPage.java
Outdated
Show resolved
Hide resolved
...bug.ui.memory.search/src/org/eclipse/cdt/debug/ui/memory/search/MemorySearchResultsPage.java
Outdated
Show resolved
Hide resolved
...bug.ui.memory.search/src/org/eclipse/cdt/debug/ui/memory/search/MemorySearchResultsPage.java
Outdated
Show resolved
Hide resolved
...bug.ui.memory.search/src/org/eclipse/cdt/debug/ui/memory/search/MemorySearchResultsPage.java
Outdated
Show resolved
Hide resolved
...bug.ui.memory.search/src/org/eclipse/cdt/debug/ui/memory/search/MemorySearchResultsPage.java
Outdated
Show resolved
Hide resolved
The failed test is a known flaky test tracked here - #194 |
Thank you for your review @jonahgraham! I will work on the requested changes. |
...bug.ui.memory.search/src/org/eclipse/cdt/debug/ui/memory/search/MemorySearchResultsPage.java
Show resolved
Hide resolved
...bug.ui.memory.search/src/org/eclipse/cdt/debug/ui/memory/search/MemorySearchResultsPage.java
Show resolved
Hide resolved
The Memory Browser and Memory views will jump to the memory address after a user selects a value from the Memory Search Result list. If the views belong to the same tab group as the Search view, then it might not be very intuitive for the user to switch back to one of the Memory views and see the effect of the selection. The current commit adds on the the double click listener (from MemorySearchResultsPage.java) the behavior to refocus on the Memory Browser/ Memory view. Resolves: eclipse-cdt#515
- Use modern java instanceof - Log unexpected exceptions - reorder tests - add missing null checks - invert some if conditions for less nesting
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.
@LizzMre LGTM - thanks for this improvement.
I split the code cleanup into a separate commit than the new functionality and cleaned up a couple of other things in the class.
Can you have a quick look and see if you are ok with this and then I can merge once CI reports.
@jonahgraham Thank you again for all you support and very good suggestions! I didn't get to mention that in the end I chose the double click event as it had more sense and would not interfere with the simple selection behavior as you pointed out in one of your earlier comments. I am ok with the final version. |
My pleasure. Once the CI finishes I will merge this.
As far as I was concerned you did mention it because it was in your commit message! It is a good solution and I appreciate you making the search result page work better. |
The Memory Browser and Memory views will jump to the memory address after a user selects a value from the Memory Search Result list. If the views belong to the same tab group as the Search view, then it might not be very intuitive for the user to switch back to one of the Memory views and see the effect of the selection.
The current commit adds on the the double click listener(from MemorySearchResultsPage.java) the behavior to refocus on the Memory Browser/ Memory view.
Resolves: #515