Wrap viewer in CheckboxTablePart within a FilteredTable #1666

ptziegler · 2025-03-07T21:39:56Z

By using the FilteredTable, we get a quick-filter for all extending parts for free. This includes:

The Cross-platform page of the "Deployable features" export wizard.
The Update Java Classpath page of the "Update classpath..." action
The Selection page of the "Import Plug-ins and Fragments" wizard
The Update Java Classpath page of the "Plug-in from Existing JAR Archives" wizard.
The Template Selection page of the "Plug-in Project" page.

Together with the quick-filter comes a caching of the checked table items. Those elements remain checked, even if currently hidden by the filter and will be restored once they become visible again.

Furthermore, the PluginListPage has been adapted to use a CheckboxTablePart rather than a CheckboxTreePart (effectively reverting 4c1edad), to match the list of plugins that it displays.

Contributes to #1497

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/CachedCheckboxTableViewer.java

ptziegler · 2025-03-07T21:42:50Z

ui/org.eclipse.pde.ui/.settings/.api_filters

+    <resource path="src/org/eclipse/pde/internal/ui/shared/CachedCheckboxTableViewer.java" type="org.eclipse.pde.internal.ui.shared.CachedCheckboxTableViewer">
+        <filter comment="Extends CheckboxTableViewer to match CachedCheckboxTreeviewer" id="571473929">
+            <message_arguments>
+                <message_argument value="CheckboxTableViewer"/>
+                <message_argument value="CachedCheckboxTableViewer"/>
+            </message_arguments>
+        </filter>
+    </resource>


I'm not sure whether inheritance is better than composition. But otherwise it's not possible to respond to e.g. calling setAllChecked() on the viewer.

As far as I know, one should in general prefer composition/references over inheritance. But first a solution has to work technically.

ptziegler · 2025-03-07T21:45:21Z

Furthermore, the PluginListPage has been adapted to use a CheckboxTablePart rather than a CheckboxTreePart (effectively reverting 4c1edad), to match the list of plugins that it displays.

Note that this breaks M2E, as it extends the internal BasePluginListPage.

ptziegler · 2025-03-07T21:47:40Z

And a side-by-side comparison of the Plugin table, which has been migrated from a tree- to a table viewer, as well as one of the tables for which the quick-filter is added due to the FilteredTable.

github-actions · 2025-03-07T22:12:39Z

Test Results

285 files ±0 285 suites ±0 48m 38s ⏱️ - 3m 6s
3 611 tests ±0 3 535 ✅ ±0 76 💤 ±0 0 ❌ ±0
11 025 runs ±0 10 794 ✅ ±0 231 💤 ±0 0 ❌ ±0

Results for commit 273296e. ± Comparison against base commit 5dc5f26.

♻️ This comment has been updated with latest results.

vogella · 2025-03-24T11:38:46Z

That looks very useful @ptziegler. Can you solve the conflicts? Would it make sense to implement the FilteredCheckBoxTable in platform.ui?

@HannesWell any concerns with the general approach?

ptziegler · 2025-03-24T12:03:48Z

Would it make sense to implement the FilteredCheckBoxTable in platform.ui?

Both classes would then be identical, with the only difference being that one uses a WorkbenchJob and the other a BasicUIJob. But that doesn't really matter for the way this is used here.

And given that you can use an e4 table with an e3 workbench, I think that's the better approach, rather than to duplicate the class.

Can you solve the conflicts?

I can have a look later today. I'll then also try to prepare a PR for M2E so its copy of the PluginListPage also uses the correct content provider.

ptziegler · 2025-03-24T18:52:31Z

Conflict has been resolved and M2E PR has been created: eclipse-m2e/m2e-core#1968

vogella · 2025-03-25T09:02:40Z

My concern is that older m2e will not work with new Eclipse versions and new m2e versions will not work with older Eclipse versions. Could this be handled in m2e but checking on a certain plug-in version or an instanceOf check?

HannesWell

any concerns with the general approach?

No objection from my side.

The Cross-platform page of the "Deployable features" export wizard.

Looking at the provided screenshot and thinking about that there are generally not that many supported archs, I would say that a search is not really necessary in this particular case.

After a first quick look I just have a few minor code-style remarks.

And maybe a bit off-topic, but do you think it would be simple to add a search for the dependency-tree shown in
DependenciesViewTreePage?

HannesWell · 2025-03-25T22:48:05Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/parts/CheckboxTablePart.java

+	private FilteredCheckboxTable filteredTable;
+


Is this field necessary? It looks like this could also just be a local variable in createStructuredViewer().

No, the viewer is only used inside the method and can be converted to a local variable. I've updated the PR.

HannesWell · 2025-03-25T22:57:52Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/CachedCheckboxTableViewer.java

+	 * @return checked leaf elements
+	 */
+	public Object[] getAllCheckedElements() {
+		return checkState.toArray(new Object[checkState.size()]);


Suggested change

return checkState.toArray(new Object[checkState.size()]);

return checkState.toArray(Object[]::new);

HannesWell · 2025-03-25T22:59:05Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/CachedCheckboxTableViewer.java

+		for (Object element : elements) {
+			checkState.add(element);
+		}


Suggested change

for (Object element : elements) {

checkState.add(element);

}

Collections.addAll(checkState, elements);

Done as well.

ptziegler · 2025-03-26T06:15:16Z

Looking at the provided screenshot and thinking about that there are generally not that many supported archs, I would say that a search is not really necessary in this particular case.

The filter is added automatically by virtue of extending the WizardCheckboxTablePart. It could probably be configured to exclude the filter for this specific case, but I believe that this would just cause everything to be less readable.

And maybe a bit off-topic, but do you think it would be simple to add a search for the dependency-tree shown in
DependenciesViewTreePage?

That's trivial. But this should then probably also be applied to the DependenciesViewListPage.

HannesWell · 2025-03-26T21:22:35Z

Looking at the provided screenshot and thinking about that there are generally not that many supported archs, I would say that a search is not really necessary in this particular case.

The filter is added automatically by virtue of extending the WizardCheckboxTablePart. It could probably be configured to exclude the filter for this specific case, but I believe that this would just cause everything to be less readable.

Acknowledged. It also doesn't harm to have it. So keeping it simpler is fine.

And maybe a bit off-topic, but do you think it would be simple to add a search for the dependency-tree shown in
DependenciesViewTreePage?

That's trivial. But this should then probably also be applied to the DependenciesViewListPage.

Awesome! Yes adding it there would also make sense.
I asked for this, because it's currently cumbersome to check if a Plug-in project depends on a specific bundle/plugin if the list/tree is large. And in the tree mode a search should make it very simple to identify the chain of dependencies that leads to a specify transitive dependency of a Plugin.

Do you think it would make sense to add this in this PR as well or would you prefer a separate one?

ptziegler · 2025-04-23T19:13:09Z

Do you think it would make sense to add this in this PR as well or would you prefer a separate one?

Ye ask and ye shall receive: #1737
Adding a quick-filter to the dependency view is unrelated to a cached checkbox-table, so it only makes sense to handle that separately.

ptziegler · 2025-04-23T19:13:40Z

I've also removed the changes to the plugin table so that it doesn't cause issues with M2E.

By using the FilteredTable, we get a quick-filter for all extending parts for free. This includes: - The Cross-platform page of the "Deployable features" export wizard. - The Update Java Classpath page of the "Update classpath..." action - The Template Selection page of the "Plug-in Project" page. Together with the quick-filter comes a caching of the checked table items. Those elements remain checked, even if currently hidden by the filter and will be restored once they become visible again.

vogella · 2025-05-12T13:21:14Z

@ptziegler is this one ready from your side? I would like to merge it. Would be nice to these filters.

ptziegler · 2025-05-12T19:52:54Z

@vogella This one should be good to go.

vogella · 2025-05-12T20:13:53Z

Thanks @ptziegler

ptziegler commented Mar 7, 2025

View reviewed changes

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/CachedCheckboxTableViewer.java Show resolved Hide resolved

ptziegler commented Mar 7, 2025

View reviewed changes

ptziegler force-pushed the use-filtered-table branch from 12ec3ef to 54ce71c Compare March 7, 2025 22:23

ptziegler force-pushed the use-filtered-table branch 2 times, most recently from d935870 to ff41115 Compare March 24, 2025 18:26

ptziegler mentioned this pull request Mar 24, 2025

Adapt PluginListPage to changes in PDE eclipse-m2e/m2e-core#1968

Closed

HannesWell reviewed Mar 25, 2025

View reviewed changes

ptziegler force-pushed the use-filtered-table branch from ff41115 to 94203e8 Compare April 23, 2025 19:11

HannesWell force-pushed the use-filtered-table branch from 94203e8 to ae57261 Compare April 25, 2025 04:51

vogella force-pushed the use-filtered-table branch from ae57261 to 273296e Compare May 12, 2025 11:26

vogella merged commit 7c4e17e into eclipse-pde:master May 12, 2025
19 checks passed

	return checkState.toArray(new Object[checkState.size()]);
	return checkState.toArray(Object[]::new);

Wrap viewer in CheckboxTablePart within a FilteredTable #1666

Wrap viewer in CheckboxTablePart within a FilteredTable #1666

Uh oh!

Conversation

ptziegler commented Mar 7, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptziegler commented Mar 7, 2025

Uh oh!

ptziegler commented Mar 7, 2025

Uh oh!

github-actions bot commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

vogella commented Mar 24, 2025

Uh oh!

ptziegler commented Mar 24, 2025

Uh oh!

ptziegler commented Mar 24, 2025

Uh oh!

vogella commented Mar 25, 2025

Uh oh!

HannesWell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptziegler commented Mar 26, 2025

Uh oh!

HannesWell commented Mar 26, 2025

Uh oh!

ptziegler commented Apr 23, 2025

Uh oh!

ptziegler commented Apr 23, 2025

Uh oh!

vogella commented May 12, 2025

Uh oh!

ptziegler commented May 12, 2025

Uh oh!

Uh oh!

vogella commented May 12, 2025

Uh oh!

Uh oh!

github-actions bot commented Mar 7, 2025 •

edited

Loading