UI support for setting filter attribute in feature editor #1942

gian-benito · 2025-08-22T03:53:32Z

Summary

Continuation of closed PR #1931

Issue
Add support for setting the filter attribute in the feature editor under the dependencies tab #1808,

Solution

Added a filter textfield
Textfield contents are validated by parsing as an OSGi filter
Textfield contents are also verified to have valid syntax
User input is processed by setFilter into an XML write function, which directly imports it into the corresponding XML file.
tested for multiple features and existing Eclipse PDE features

@KN0987 @BAlgarra

laeubi · 2025-09-04T05:36:53Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginImport.java


+	@Override
+	public String getFilter() {
+		// TODO Auto-generated method stub


TODO should be rempved

laeubi · 2025-09-04T05:37:02Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginImport.java

+	@Override
+	public String getFilter() {
+		// TODO Auto-generated method stub
+		return version;


Suggested change

return version;

return filter;

laeubi · 2025-09-04T05:37:26Z

apitools/org.eclipse.pde.api.tools/.classpath

 	<classpathentry kind="src" path="xslt"/>
 	<classpathentry kind="output" path="bin"/>
-</classpath>
+</classpath>


please revert whit-space only changes

laeubi · 2025-09-04T05:38:19Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/ui/templates/PluginReference.java

 	}
-
-}
+}


Do not include unrelated changes

laeubi · 2025-09-04T05:39:59Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/MatchSection.java

+				((IFeatureImport) fCurrentImport).setFilter(filter);
+			}
+		} catch (InvalidSyntaxException e) {
+			PDEPlugin.logException(e);


this should not be logged but create an error marker, so this likely is better done in the Feature Validation process and when users are typing in a filter than during storing of the filter.

@laeubi

I referenced how Platform Filters were validated in the main page of the Plug-In Project and implemented a script based on GeneralInfoSection.java. Would this be a valid fix?

If there is already a utility code fr this it is of course perfectly valid to reuse that.

gian-benito · 2025-09-06T00:25:08Z

Thank you for the feedback @laeubi. I'll address those changes and update the pull request.

Co-authored-by: Brandon Algarra <[email protected]> Co-authored-by: Khang Nguyen <[email protected]>

gian-benito · 2025-10-31T03:13:21Z

Hi @HannesWell, just wanted to follow up on this pull request. We have addressed the feedback given by @laeubi and would like to know if there is anything else needed before it gets merged.

Thank you for your time!

Copilot

Pull Request Overview

This PR adds filter support to plugin and feature imports in PDE. The changes introduce a new "filter" field to the MatchSection UI component, allowing users to specify OSGi filter expressions for conditional dependencies.

Key changes:

Added filter field UI components to MatchSection with validation
Extended IPluginImport API with getFilter() and setFilter() methods
Implemented filter property storage in PluginImport and PluginImportNode classes

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
pderesources.properties	Added UI labels for filter field and error message
PDEUIMessages.java	Added constants for filter-related messages
ControlValidationUtility.java	Added validateFilterField() method for filter validation
MatchSection.java	Added filter field UI, validation, and handlers to the match section
IPluginImport.java	Added P_FILTER constant and filter getter/setter methods to the API
PluginImport.java	Implemented filter property storage and change notification
PluginImportNode.java	Implemented filter getter/setter for XML-based plugin imports

Comments suppressed due to low confidence (3)

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginImport.java:240

The restoreProperty method is missing a case for P_FILTER. When the filter property is changed and needs to be restored (e.g., during undo/redo operations), the method won't handle it correctly. Add a case to check for P_FILTER and call setFilter() accordingly.

	public void restoreProperty(String name, Object oldValue, Object newValue) throws CoreException {
		if (name.equals(P_MATCH)) {
			setMatch(((Integer) newValue).intValue());
			return;
		}
		if (name.equals(P_REEXPORTED)) {
			setReexported(((Boolean) newValue).booleanValue());
			return;
		}
		if (name.equals(P_OPTIONAL)) {
			setOptional(((Boolean) newValue).booleanValue());
			return;
		}
		if (name.equals(P_VERSION)) {
			setVersion(newValue != null ? newValue.toString() : null);
			return;
		}
		super.restoreProperty(name, oldValue, newValue);
	}

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/MatchSection.java:97

The cancelEdit method only cancels edits for fVersionText but not for fFilterText. This could leave uncommitted changes in the filter field when the user cancels editing. Add fFilterText.cancelEdit() to ensure consistent behavior.

	public void cancelEdit() {
		fVersionText.cancelEdit();
		super.cancelEdit();
	}

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/MatchSection.java:91

The commit method commits version text but doesn't commit filter text changes. This means filter field changes won't be persisted when the form is saved. Add fFilterText.commit() to ensure filter changes are saved properly.

	public void commit(boolean onSave) {
		if (isDirty() == false) {
			return;
		}
		if (fCurrentImport != null && fVersionText.getText().isEnabled()) {
			fVersionText.commit();
			String value = fVersionText.getValue();
			int match = IMatchRules.NONE;
			if (value != null && value.length() > 0) {
				applyVersion(value);
				match = getMatch();
			}
			applyMatch(match);
		}
		super.commit(onSave);
	}

Copilot · 2025-10-31T04:09:41Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/MatchSection.java

+	private void applyFilter(String filter) {
+		try {
+			if (fCurrentImport == null)
+				return;
+			if (filter == null || filter.isEmpty())
+				((IFeatureImport) fCurrentImport).setFilter(null);
+			else {
+				FrameworkUtil.createFilter(filter);
+				((IFeatureImport) fCurrentImport).setFilter(filter);
+			}
+		} catch (InvalidSyntaxException e) {
+		} catch (CoreException e) {
+			PDEPlugin.logException(e);
+		}
+	}


The method unconditionally casts fCurrentImport to IFeatureImport, but MatchSection is also used for plugin imports (IPluginImport) in DependenciesPage. This will cause a ClassCastException when used in the plugin context. The code should check the type and cast appropriately to either IPluginImport or IFeatureImport, both of which support the filter property.

Copilot · 2025-10-31T04:09:41Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/MatchSection.java

+
 		resetMatchCombo(fCurrentImport);
+		fFilterText.setEditable(isEditable());
+		fFilterText.setValue(((IFeatureImport) fCurrentImport).getFilter());


The unconditional cast to IFeatureImport will cause a ClassCastException when MatchSection is used in the plugin dependencies context where fCurrentImport is an IPluginImport. Check the instance type and cast appropriately, as both IPluginImport and IFeatureImport support the filter property.

Suggested change

fFilterText.setValue(((IFeatureImport) fCurrentImport).getFilter());

fFilterText.setValue(fCurrentImport.getFilter());

Copilot · 2025-10-31T04:09:41Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/MatchSection.java

+				FrameworkUtil.createFilter(filter);
+				((IFeatureImport) fCurrentImport).setFilter(filter);
+			}
+		} catch (InvalidSyntaxException e) {


The InvalidSyntaxException is silently swallowed without logging or user feedback. While validation occurs in validateFilter(), the exception here indicates the filter was accepted by the validator but failed when creating the actual filter. This scenario should be logged for debugging purposes, similar to how CoreException is handled.

Suggested change

} catch (InvalidSyntaxException e) {

} catch (InvalidSyntaxException e) {

PDEPlugin.logException(e);

Copilot · 2025-10-31T04:09:42Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginImport.java

+
+
 }


Extra blank lines at the end of the file should be removed to maintain consistent code formatting.

Copilot · 2025-10-31T04:09:42Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/MatchSection.java


 public class MatchSection extends PDESection implements IPartSelectionListener {

+


Extra blank line after the class declaration should be removed for consistent formatting.

Suggested change

gian-benito force-pushed the filterImport branch from cbb0d90 to 78bca44 Compare August 22, 2025 04:00

gian-benito changed the title ~~UI support for setting filter attributes in feature editor~~ UI support for setting filter attribute in feature editor Aug 22, 2025

laeubi requested a review from HannesWell September 4, 2025 05:35

laeubi reviewed Sep 4, 2025

View reviewed changes

gian-benito force-pushed the filterImport branch 2 times, most recently from 841e6ef to eb94655 Compare September 10, 2025 06:05

Added Filter Textfield support

eb94655

Co-authored-by: Brandon Algarra <[email protected]> Co-authored-by: Khang Nguyen <[email protected]>

laeubi requested a review from Copilot October 31, 2025 04:03

Copilot AI reviewed Oct 31, 2025

View reviewed changes

               	}
-              }
+              }

	fFilterText.setValue(((IFeatureImport) fCurrentImport).getFilter());
	fFilterText.setValue(fCurrentImport.getFilter());

	} catch (InvalidSyntaxException e) {
	} catch (InvalidSyntaxException e) {
	PDEPlugin.logException(e);


		public class MatchSection extends PDESection implements IPartSelectionListener {

UI support for setting filter attribute in feature editor #1942

Are you sure you want to change the base?

UI support for setting filter attribute in feature editor #1942

Uh oh!

Conversation

gian-benito commented Aug 22, 2025

Summary

Uh oh!

laeubi Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

laeubi Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

laeubi Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

laeubi Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

laeubi Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

gian-benito Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laeubi Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

gian-benito commented Sep 6, 2025

Uh oh!

gian-benito commented Oct 31, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gian-benito Sep 7, 2025 •

edited

Loading