Create static variant of ImageDescriptor.imageDescriptorFromURI()

[ptziegler]

This method is intended to be used as a wrapper for createFromURL() to avoid having to deal with the checked MalformedURLException. However, this method is effectively unusable, as it requires the user to already have an instance of the ImageDescriptor they want to created.

Instead, create a new, static createFromURI() method, mark the old method as deprecated and internally delegate to the new method.

The old method was created as a response to Bug 559656 [1], in order to match the signature of IResourceUtilities. But because the latter is accessed via an OSGi service, it doesn't need to be static. Which is something that doesn't really work for image descriptors.

[1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=559656

[ptziegler]

@vogella You contributed the original method. wdyt?

[github-actions]

Test Results

 1 214 files   -   306   1 214 suites   - 306   1h 31m 50s ⏱️ - 1m 49s
 7 729 tests ±    0   7 495 ✅  -     6  233 💤 + 5  1 ❌ +1 
16 232 runs   - 6 456  15 716 ✅  - 6 419  515 💤  - 38  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 5ee6314. ± Comparison against base commit 1876695.

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.

[vogella]

LGTM, thanks @ptziegler

[laeubi]

As we are currently changing the method, catching NPE is really really BAD!
We should check for null like in createFromURL

Suggested change

	} catch (MalformedURLException | NullPointerException e) {
	} catch (MalformedURLException e) {

[ptziegler]

In e.g. createFromURL(), we have an explicit null check where getMissingImageDescriptor() is returned. I think it makes more sense to do the same here, rather than forwarding the exception.

[ptziegler]

I've also noticed that new URL(String) will be deprecated in Java 20, so I switched to the equivalent URI.toURL().

[merks]

Be very careful. It looks so innocent and obvious, bu there are cases where URI creation will thrown an exception where URL creation does not. E.g., if there is a space in the string

[vogella]

@laeubi do you still have open concerns here?

[laeubi]

Looks fine for me, using this method somewhere in the code (even in a testcase) might help to demonstrate its usefulness.

Maybe we should mention in the javadoc that this only works for URIs that can be converted to URLs?

[ptziegler]

Maybe we should mention in the javadoc that this only works for URIs that can be converted to URLs?

Adding a disclaimer sounds like a good idea.

[HannesWell]

Be very careful. It looks so innocent and obvious, bu there are cases where URI creation will thrown an exception where URL creation does not. E.g., if there is a space in the string

That's right.
But here an URI is converted into a URL and as far as I know URIs are strict regarding their content and therefore the conversion URI -> URL should be (relatively) save? But maybe I'm wrong. It's definitively not true for the opposite: URL -> URL can have many problems since URLs are not strictly validating their content.

[HannesWell]

Lets move this forward. The change looks good to me and unless somebody objects I plan to submit this within the next days.

Thank you @ptziegler.

Be very careful. It looks so innocent and obvious, bu there are cases where URI creation will thrown an exception where URL creation does not. E.g., if there is a space in the string

That's right. But here an URI is converted into a URL and as far as I know URIs are strict regarding their content and therefore the conversion URI -> URL should be (relatively) save? But maybe I'm wrong. It's definitively not true for the opposite: URL -> URL can have many problems since URLs are not strictly validating their content.

Since there was no opposition I assume that assessment is right, but with the current state of the doc even a more dangerous behavior would be fine.

[HannesWell]

If there are no further remarks, I plan to submit this Monday (European) evening.

[HannesWell]

Test failures are unrelated, submitting.

Thanks again.