Do not return null from ClassFileEditorInputFactory when type not found#2372
Do not return null from ClassFileEditorInputFactory when type not found#2372laeubi wants to merge 1 commit intoeclipse-jdt:masterfrom
Conversation
Currently the ClassFileEditorInputFactory tries to update a stored element reference to a type in the project when it encounters an IOrdinaryClassFile. If that lookup fails (for whatever reason) it return null that results in a complete crash of the editor during restore as null is not a valid EditorInput. This now returns an InternalClassFileEditorInput instead to make it show the editor again after a restart. Fixes eclipse-jdt#2371
| type= project.findType(type.getFullyQualifiedName()); | ||
| if (type == null) | ||
| return null; | ||
| return new InternalClassFileEditorInput(cf); |
There was a problem hiding this comment.
I've commented already on #2353 (comment).
This would bring back the problem described in the comment above, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=83221
I will push a patch that fixes that in a different way without regression.
There was a problem hiding this comment.
This would bring back the problem described in the comment above, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=83221
Can you please explain why. The null is only returned when the type is not found, the Bug 83221 is fixing the case where it is found...
There was a problem hiding this comment.
Because of #2245 aka eclipse-pde/eclipse.pde#1667.
Have you tried out the reproducer? It does not contain anything from PDE just plain JDT project...
There was a problem hiding this comment.
Also please look at the patch from Bug 83221 da65fe2
it does not aimed to ever return null .. then there was a potential problem that the type is null and the check was returned then returning null here 5a30402 .
so how could it be a "regression" to the bug if it never would open the editor in such case? This does not make sense as one really can't depend on seeing a broken ClassFileEditor (again without PDE) after a restart, so this is clearly a bug and it is unrelated to if PDE has any change or not.
You asked me to stay "professional" but honestly this does not feel any profession from your side to ever repeat the same sentence without even look at the reported problem.
There was a problem hiding this comment.
but honestly this does not feel any profession from your side to ever repeat the same sentence without even look at the reported problem.
How do you know that? Please stop making offending personal comments!
Please carefully read https://github.com/eclipse-jdt/eclipse.jdt.ui?tab=coc-ov-file#our-standards and understand that personal attacks are violating our code of conduct.
I do not want to answer to such comments anymore.
There was a problem hiding this comment.
You asked me to create an own issue for my different problem and I did it here:
it describes detailed steps how to reproduce the issue that ClassFileEditorInputFactory return null with an example project that only uses JDT.
I also added detailed description in the PR to fix that problem when it was introduced and why it causes problem.
Then you argue that it is caused by a change in PDE... so maybe you can kindly give me an advice on what conclusion I should draw from that?
|
I'm closing this in favor of |
2 participants
Currently the
ClassFileEditorInputFactorytries to update a stored element reference to a type in the project when it encounters anIOrdinaryClassFile. If that lookup fails (for whatever reason) it returnnullthat results in a complete crash of the editor during restore asnullis not a validEditorInput.What it does
This now returns an
InternalClassFileEditorInputinstead to make it show the editor again after a restart.Fixes #2371
How to test
See instructions here:
Author checklist
@iloveeclipse please kindly review this as it is a long outstanding issue (has been introduced 19 years ago with fixing a NPE in Bug 170552 as a follow up from Bug 83221) it would be good to have it in M2.
This has also been reported before as
It is NOT related to the follwoing (so please don't expect it shows any source)
FYI @merks