PDE should not warn if resource URI of unknown scheme cannot be found by Dani-Hub · Pull Request #2032 · eclipse-pde/eclipse.pde

Dani-Hub · 2025-10-19T15:30:37Z

Fixes #2031

Dani-Hub · 2025-10-19T17:30:39Z

Could someone help my try to understand the test failures? I would like to see the test details for judgment on whether it is possibly caused by my changes or not - Thanks!

github-actions · 2025-10-19T21:32:05Z

Test Results

771 files ±0 771 suites ±0 54m 49s ⏱️ + 3m 10s
3 637 tests ±0 3 583 ✅ ±0 54 💤 ±0 0 ❌ ±0
10 845 runs ±0 10 682 ✅ ±0 163 💤 ±0 0 ❌ ±0

Results for commit e7f3431. ± Comparison against base commit b00ef59.

♻️ This comment has been updated with latest results.

Dani-Hub · 2025-10-21T15:04:50Z

Any help would be appreciated.

HannesWell

Sorry for the late reply, the past few days have been busy.
I have added a few general comments, altough I think we should revisite the proposed solution as discussed in #2031 (comment) ff.

But still I think this is a good enhancement if we find a suitable solution that fits all cases.

HannesWell · 2025-10-23T20:09:05Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/ExtensionsErrorReporter.java

+		if (bundleJar == null && isValidUriOfUnknownProtocol(location)) {
+			return true;
+		}
+
+		return false;


This can be more compact, as if true then return true is a bit cumbersome.

Suggested change

if (bundleJar == null && isValidUriOfUnknownProtocol(location)) {

return true;

}

return false;

return bundleJar == null && isValidUriOfUnknownProtocol(location);

HannesWell · 2025-10-23T20:11:38Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/ExtensionsErrorReporter.java

+				boolean isKnownProtocol = false;
+				// Exclude those URI forms where we have already special
+				// handling for or where we are sure that they are known URLs:
+				for (String knownScheme : KNOWN_URI_SCHEMES) {
+					if (knownScheme.equalsIgnoreCase(scheme)) {
+						isKnownProtocol = true;
+						break;
+					}
+				}


I know large parts of the PDE code-base are still in an old Java style, but that shouldn't prevent us from using newer, more readable styles in new or modified parts of the code:

Suggested change

boolean isKnownProtocol = false;

// Exclude those URI forms where we have already special

// handling for or where we are sure that they are known URLs:

for (String knownScheme : KNOWN_URI_SCHEMES) {

if (knownScheme.equalsIgnoreCase(scheme)) {

isKnownProtocol = true;

break;

}

}

boolean isKnownProtocol = KNOWN_URI_SCHEMES.stream().anyMatch(scheme::equalsIgnoreCase);

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/ExtensionsErrorReporter.java

Dani-Hub · 2025-10-26T11:47:27Z

I don't understand how I got this merge mess. Any suggestions?

iloveeclipse · 2025-10-26T12:01:26Z

I don't understand how I got this merge mess. Any suggestions?

Don't merge but rebase your commit on top of master.

merks · 2025-10-26T12:06:01Z

And, once you have PR started, use amend and force push for any subsequent changes.

Sometimes when I get into this state, I rename the branch to which I pushed my PR changes. check out master, do a pull, create a new branch with exactly the same name as the one I was using, apply the changes, then commit and force push by my fork. This will overwrite all the commits in that branch in your force and will update this existing PR.

Dani-Hub · 2025-10-26T12:23:15Z

Actually I did ammend, but somehow a previous PULL must have happened. I follow now Ed's approach, thank you!

Dani-Hub · 2025-10-28T03:55:47Z

@HannesWell : A friendly reminder for a second review - Thanks!

HannesWell · 2025-10-28T06:03:47Z

A friendly reminder for a second review - Thanks!

I plan to have a look at it this evening.

HannesWell

Thank you for the update and your patience (Sorry last week was busy).

Generally this looks already quite good, but the new preference page don't look nice at the moment and the method that performs the actual check can be simpler.
Please see my comments below.

HannesWell · 2025-11-03T17:45:38Z

...lipse.pde.ui/src/org/eclipse/pde/internal/ui/preferences/PDECompilersConfigurationBlock.java

-	private static Key[] fgAllKeys = {KEY_F_UNRESOLVED_FEATURES, KEY_F_UNRESOLVED_PLUGINS, KEY_P_BUILD, KEY_P_BUILD_MISSING_OUTPUT, KEY_P_BUILD_SOURCE_LIBRARY, KEY_P_BUILD_OUTPUT_LIBRARY, KEY_P_BUILD_SRC_INCLUDES, KEY_P_BUILD_BIN_INCLUDES, KEY_P_BUILD_JAVA_COMPLIANCE, KEY_P_BUILD_JAVA_COMPILER, KEY_P_BUILD_ENCODINGS, KEY_P_INTERNAL, KEY_P_SERVICE_COMP_WITHOUT_LAZY, KEY_P_NO_AUTOMATIC_MODULE_NAME, KEY_P_DEPRECATED, KEY_P_DISCOURAGED_CLASS, KEY_P_INCOMPATIBLE_ENV, KEY_P_MISSING_EXPORT_PKGS, KEY_P_NO_REQUIRED_ATT, KEY_P_NOT_EXTERNALIZED, KEY_P_UNKNOWN_ATTRIBUTE, KEY_P_UNKNOWN_CLASS, KEY_P_UNKNOWN_ELEMENT, KEY_P_UNKNOWN_IDENTIFIER, KEY_P_UNKNOWN_RESOURCE, KEY_P_UNRESOLVED_EX_POINTS, KEY_P_UNRESOLVED_IMPORTS, KEY_P_VERSION_EXP_PKG, KEY_P_VERSION_IMP_PKG, KEY_P_VERSION_REQ_BUNDLE, KEY_P_VERSION_EXEC_ENV_TOO_LOW, KEY_S_CREATE_DOCS, KEY_S_DOC_FOLDER, KEY_S_OPEN_TAGS };
+			private static Key[] fgAllKeys = { KEY_F_UNRESOLVED_FEATURES, KEY_F_UNRESOLVED_PLUGINS, KEY_P_BUILD,
+					KEY_P_BUILD_MISSING_OUTPUT, KEY_P_BUILD_SOURCE_LIBRARY, KEY_P_BUILD_OUTPUT_LIBRARY,
+					KEY_P_BUILD_SRC_INCLUDES, KEY_P_BUILD_BIN_INCLUDES, KEY_P_BUILD_JAVA_COMPLIANCE,
+					KEY_P_BUILD_JAVA_COMPILER, KEY_P_BUILD_ENCODINGS, KEY_P_INTERNAL, KEY_P_SERVICE_COMP_WITHOUT_LAZY,
+					KEY_P_NO_AUTOMATIC_MODULE_NAME, KEY_P_DEPRECATED, KEY_P_DISCOURAGED_CLASS, KEY_P_INCOMPATIBLE_ENV,
+					KEY_P_MISSING_EXPORT_PKGS, KEY_P_NO_REQUIRED_ATT, KEY_P_NOT_EXTERNALIZED, KEY_P_UNKNOWN_ATTRIBUTE,
+					KEY_P_UNKNOWN_CLASS, KEY_P_UNKNOWN_ELEMENT, KEY_P_UNKNOWN_IDENTIFIER, KEY_P_UNKNOWN_RESOURCE,
+					KEY_P_IGNORED_RESOURCE_PROTOCOLS, KEY_P_UNRESOLVED_EX_POINTS, KEY_P_UNRESOLVED_IMPORTS,
+					KEY_P_VERSION_EXP_PKG, KEY_P_VERSION_IMP_PKG, KEY_P_VERSION_REQ_BUNDLE,
+					KEY_P_VERSION_EXEC_ENV_TOO_LOW, KEY_S_CREATE_DOCS, KEY_S_DOC_FOLDER, KEY_S_OPEN_TAGS };


Please don't indent this field so deep.
I know this is result of just formatting the fields lines, but this is just because the previous field isn't properly formatted (and somehow this confuses the formattter).
This can be avoided if you format the field above too.
If you want to you can also include a 'formatting'-fix of the previous field in this change.

Thanks for the hint how to solve this - done.

Great 👍🏽
Since you've modified this and the field above already, you could make it final too.

HannesWell · 2025-11-03T18:20:28Z

...lipse.pde.ui/src/org/eclipse/pde/internal/ui/preferences/PDECompilersConfigurationBlock.java

+				Composite comp = SWTFactory.createComposite(client, 2, 2, GridData.FILL_HORIZONTAL, 0, 0);
+				createTextControl(comp, PDEUIMessages.compilers_p_ignored_uri_protocols,
+						KEY_P_IGNORED_RESOURCE_PROTOCOLS, CompilerFlags.PLUGIN_FLAGS);
+				SWTFactory.createLabel(comp, PDEUIMessages.compilers_p_ignored_uri_protocols_details, 2);


On Windows-11 this doesn't look good:

The label shouldn't be in bold case and I think the details shouldn't be in a separate line. Instead I think you should provide the details as a tool-tip. It could be set after returning the created Text from the createTextControl() method.

I cannot tell immediately why the label is bold, but maybe it's related to how the previous elements are created (cascaded into each other).

Oops, I'll need to look deeper into that - tomorrow.

I understand now where the bold font is coming from: It is caused by the usage of the SWTFactory methods which all have in common to set the font of the created widget based on the font of its parent. But the parent in this case is the one created from createExpansibleComposite, which sets a bold font. This explains why the existing methods in PDECompilersConfigurationBlock such as createComboControl do not use the SWTFactory methods for label and combo creation. My current approach to fix this is to create my intermediate Composite manually without SWTFactory being involved and intentionally do not set the parent font. Is this an acceptable approach (See my last commit) or what would you suggest as an alternative here?

Good investigation. Now this looks nice. :)

HannesWell · 2025-11-03T18:35:25Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/ExtensionsErrorReporter.java

+		final var ignoredProtocols = CompilerFlags.getString(fProject, CompilerFlags.P_IGNORED_RESOURCE_PROTOCOLS);
+		final var st = new StringTokenizer(ignoredProtocols, ","); //$NON-NLS-1$
+		if (!st.hasMoreTokens()) {
+			return false;
+		}
+		try {
+			final var uri = new URI(location);
+			if (uri.isAbsolute()) {
+				// Exclude syntactically valid file paths that are also
+				// syntactically valid URIs, such as "C:/somePath" where
+				// "C" would be interpreted as URI protocol:
+				try {
+					java.nio.file.Path.of(location);
+				} catch (java.nio.file.InvalidPathException e) {
+					// OK, location does not match a file path
+					final var scheme = uri.getScheme();
+					while (st.hasMoreElements()) {
+						String protocol = st.nextToken().trim();
+						if (scheme.equalsIgnoreCase(protocol)) {
+							return true;
+						}
+					}
+				}
+			}
+		} catch (URISyntaxException e) {
+			// location is not a valid URI
+		}
 		return false;


As RFC 2396 specifies that the scheme must be followed by a colon (for absolute URIs) I think this entire method can be simplified to just

Suggested change

final var ignoredProtocols = CompilerFlags.getString(fProject, CompilerFlags.P_IGNORED_RESOURCE_PROTOCOLS);

final var st = new StringTokenizer(ignoredProtocols, ","); //$NON-NLS-1$

if (!st.hasMoreTokens()) {

return false;

}

try {

final var uri = new URI(location);

if (uri.isAbsolute()) {

// Exclude syntactically valid file paths that are also

// syntactically valid URIs, such as "C:/somePath" where

// "C" would be interpreted as URI protocol:

try {

java.nio.file.Path.of(location);

} catch (java.nio.file.InvalidPathException e) {

// OK, location does not match a file path

final var scheme = uri.getScheme();

while (st.hasMoreElements()) {

String protocol = st.nextToken().trim();

if (scheme.equalsIgnoreCase(protocol)) {

return true;

}

}

}

}

} catch (URISyntaxException e) {

// location is not a valid URI

}

return false;

String ignoredProtocols = CompilerFlags.getString(fProject, CompilerFlags.P_IGNORED_RESOURCE_PROTOCOLS);

if (ignoredProtocols.isEmpty()) {

return false;

}

return Arrays.stream(ignoredProtocols.split(",")).map(p -> p + ":").anyMatch(location::startsWith); //$NON-NLS-1$ //$NON-NLS-2$

I.e. it just checks if the location starts with any protocol plus a colon.

There are two issues with this alternative solution: (1) It doesn't trim any leading or trailing spaces (and this seems to be done everywhere else) and (2) it doesn't do a case-insensitive match (protocol comparison is defined to be case-insensitive). So I'm working on a modified approach now.

That's both correct.
But your modifications to refine this approach look good. :)

Dani-Hub · 2025-11-06T15:42:13Z

@HannesWell Could you please proceed with the review?

HannesWell

Thank you for the update.
The UI looks better now, but it still has to be improved further. Please see my remarks below.

Do you also have a simple example at hand to try out the possibly ignored URI protocols, i.e. a corresponding plugin.xml snippet?

HannesWell · 2025-11-07T00:10:32Z

...lipse.pde.ui/src/org/eclipse/pde/internal/ui/preferences/PDECompilersConfigurationBlock.java

+				Composite comp = createComposite(client, 2, 2, GridData.FILL_HORIZONTAL, 0, 0);
+				createTextControl(comp, PDEUIMessages.compilers_p_ignored_uri_protocols,
+						KEY_P_IGNORED_RESOURCE_PROTOCOLS, CompilerFlags.PLUGIN_FLAGS);
+				SWTFactory.createLabel(comp, PDEUIMessages.compilers_p_ignored_uri_protocols_details, 2);


This label should be a tool-tip of the text field instead.
Not having an extra description field makes the UI cleaner.

And it maybe makes the code even simpler as you can maybe even skip the extra container/Composite. But I'm not sure about the latter and it would have to be investigated/tried out.

You can get the text-field by returning it from the createTextControl() method.

This label should be a tool-tip of the text field instead. Not having an extra description field makes the UI cleaner.

All existing comparable text fields in the Eclipse wizards providing comma-separated lists use an extra label to explain the usage. See for example:

In the File Search dialog we have a comparable pattern:

In the rarer case were no such a label is provided the Eclipse UI does not show a tool tip. I would like to point these out before we change the design. So, given this additional information, do we still want to user your alternative approach?

In the rarer case were no such a label is provided the Eclipse UI does not show a tool tip. I would like to point these out before we change the design. So, given this additional information, do we still want to user your alternative approach?

Thanks for the analysis and comparison.
I would argue that wizards are also a bit different preferences and I have to admit it looked a bit strange to me at this point respectively it stand out from the rest, which I think shouldn't be the case. If you really prefer to have the description as a label, maybe it's better to have it above?

I'm personally completely open to any direction. I just pointed out the seemingly existing practice so that we have a complete picture to argue about and I will apply any preferred UI approach that you reviewers would like to see.

I have now moved the explanatory label before the text field in my last ammendment. Does this look better to you? (It doesn't to me but again, please decide yourself)

I just pointed out the seemingly existing practice so that we have a complete picture to argue about and I will apply any preferred UI approach that you reviewers would like to see.

I have looked through the other pages in the PDE section and not found a fully comparable case and I still have the impression that a separate label text 'breaks the pattern' on this page compared to the other entries (which admittedly are all similar but different to this one):

With my proposal to use a tool tip, I still think this fits best into the existing page, even though this is handled differently in some other wizards:

...lipse.pde.ui/src/org/eclipse/pde/internal/ui/preferences/PDECompilersConfigurationBlock.java

Dani-Hub · 2025-11-07T16:04:22Z

@HannesWell Could you please proceed with the review?

Thank you for the update. The UI looks better now, but it still has to be improved further. Please see my remarks below.

Do you also have a simple example at hand to try out the possibly ignored URI protocols, i.e. a corresponding plugin.xml snippet?

Yes, in the original example is a complete example project, here is a link to it.

Fixes eclipse-pde#2031

HannesWell

@Dani-Hub I have now pushed an update to your branch that uses the a tool-tip to add the explanation to the preference, as per other thread in this PR.

Furthermore I added a default value initialization which was missing and broke the restore of defaults for the PDE preference page that hosts the new option.

Also thank you for providing your example, that showed that this works as intended.

With all that the current state is ready for submission from my POV.
Unless you'd like to have things differently I'll submit this tomorrow evening.

If you want to push further updates to this PR, please make sure to fetch the latest state from your fork first.

Dani-Hub · 2025-11-11T15:33:50Z

@Dani-Hub I have now pushed an update to your branch that uses the a tool-tip to add the explanation to the preference, as per other thread in this PR.

Looks good to me!

Furthermore I added a default value initialization which was missing and broke the restore of defaults for the PDE preference page that hosts the new option.

Thanks for that fix!

Also thank you for providing your example, that showed that this works as intended.

With all that the current state is ready for submission from my POV. Unless you'd like to have things differently I'll submit this tomorrow evening.

If you want to push further updates to this PR, please make sure to fetch the latest state from your fork first.

No, I have nothing to change, thanks for your help!

HannesWell · 2025-11-11T21:37:58Z

Great, submitting it then.
Thank you for this contribution.

HannesWell force-pushed the #2031-PDE_should_not_warn_if_resource_URI_of_unknown_scheme_cannot_be_found branch from cfd0eec to dd902cc Compare October 23, 2025 20:01

HannesWell reviewed Oct 23, 2025

View reviewed changes

Dani-Hub force-pushed the #2031-PDE_should_not_warn_if_resource_URI_of_unknown_scheme_cannot_be_found branch from e94873d to e3d5fea Compare October 26, 2025 13:06

HannesWell requested changes Nov 3, 2025

View reviewed changes

Dani-Hub force-pushed the #2031-PDE_should_not_warn_if_resource_URI_of_unknown_scheme_cannot_be_found branch 5 times, most recently from 15f65d5 to e67db37 Compare November 4, 2025 19:13

HannesWell requested changes Nov 7, 2025

View reviewed changes

Dani-Hub force-pushed the #2031-PDE_should_not_warn_if_resource_URI_of_unknown_scheme_cannot_be_found branch 2 times, most recently from e0a569c to d86589b Compare November 10, 2025 03:17

HannesWell force-pushed the #2031-PDE_should_not_warn_if_resource_URI_of_unknown_scheme_cannot_be_found branch from d86589b to 13c3829 Compare November 10, 2025 22:37

PDE should not warn if resource URI of unknown scheme cannot be found

e7f3431

Fixes eclipse-pde#2031

HannesWell force-pushed the #2031-PDE_should_not_warn_if_resource_URI_of_unknown_scheme_cannot_be_found branch from 13c3829 to e7f3431 Compare November 10, 2025 22:38

HannesWell approved these changes Nov 10, 2025

View reviewed changes

HannesWell merged commit 08a144c into eclipse-pde:master Nov 11, 2025
19 checks passed

Conversation

Dani-Hub commented Oct 19, 2025

Uh oh!

Dani-Hub commented Oct 19, 2025

Uh oh!

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

Uh oh!

Test Results

Uh oh!

Dani-Hub commented Oct 21, 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!

Uh oh!

Dani-Hub commented Oct 26, 2025

Uh oh!

iloveeclipse commented Oct 26, 2025

Uh oh!

merks commented Oct 26, 2025

Uh oh!

Dani-Hub commented Oct 26, 2025

Uh oh!

Dani-Hub commented Oct 28, 2025

Uh oh!

HannesWell commented Oct 28, 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!

Dani-Hub Nov 4, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dani-Hub Nov 3, 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!

Choose a reason for hiding this comment

Uh oh!

Dani-Hub commented Nov 6, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Dani-Hub commented Nov 7, 2025

Uh oh!

github-actions bot commented Oct 19, 2025 •

edited

Loading

Dani-Hub Nov 4, 2025 •

edited

Loading

Dani-Hub Nov 3, 2025 •

edited

Loading