Implement filter-based table viewer via the new FilterTable class #2567

ptziegler · 2024-12-02T18:08:53Z

This moves the viewer-agnostic components of the FilterTree widget into
an AbstractFilteredStructuredViewer base class, which is then used to
implement the FilterTable widget.

The base class has been moved to org.eclipse.jface.text together with
the TextMatcher, to allow it to be used together with both an E3 and E4
workbench.

For the AbstractFilteredStructuredViewer, following methods have been
added to support this abstraction:

isShowFilterControls()
isQuickSelectionMode()
init(int)

For the FilteredTree, following fields and methods have been marked as
for-removal:

filterToolBar
clearButtonControl
updateToolbar(boolean)

To avoid code-duplication as a result of bug 260664, the code of
clearText() has been moved to a separate doClearText() method, so that
the same code can be invoked inside the listeners, without having to
worry about clearText() being overridden by subclasses.

This change adds a dependency from org.eclipse.jface.text to
org.eclipse.ui.workbench.

ptziegler · 2024-12-02T18:09:37Z

See eclipse-pde/eclipse.pde#1497 for a potential use-case.

Note: This also works with virtual tables:

recording.webm

vogella · 2024-12-02T18:11:04Z

Did you also consider the FilteredTree from the E4 package? This way this class could also be used in pure e4 rcp apps.

ptziegler · 2024-12-02T18:21:35Z

Did you also consider the FilteredTree from the E4 package? This way this class could also be used in pure e4 rcp apps.

Not for the time being. And looking at it, it really is just a copy-paste of the E3 class. So ideally, I'd like to get some feedback on the current implementation before I repeat the same for the E4 one.

vogella · 2024-12-02T18:36:57Z

IIRC we copied it and removed the E3 reference (IIRC the help system) and added the option to enable/ disable the filter at runtime.

If it's functionality is the same I think we should only add one class ( to the E4 package so that it can be used in ide and E4 RCP).

github-actions · 2024-12-02T19:08:08Z

Test Results

1 214 files - 607 1 214 suites - 607 1h 17m 26s ⏱️ - 33m 22s
7 727 tests + 1 7 494 ✅ - 3 233 💤 + 5 0 ❌ - 1
16 228 runs - 8 111 15 714 ✅ - 7 877 514 💤 - 233 0 ❌ - 1

Results for commit f36df7f. ± Comparison against base commit 98ff2aa.

This pull request removes 6 and adds 7 tests. Note that renamed tests count towards both.

UiTestSuite org.eclipse.ui.tests.filteredtree.TextMatcherTest ‑ testCaseInsensitivity
UiTestSuite org.eclipse.ui.tests.filteredtree.TextMatcherTest ‑ testChinese
UiTestSuite org.eclipse.ui.tests.filteredtree.TextMatcherTest ‑ testEmpty
UiTestSuite org.eclipse.ui.tests.filteredtree.TextMatcherTest ‑ testMultipleWords
UiTestSuite org.eclipse.ui.tests.filteredtree.TextMatcherTest ‑ testSingleWords
UiTestSuite org.eclipse.ui.tests.filteredtree.TextMatcherTest ‑ testSuffixes

UiTestSuite org.eclipse.ui.tests.filteredtree.FilteredTableTests ‑ testAddAndRemovePattern
org.eclipse.jface.text.tests.TextMatcherTest ‑ testCaseInsensitivity
org.eclipse.jface.text.tests.TextMatcherTest ‑ testChinese
org.eclipse.jface.text.tests.TextMatcherTest ‑ testEmpty
org.eclipse.jface.text.tests.TextMatcherTest ‑ testMultipleWords
org.eclipse.jface.text.tests.TextMatcherTest ‑ testSingleWords
org.eclipse.jface.text.tests.TextMatcherTest ‑ testSuffixes

This pull request skips 5 tests.

org.eclipse.ui.genericeditor.tests.HoverTest ‑ testEnabledWhenHover
org.eclipse.ui.genericeditor.tests.HoverTest ‑ testMultipleHover
org.eclipse.ui.genericeditor.tests.HoverTest ‑ testProblemHover
org.eclipse.ui.genericeditor.tests.HoverTest ‑ testSingleHover
org.eclipse.ui.genericeditor.tests.ShowInformationTest ‑ testInformationControl

♻️ This comment has been updated with latest results.

ptziegler · 2024-12-02T20:09:28Z

Is there an E4 counterpart for the TextMatcher? Would it perhaps make sense to move it to e.g. org.eclipse.text?

vogella · 2024-12-03T15:34:12Z

Is there an E4 counterpart for the TextMatcher? Would it perhaps make sense to move it to e.g. org.eclipse.text?

I'm not aware of an e4 counterpart. Would be great to move this to an API package that everyone can use.

laeubi · 2024-12-03T15:37:28Z

Is there an E4 counterpart for the TextMatcher? Would it perhaps make sense to move it to e.g. org.eclipse.text?

I'm not aware of an e4 counterpart. Would be great to move this to an API package that everyone can use.

By the way it is confusing to talk about E3/E4 "counterparts" here as these are completely independent from the used technique (e.g. it does not require any DI at all nor does it requires E3 extension points). So making the class somewhere more generic available would be beneficial and as it is a final class let the "old" extend the "new" should work flawlessly.

laeubi · 2024-12-03T15:40:51Z

bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/dialogs/FilteredTable.java

+ *
+ * @since 3.135
+ */
+public non-sealed class FilteredTable extends AbstractFilteredViewer {


Just an idea but maybe having a JFace FilteredTableViewer could be a better place for this so it can be used even outside of Eclipse Platform in SWT/JFace Applications.

Maybe even one can abstract the whole thing to use a StructuredViewer then it can be used with any table/tree/grid ...

Just an idea but maybe having a JFace FilteredTableViewer could be a better place for this so it can be used even outside of Eclipse Platform in SWT/JFace Applications.

The FilteredTable/FilteredTree depend on the WorkbenchJob/BasicUIJob, so there is an inherent dependency to the E3/E4 workbench, which effectively prevents them to be moved to JFace.

Though it might be possible to specify a plain Job in the abstract base class and then have the implementations provide their respective instances... This would at the very least avoid the code duplication.

Maybe even one can abstract the whole thing to use a StructuredViewer then it can be used with any table/tree/grid ...

The base class only specifies a ColumnViewer but one could probably go even lower...

vogella · 2024-12-03T15:46:22Z

By the way it is confusing to talk about E3/E4 "counterparts"

It is not, new elements which can be used in E3 and pure E4 must of have dependencies to the old workbench implementation, as the e3 initialization code does not run in pure E4.

laeubi · 2024-12-03T15:48:32Z

It is not, new elements which can be used in E3 and pure E4 must of have dependencies to the old workbench implementation, as the e3 initialization code does not run in pure E4.

Them mentioned class do not have any dependencies to a (E3 or E4) workbench as far as I can see...

This moves the viewer-agnostic components of the FilterTree widget into an AbstractFilteredStructuredViewer base class, which is then used to implement the FilterTable widget. The base class has been moved to org.eclipse.jface.text together with the TextMatcher, to allow it to be used together with both an E3 and E4 workbench. For the AbstractFilteredStructuredViewer, following methods have been added to support this abstraction: - isShowFilterControls() - isQuickSelectionMode() - init(int) For the FilteredTree, following fields and methods have been marked as for-removal: - filterToolBar - clearButtonControl - updateToolbar(boolean) To avoid code-duplication as a result of bug 260664, the code of clearText() has been moved to a separate doClearText() method, so that the same code can be invoked inside the listeners, without having to worry about clearText() being overridden by subclasses. This change adds a dependency from org.eclipse.jface.text to org.eclipse.ui.workbench.

ptziegler · 2024-12-03T20:48:40Z

In principle, the TextMatcher class should work regardless of whether an E3 or E4 workbench is used. I've therefore moved the class from org.eclipse.ui.workbench to org.eclipse.jface.text. The same is true for the abstract base class I've created for the FilteredTable/Tree, so that I don't have to duplicate the implementation for the E4 classes.

Though I still have to think about how to best adapt the E4 classes...

eclipse-platform-bot · 2024-12-03T20:49:40Z

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

tests/org.eclipse.jface.text.tests/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From 429f39eca4fa832b8e6f6fda15de9a20e1c14c81 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Tue, 3 Dec 2024 20:49:18 +0000
Subject: [PATCH] Version bump(s) for 4.35 stream


diff --git a/tests/org.eclipse.jface.text.tests/META-INF/MANIFEST.MF b/tests/org.eclipse.jface.text.tests/META-INF/MANIFEST.MF
index 510688556a..e0b12814d6 100644
--- a/tests/org.eclipse.jface.text.tests/META-INF/MANIFEST.MF
+++ b/tests/org.eclipse.jface.text.tests/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %Plugin.name
 Bundle-SymbolicName: org.eclipse.jface.text.tests
-Bundle-Version: 3.13.700.qualifier
+Bundle-Version: 3.13.800.qualifier
 Bundle-Vendor: %Plugin.providerName
 Bundle-Localization: plugin
 Export-Package: 
-- 
2.47.0

Further information are available in Common Build Issues - Missing version increments.

laeubi · 2024-12-04T07:40:37Z

Is there an E4 counterpart for the TextMatcher? Would it perhaps make sense to move it to e.g. org.eclipse.text?

Interestingly enough I found we already have org.eclipse.core.text.StringMatcher so maybe that's another place to put a generic TextMatcher ...

ptziegler marked this pull request as draft December 2, 2024 20:39

laeubi reviewed Dec 3, 2024

View reviewed changes

ptziegler force-pushed the filtered-table branch from 5ccbcda to 763212e Compare December 3, 2024 20:44

Version bump(s) for 4.35 stream

f36df7f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement filter-based table viewer via the new FilterTable class #2567

Implement filter-based table viewer via the new FilterTable class #2567

ptziegler commented Dec 2, 2024 •

edited

Loading

ptziegler commented Dec 2, 2024 •

edited

Loading

vogella commented Dec 2, 2024

ptziegler commented Dec 2, 2024

vogella commented Dec 2, 2024

github-actions bot commented Dec 2, 2024 •

edited

Loading

ptziegler commented Dec 2, 2024

vogella commented Dec 3, 2024

laeubi commented Dec 3, 2024

laeubi Dec 3, 2024 •

edited

Loading

ptziegler Dec 3, 2024

vogella commented Dec 3, 2024

laeubi commented Dec 3, 2024

ptziegler commented Dec 3, 2024

eclipse-platform-bot commented Dec 3, 2024

laeubi commented Dec 4, 2024

Implement filter-based table viewer via the new FilterTable class #2567

Are you sure you want to change the base?

Implement filter-based table viewer via the new FilterTable class #2567

Conversation

ptziegler commented Dec 2, 2024 • edited Loading

ptziegler commented Dec 2, 2024 • edited Loading

vogella commented Dec 2, 2024

ptziegler commented Dec 2, 2024

vogella commented Dec 2, 2024

github-actions bot commented Dec 2, 2024 • edited Loading

Test Results

ptziegler commented Dec 2, 2024

vogella commented Dec 3, 2024

laeubi commented Dec 3, 2024

laeubi Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

ptziegler Dec 3, 2024

Choose a reason for hiding this comment

vogella commented Dec 3, 2024

laeubi commented Dec 3, 2024

ptziegler commented Dec 3, 2024

eclipse-platform-bot commented Dec 3, 2024

laeubi commented Dec 4, 2024

ptziegler commented Dec 2, 2024 •

edited

Loading

ptziegler commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 2, 2024 •

edited

Loading

laeubi Dec 3, 2024 •

edited

Loading