Do not return null editor input from the ClassFileEditorInputFactory by laeubi · Pull Request #2349 · eclipse-jdt/eclipse.jdt.ui

laeubi · 2025-07-17T04:40:28Z

Currently ClassFileEditorInputFactory return null in some cases when restoring an editor input from an IMemento. While this is legal in general for IElementFactory it is not very useful for factories that restore editor inputs because this will fail to instantiate the editor with a nasty exception without any way for the editor to handle or the user to understand the underlying problem.

This now changes two things:

In case of an error, a special ExceptionalEditorInput is created, that when later used to gather its underlying data will rethrow the original exception so we get a useful message about the cause.
In case of an element not found by the project an InternalClassFileEditorInput is returned as we actually have the class data already there.

laeubi · 2025-07-17T06:54:42Z

 					type= project.findType(type.getFullyQualifiedName());
 					if (type == null)
-						return null;
+						return new InternalClassFileEditorInput(cf);


If one wants, here we can pass a boolean to Indicate that the type was not found on the project if that's useful for anything later on.

Currently ClassFileEditorInputFactory return null in some cases when restoring an editor input from an IMemento. While this is legal in general for IElementFactory it is not very useful for factories that restore editor inputs because this will fail to instantiate the editor with a nasty exception without any way for the editor to handle or the user to understand the underlying problem. This now changes two things: 1) In case of an error, a special ExceptionalEditorInput is created, that when later used to gather its underlying data will rethrow the original exception so we get a useful message about the cause. 2) In case of an element not found by the project an InternalClassFileEditorInput is returned as we actually have the class data already there.

laeubi · 2025-07-18T08:01:30Z

@iloveeclipse would you mind merging this PR as it already improves the situation? I then plan to do another smaller PR and then continue with

Let the ClassFileEditor wait unless a classfile in on the classpath #2351

iloveeclipse · 2025-07-18T08:20:18Z

This now changes two things:

In case of an error, a special ExceptionalEditorInput is created, that when later used to gather its underlying data will rethrow the original exception so we get a useful message about the cause.

In case of an element not found by the project an InternalClassFileEditorInput is returned as we actually have the class data already there.

Can you provide tests for both cases? I believe you could reuse some from Problem during restore of ClassFileEditor #2348?
Can you attach screenshots how it looks like with both cases?

I will try to review this afternoon.

laeubi · 2025-07-18T08:43:50Z

If one would test this it would better be to do it at ClassFileEditorInputFactory level, but there is no existing tests and due to how it works might not be very easy to test (one can do so for the null case but I'm not sure that's very interesting).

The testcase #2348 is very specific to the implementation details and does not very well covers the case we see here.

If there are concerns I can reduce the change to a much simpler one I just though to cover the other cases as well is maybe nice, see:

(simplified) Do not return null editor input from the ClassFileEditorInputFactory #2353

Currently it looks like that but I plan to add another PR for fix this but better want to do smaller steps that are easy to review and merge:

mehmet-karaman · 2025-07-18T09:01:58Z

For the test he needs to adjust the ClassFileEditor getAdapter method, because in original case it wasn't possible to gather the parent composite of that editor.. Maybe in that state he could just implement simple unit test and skip the UI test.. The UI test could be done in the other PR (otherwise he has to adjust the UI Test again in the other PR)

iloveeclipse · 2025-07-18T15:43:40Z

I don't see any difference to the original behavior in the use case below (original one from #2245).

Create a plugin project
Add any plugin as a plugin dependency to the MANIFEST.MF (for example: org.eclipse.jface)
Navigate to the newly created Project in the Package Explorer
Expand "Plug-in Dependencies"
Find the newly introduced depencies entry and open any class file in the ClassFileEditor (it just must be a class inside the -Plug-in Dependencies container)
Restart Eclipse

I test it by creating a breakpoint at org.eclipse.pde.internal.core.RequiredPluginsInitializer.DeferredClasspathContainerInitializerJob.run(IProgressMonitor).
If this breakpoint is set, I still see exact same picture like before: "The class file is not on the classpath".

Can you please provide steps to verify what the patch is doing?

mehmet-karaman · 2025-07-21T10:04:15Z

+
+	@Override
+	public String getToolTipText() {
+		return String.format("Error while gathering input for %s: %s", fIdentifier, fException); //$NON-NLS-1$


Is there not an externalization necessary?

I don't know if there is a separation inbetween UI and Exception messages. But this could be used in the UI

mehmet-karaman · 2025-07-21T10:10:24Z

+
+import org.eclipse.jdt.core.IOrdinaryClassFile;
+
+public class OrdinaryClassFileEditorInput extends InternalClassFileEditorInput {


Why don't we use directly the InternalClassFileEditorInput and create a new class without any additional functionality or marking (Checked also against the check_incomplete_input branch but couldn't find the use of it)

The idea was to make this case distinguishable from an "normal" input, but might not be strictly needed. In the meantime I cane to the conclusion that maybe the whole checks here can be omitted and everything is better handled in the editor, but one should not change to much at once ... so I would like to wait if we can agree at least on something that should be merged.

In overall the solution looks good. The second PR has no issues from my side.. The last PR (load the Sources when it is available) worked only once and then never again.. I will try to understand the problem, did you pushed your latest changes there?

Lets first finish this ones

If there is no source show the raw class file content #2357

(simplified) Do not return null editor input from the ClassFileEditorInputFactory #2353

before further proceed, it all gets to much complicate to have even more PRs inflight. Once this is done I can rebase and we can easier discuss on possible improvements or increments.

Is the merge order of the PRs correct?

Do not return null editor input from the ClassFileEditorInputFactory #2349

If there is no source show the raw class file content #2357

(simplified) Do not return null editor input from the ClassFileEditorInputFactory #2353

2349 contains already the change from 2353 ..

The order might be:
2353
2349
2357

laeubi · 2025-07-22T14:28:33Z

Closing as not enough interest from JDT maintainers and

(simplified) Do not return null editor input from the ClassFileEditorInputFactory #2353

is simpler.

This was referenced Jul 17, 2025

Editor is not restored leading to ClassFileEditorInputFactory returned null error #2245

Closed

Problem during restore of ClassFileEditor #2348

Closed

Problem during restore of ClassFileEditor eclipse-pde/eclipse.pde#1876

Closed

laeubi commented Jul 17, 2025

View reviewed changes

laeubi force-pushed the do_not_return_null_from_factory branch from 79d6555 to 66378bc Compare July 17, 2025 07:33

laeubi mentioned this pull request Jul 17, 2025

Let the ClassFileEditor wait unless a classfile in on the classpath #2351

Closed

mehmet-karaman reviewed Jul 21, 2025

View reviewed changes

laeubi closed this Jul 22, 2025


		import org.eclipse.jdt.core.IOrdinaryClassFile;

		public class OrdinaryClassFileEditorInput extends InternalClassFileEditorInput {

Uh oh!

Conversation

laeubi commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laeubi commented Jul 18, 2025

Uh oh!

iloveeclipse commented Jul 18, 2025

Uh oh!

laeubi commented Jul 18, 2025

Uh oh!

mehmet-karaman commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iloveeclipse commented Jul 18, 2025

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!

mehmet-karaman Jul 21, 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!

laeubi commented Jul 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mehmet-karaman commented Jul 18, 2025 •

edited

Loading

mehmet-karaman Jul 21, 2025 •

edited

Loading