(simplified) Do not return null editor input from the ClassFileEditorInputFactory#2353
(simplified) Do not return null editor input from the ClassFileEditorInputFactory#2353laeubi wants to merge 1 commit into
Conversation
Currently ClassFileEditorInputFactory return null in cases when restoring an editor input from an IMemento and the class is not found on the (current) classpath of the project. This prevents the editor from showing up and results in a generic error message instead and prevents proper handling of the case in the editor. This now in case of an element not found by the project an InternalClassFileEditorInput is returned as we actually have the class data there. The editor can then further handel the case much better.
|
I don't see any difference to the original behavior in the use case below (original one from #2245).
I test it by creating a breakpoint at org.eclipse.pde.internal.core.RequiredPluginsInitializer.DeferredClasspathContainerInitializerJob.run(IProgressMonitor). Can you please provide steps to verify what the patch is doing? |
Please compare with the Picture from the report: 
it says Now its is "The class file is not on the classpath" and it is the 
|
|
I compare with master of JDT UI and see no difference. |
|
The please check if you hit this code path: that is what #2245 complains and I can reproduce it with a single project in the workspace started from Eclipse directly. "The class file is not on the classpath" is a different one and I would address this individually. What I did was:
No breakpoints or similar required. |
I created PR to handle the case here: |
|
@mehmet-karaman I think this one should be merged as a first PR so we get rid of this nasty error about |
| } | ||
| //if not found on the project still return an input | ||
| //so it can be handled by the editor | ||
| return new InternalClassFileEditorInput(cf); |
There was a problem hiding this comment.
I believe this will bring back https://bugs.eclipse.org/bugs/show_bug.cgi?id=83221.
Have you tried this case: same bundle from two different platform states.
The bundle is dependency of a workspace plugin project.
Start with platform IBuild_1, open class from that bundle, exit Eclipse.
Change launch config to use bundle version from platform IBuild_2 & start Eclipse.
What would happen? If we show class bytes, we have similar problem to https://bugs.eclipse.org/bugs/show_bug.cgi?id=83221 again.
There was a problem hiding this comment.
@iloveeclipse I must confess we are running in circles again, yes this will bring back "The classfile is not on the classpath" but this is intentional. Beside that even though Bug 83221 shows the same message the reason is different, anyways this is fixed with
so if you want that one to be merged first, I'm fine with that... we just need to solve one thing to break out of the cycle.
But me and @mehmet-karaman see this null pointer problem and that also what is complained in the initial report in JDT.ui ... I could also merge two PRs if it feels "better".
I also don't see how it could be better to run into a generic null problem than showing a proper message this is a bit out of scope here. So I understand Bug 83221 that it more fixes the issue with a changed classpath location then one where the file is absent all together.
There was a problem hiding this comment.
I believe it would be better to start from scratch:
- close all the PR's that are flying around (I count 4?), because each in isolation seem not to solve a problem we want to solve, and if partially merged they seem not to have much value or probably would rather make things worse.
- create one PR that solves the original problem - the class editor belongs to a platform bundle, the class file and the bundle exist in the target platform, we don't want to see error nor class bytes, we want to see source code on startup (but because of delayed PDE init can't be found by JDT UI at the startup time).
- On one of the PR's you've mentioned that the new input classes you introduced in some PR's were actually not needed, if that is the case, it would make sense to remove them.
- We can address all other issues you've mentioned (deleted classes, missing class files for some reasons, missing sources, show class bytes if there are no sources, whatever else) in different PR's once the original issue is solved.
There was a problem hiding this comment.
There is only one "original issue" and that is
this PR fixes the problem.
Everything else are just follow ups that solve different problems!
There was a problem hiding this comment.
this PR fixes the problem.
This PR doesn't fix it, I still see error editor and not the source of the previously opened class file.
See #2353 (comment) with steps to reproduce.
Here once again, with extra details to make sure we are testing same thing:
- Close org.eclipse.jface plugin project if you have it opened in the debugger
- In an empty workspace in debuggee Eclipse
- Create a simple plugin project
- Add org.eclipse.jface plugin as a plugin dependency to the MANIFEST.MF
- Switch to the Java perspective (default one, no extra views opened)
- Open ILabelProvider via "Open Type" (should be one single hit and it should show up in the Package Explorer under Plug-In dependencies of the bundle just created).
- Restart Eclipse
- Now an error part is shown, not the class file editor with source.
Probably you are misguided by some specific setup you have, or some specific class that always resolves for you, or some other bundles/projects in debuggee workspace.
I use plain SDK & plain local target with no m2e or other plugins that might affect classpath or target resolving in either way, and a single plugin bundle project in the workspace. Add JFace as dependency to the bundle and open few JFace classes like ILabelProvider etc. JFace project must be closed in the debugger workspace, so that the runtime sees bundle as a jar and not as a classpath directory faked by PDE. Before closing Eclipse, the editor is opened on the class file
To exclude the possibility it has something with the other dependencies resolving, I've also tried with a dummy bundle with two interfaces exported, I've exported that as deployable bundle to a directory along with sources and added the directory to the target platform, same effect (no improvement at all).
Note 1: you do not need the breakpoint, in most cases the PDE job is initialized classpath container after JDT UI wants to restore the editor. Breakpoint is there to guarantee the bug is reproducible independently on timing of your worksatation.
Note 2: #2351 sometimes fixes that (highly dependent on timing or something else, I guess JDT model events the code expects aren't always sent as expected). In most cases it also doesn't work, and simply disassembled code is shown... There is also an interesting side effect, that if the the first (shown) editor is not properly restored and shows only bytecode, opening any other Java file from same project and switching back to the .class editor shows back the sources but funny enough, it doesn't restore the breadcrumb, which is sometimes shown after some editor switches.
Note 3: you can be sure we are talking about same issue if after the startup that shows the "error" class file, same type can be opened via "Open Type" and opened class editor shows properly rendered Java code. That is what #2245 about. The class is there, the dependency is there, the sources can be found, just on startup it all doesn't work...
Sorry but it seems I can not help here if you insist to show a completely different problem than is described in I can 100% reproduce this 
with the exact stacktrace that is given in the ticket as I described here (no breakpoints, just 100% reproducible just from top of master or any SDK download) and what is also exactly what @mehmet-karaman can reproduce here. And that is what this issue fixed. So I let it decide to you (or JDT or whomever) how to proceed here, but it really becomes hilarious:
so instead of just merging both (as any of those is at least reproducible by one of you and go on with fixing other issues (e.g. no source is currently shows what is fixed here, one better continues to complain that I should fix "THE Problem". For me this only shows that obviously this is n_ot a problem as all or completely unimportant_, if not even small improvements can be completed so I just wasted my time to analyze and trying to improve something here. |
Instead of being that emotional, please try to understand my issue: I can reproduce the problem, I can see that none of the PR's is fixing this problem (except original trio created by Mehmet), and I know that the problem is there and real. So if you can't reproduce the problem, you probably should ask "why can't I reproduce it"?
I do not work in the model of "just merging" something, especially if I can see that the problem is not solved.
I can't understand why you don't want to accept the fact that other people also might have right. I do not complain, I simply state the fact that I can observe and reproduce a bug and the bug needs a fix and I don't see the fix in this PR. Nothing else. After the eclipse-pde/eclipse.pde#1667 we have regression in JDT UI. This regression can be reproduced with steps here: #2353 (comment). All what I want is a PR that fixes that. So far the fix was originally provided by @mehmet-karaman, but you was not agreed with that fix and started to create different PR's. This is right way to go, thanks for your efforts. I was asked to review these PR's and I did spend my time doing that, so far without success. This is how usual review goes. One checks the problem steps, checks the PR, sees the problem is not solved and asks for a fix. All what I want is a PR that fixes the problem. |
|
So well the I assume JDT will came up with a "real" fix and is not interested in contributions in that area, but then please don't blame PDE for that. I have created a local fix and it worked fine, except some hickups like seen here So I decided to merge whats already working and is easy to review, but it seems easier to blame other projects than trying to agree on smaller steps of improvements. |
|
@laeubi, please, try to be constructive, you show so much negative energy that it really not helpful.
This is wrong assumption, for sure we want contribution, but it doesn't mean we will merge everything contributed.
@laeubi , please try to reproduce with steps I showed. There is currently no opened PR that would fix that. If you would provide that PR, I will review it and if it works it will be merged.
I do not blame, a state the facts. We have a regression in JDT UI, the regression is coming from eclipse-pde/eclipse.pde#1667, it is reproducible, if I revert that PR, the problem disappears. I don't want anything special here. Take steps to reproduce. Provide PR that fixes that. Discuss on that PR towards solution. This is the way to go. |
|
I have now described what this fixes and how to verify the fix here Before the fixEditor does not restore and only shows a generic error message After the fixEditor does restore and detects a problem with the classpath and displays it to the user shown to the user I leave it to the maintainers of JDT to decide if that's something useful to fix and show the real problem instead to let the Editor itself look broken. Nothing more or less this PR fixes. |
|
Closing as not a problem see |
2 participants
Currently ClassFileEditorInputFactory return null in cases when restoring an editor input from an IMemento and the class is not found on the (current) classpath of the project. This prevents the editor from showing up and results in a generic error message instead and prevents proper handling of the case in the editor.
This now in case of an element not found by the project an InternalClassFileEditorInput is returned as we actually have the class data there. The editor can then further handel the case much better.