Filtered tree is implemented for Marker Support Views. - #4208
Conversation
|
@iloveeclipse Initial implementation of filtered tree.. Please check if it looks good. |
There was a problem hiding this comment.
Pull request overview
This PR adds a search-based filtering UI (a FilteredTree search box) to Marker Support Views (e.g., Bookmarks/Problems/Tasks) by wrapping the existing markers TreeViewer in a filtered tree and providing a marker-specific PatternFilter implementation.
Changes:
- Introduces a
MarkersFilteredTreethat hosts the existingMarkersTreeViewerwhile adding a filter text box. - Adds
MarkerPatternFilterto match markers against the values of all currently visible marker fields/columns. - Externalizes the filter box initial text and adds the required bundle dependency for
org.eclipse.e4.ui.dialogs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| bundles/org.eclipse.ui.ide/src/org/eclipse/ui/views/markers/internal/messages.properties | Adds an externalized string for the filter box initial text. |
| bundles/org.eclipse.ui.ide/src/org/eclipse/ui/views/markers/internal/MarkerMessages.java | Adds the corresponding NLS field for the new message key. |
| bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers/MarkersFilteredTree.java | New FilteredTree subclass that creates the markers-specific TreeViewer. |
| bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers/MarkerPatternFilter.java | New PatternFilter that matches concrete markers based on visible column values. |
| bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers/ExtendedMarkersView.java | Replaces direct Tree creation with the new filtered tree and wires the initial text. |
| bundles/org.eclipse.ui.ide/META-INF/MANIFEST.MF | Adds org.eclipse.e4.ui.dialogs to Require-Bundle for FilteredTree/PatternFilter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Is it possible to hide the filter line (like in the Error Log view)? |
I will implement a preference for this like Error Log view. Which can be turned ON and OFF. |
16d6b3c to
aa5e173
Compare
|
Functionality-wise looks good. What I miss (as end user):
and after 
|
|
Looks like filtering happens after the marker limit. That would result in strange experience for the user. Please check if that is the case and fix or document it if that is the case. |
Could you elaborate a bit, maybe with a screenshot of "strange" state? |
|
Just guessing, but if you limit to some small number, say 10, and the filter applies only to those 10 but not the actual 1000 in total, for example, then it would be pretty confusing and misleading. |
|
Could it be we are not on same page? The text filter is supposed to be applied on shown items after all possible filters are set, not to override whatever filters are set. It is not persisted and is supposed to be a quick and easy way to find something in a view. See Errors view or Preferences page. |
That is a good use case. Also the view has IIRC some automatic grouping (was it 1000?) and loads only the the next list on request. So if I have 2000 warnings grouped and filter for "my error" I would expect to see all not not only the one in the first group. I did not test it, no time for testing this change, just describing how I would expect it to work. |
|
OK, thanks for clarification. Let summarize here the main use case here & my understanding. The problem with Problems/Tasks/Bookmarks view is that the filtering rules are not easy to change and need lot of clicks. So if user has some focused task, sees 100 problems on a project and "just" want filter by the package or file name or problem description, of course he could go to the "Filters" page, create new rule, define what should be shown/selected/which scope etc, and after ~20 clicks user will either give up or create a rule which he would need to disable few seconds later because he would want to restore "default" problems set visible for the project. With the text filter, user can just type first letter of the package or file name or problem description and after typing 1-3 characters the task will be done. To see all problems again, "clear" the filter line - one click. Now of course there could be 10.000 problems in the workspace and we only show some subset (for selected project), let say 100 problems in the tree. I would not expect that we now unset all the complicated filters user might have set in the view and start filtering only by the text. Suddently all 10.000 may be shown if text filter matches! However, we also have two existing item limits in marker views which apply after all the filtering above is done.
Similar like with the "complex" filter configurations above, I believe both limits must also remain, but the "quick" filter should be applied before these limits. So the summary is:
|
29acf10 to
dbae51b
Compare
Latest commit has this fix. |
dbae51b to
ddaa061
Compare
This is also fixed now. |
@raghucssit : Please check this, and please rebase on master before you push next time. |
ddaa061 to
51d9799
Compare
Bookmarks View, Problems View and Tasks Views will have a search filter box. see eclipse-platform#4204
51d9799 to
6f642f8
Compare
Latest push has this fix. Now quick search filter searches the items within the limit of viewer filter and also workbench viewer limit. |
|
All the failed tests are passing in my local RHEL machine.. I don't understand what is the problem. I run them as Junit Plugin Test. |
|
Going to the workspace logs from the test, I see in the workspace log following error: Shortly after that tests starts to fail. There are also follow up errors like I assume code changes in this PR cause exceptions in unexpected places, and sooner or later that prevents different UI code to work properly. => this is the problem with the patch. |
|
This could be the fix, let see: e574b98 |
|
First impression after quick test: it works mostly as expected, but see this picture, where overall numbers in the view description are OK, but category numbers are inconsistent: 
I will proceed with code review. |
Yeah. Category text is untouched after filter.. I will fix it.. I think i have to fix Label Provider.. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (6)
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers/ExtendedMarkersView.java:202
- The comment for showFilterTextSetByUser has multiple grammatical errors and refers to a "toolbar filter", but the toggle is contributed to the view menu (see createShowFilterTextAction). This makes the documentation misleading.
/**
* True if user has ever invoked the toolbar filter to show/hide the search
* filter text box Other wise false. I false case preference value is used to
* show/hide the filter text.
*/
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers/ExtendedMarkersView.java:167
- Typo in Javadoc: "Momento" should be "Memento" (and the sentence should describe that this key is stored in the view memento/state).
This issue also appears on line 198 of the same file.
/**
* Momento key to store show/hide search text filter box in the view's header.
*/
private static final String TAG_SHOW_FILTER_TEXT = "showFilterText"; //$NON-NLS-1$
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers/MarkerPatternFilter.java:61
- The Javadoc for isBeyondWorkbenchViewerLimit(...) has an unmatched closing tag, which can cause Javadoc warnings/errors (and the build is configured to fail on Javadoc errors).
/**
* Check if the item is beyond the General preference {@code LARGE_VIEW_LIMIT}.
* </p>
*/
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers/ExtendedMarkersView.java:1582
- The Javadoc for addShowFilterTextPreferenceListener() is grammatically incorrect ("user don't") and unclear about when the preference is applied.
* Listen to
* {@link IDEInternalPreferences#INITIALLY_SHOW_FILTER_TEXT_IN_MARKER_VIEWS}
* preference changes and apply onto search filter box only if user don't have
* local settings.
*/
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/IDEInternalPreferences.java:186
- Minor grammar in the new preference Javadoc: "customize" should be "customizes", and the wording should avoid referring to a specific UI location like "toolbar filters" (the toggle is in the view menu).
/**
* Whether the filter search box in marker views (Bookmarks, Tasks and Problems)
* should be shown or not initially. If the user customize the behavior in views
* toolbar filters then this preference has no effect.
*/
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/messages.properties:505
- The tooltip mentions "Toolbar filter", but the code contributes the toggle to the view menu (ExtendedMarkersView#createShowFilterTextAction). The tooltip should reflect where the user actually changes this setting.
WorkbenchPreference_showFilterTextInMarkerViews = &Initially show text filter in marker views
WorkbenchPreference_showFilterTextInMarkerViewsToolTip = This preference is used to show/hide the text filter in the Problems, Tasks, Bookmarks views. This preference has effect only if the user has not explicitly set the visibility of the text filter in the view's Toolbar filter.
In Problems View categories like Warnings and Error has the count of number of items under it. We need to update the count after filter.
For consistency we are moving initial quick filter show/hide preference from org.eclipse.ui.ide to org.eclipse.ui.workbench bundle
|
@iloveeclipse All the review comments are fixed. And the Category text now gets the updated filter count. |
|
@raghucssit : there are 5 commits. Can you please squash them all to one & rebase on latest master state? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (7)
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers/ExtendedMarkersView.java:203
- This Javadoc has multiple typos/grammar issues ("Other wise", "I false") and refers to a "toolbar filter" even though the toggle is added to the view menu (see createShowFilterTextAction). Clarify the wording to avoid misleading future maintainers.
/**
* True if user has ever invoked the toolbar filter to show/hide the search
* filter text box Other wise false. I false case preference value is used to
* show/hide the filter text.
*/
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers/ExtendedMarkersView.java:1561
- showSearchFilterText() dereferences filteredTree without null/disposed checks. Since the preference listener is registered in init() (before createPartControl/createViewer), a preference change delivered during view initialization can call this method while filteredTree is still null, causing an NPE. Guard against null/disposed and just persist the boolean until the controls exist.
private void showSearchFilterText(boolean visible) {
showFilterText = visible;
if (showFilterTextAction != null && showFilterTextAction.isChecked() != visible) {
showFilterTextAction.setChecked(visible);
}
filteredTree.setFilterTextVisible(visible);
// if the filter text is visible, set focus to it, otherwise set focus to the
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers/MarkersFilteredTree.java:80
- JobChangeListener.done(...) calls getDisplay() on the FilteredTree without guarding against widget disposal. If the view/tree gets disposed while the refresh job is still finishing, this can throw and potentially log UI-thread exceptions. Guard with isDisposed() (or use a stable Display reference) before calling getDisplay()/asyncExec.
public void done(IJobChangeEvent event) {
Display display = getDisplay();
if (display != null && !display.isDisposed()) {
display.asyncExec(() -> {
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers/MarkerPatternFilter.java:61
- The Javadoc for isBeyondWorkbenchViewerLimit has an unmatched closing tag, which can produce malformed Javadoc (and fail builds if Javadoc warnings are treated as errors).
/**
* Check if the item is beyond the General preference {@code LARGE_VIEW_LIMIT}.
* </p>
*/
bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/IWorkbenchPreferenceConstants.java:724
- The Javadoc says the search box visibility is set via the view's "Toolbar filter", but this PR introduces the toggle as a view menu action ("Show text filter"). Updating this wording will keep the API documentation consistent with the UI.
* <p>
* This preference is a <code>boolean</code> value. This preference has an
* effect only if the user has not explicitly set the visibility of the
* search box in the view's Toolbar filter.
* </p>
bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/messages.properties:530
- The tooltip text mentions the view's "Toolbar filter", but the toggle introduced for marker views is a view menu action ("Show text filter"). Updating this string avoids confusing users and keeps it aligned with the actual UI.
WorkbenchPreference_showFilterTextInMarkerViews = &Initially show text filter in marker views
WorkbenchPreference_showFilterTextInMarkerViewsToolTip = This preference is used to show/hide the text filter in the Problems, Tasks, Bookmarks views. This preference has effect only if the user has not explicitly set the visibility of the text filter in the view's Toolbar filter.
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers/ExtendedMarkersView.java:167
- Typo in Javadoc: "Momento" should be "Memento".
This issue also appears in the following locations of the same file:
- line 199
- line 1555
/**
* Momento key to store show/hide search text filter box in the view's header.
*/
|
@raghucssit : also please check Copilot comments. There are few smaller issues with new code. |
There were some minor grammatical errors were identified by copilot and has been fixed.
All copilot suggested java doc grammar has been fixed. |
Bookmarks View, Problems View and Tasks Views will have a search filter box.
see #4204