Community
Participate
Working Groups
I200412162000 IEditorPart.getEditorInput() does not spec (Javadoc) to return null but it does so after the editor has been closed. Since getEditorSite() explicitly mentions the null case we assumed that IEditorPart.getEditorInput() never returns null.
Platform UI's code base is wildly inconsistent in its declaration of null return values and parameters. I've heard conflicting stories as to what the policy should be. I'll try to make this explicit for 3.1 -- either way.
> I've heard conflicting stories See: http://www.eclipse.org/articles/Article-API%20use/eclipse-api-usage-rules.html
I know about that doc, but I still *hear* conflicting stories. I don't believe that everyone knows about this doc. Either that or some people have decided to disagree with it. Personally, I feel it should always be explicit. That way plug-in developers know that a comment wasn't simply forgotten. But that's personal taste.
I won't commit this for 3.1 RC2, but plan on doing it for 3.1 RC3. Nick: I just want to clarify something. AbstractTextEditor calls EditorInput.setInput(null) in its dispose method. This causes the editor input to violate the specification for IEditorInput.getEditorInput(), which insists that it not be null. Is it strictly necessary for AbstractTextEditor to be setting the input to null in its dispose?
True, we set it to null. This seems not be needed when looking at the implementation. Since this bug is minor and we should concentrate on critical bugs I would not change that code now (Platform UI and Text side).
Agree with Dani's assessment. We should probably clear the input ourselves in EditorPart.dispose(). cc'ing Stefan to confirm.
This bug seems to be changing quite a bit. The original question was whether IEditorPart.getEditorInput() could return null. It seems the consensus is "yes". As such, I will fix up the doc in 3.1 RC3. If there are further questions about whether we should be doing stuff for free when disposing of parts, then perhaps we should open another bug?
Sorry, my suggestion to clear it does go counter to the current spec. It seems like we're only returning null due to AbstractTextEditor setting it to null (contrary to current spec). Please check with Stefan before making any change, as he has been trying to solidify the spec and behaviour in this area. If we change it now (even if to match current behaviour), it will need to be documented in the migration guide.
getEditorInput cannot return null. setInput cannot accept null. The spec has never permitted either of these cases, and I don't see any good reason to make this breaking change. However, the spec on dispose() says it must be the last method called on the part... so it isn't strictly necessary for a part to respect its public contract after the call to dispose. The only bug here is that AbstractTextEditor is calling EditorInput.setInput(null). We should be protecting against this with an Assert.
I have opened Bug 99715 with Platform Text, asking them to stop calling setInput(null). When that bug is fixed, we can add an assert to our own code. This is not critical for 3.1.
Moving Dougs bugs
Deferring
This is perhaps related, but maybe not. I wasn't sure where to put this. Neither AbstractTextEditor.setInput(IEditorInput) nor the chain of calls that it sets in motion set fTextEditor's editor input. This means that the fTextEditor and the AbstractTextEditor can have a different opinion about which inputEditor they are associated with. This doesn't seem to be breaking any features in stock Eclipse, but it caused me problems in some modifications I am doing to tab behaviour (for an experiment, not for release).
>fTextEditor's editor input What you meant here? If you find a bug report please just open a new bug with the details against Platform Text.
Ducky, you will need to follow Dani's suggestion. Your problem is definitely not related to the situation in this bug. Lowering the priority of this bug to reflect reality.
(mass update) Removing target milestone, this bug was targeted at a milestone in the past.
Boris I suggest to finally fix this early in the release by adding an Assert. In addition we could add an entry to the migration guide that this will be enforced in 3.6.
Created attachment 139167 [details] Patch adding Asserts
Marking for M1 to make sure this does not fall through the cracks. I haven't thought about this enough to say whether I agree with you, Dani, that we should make this change in 3.6.
OK, I digged a bit in the UI code and found this: 1. org.eclipse.ui.internal.PartTester.testEditor(IEditorPart) which gets called before an editor is opened blows up if the editor input is 'null', hence there can't be any useful/working client out there that sets 'null'. 2. in the Platform UI code you'll see that at some places IEditorPart.getEditorInput() checks for 'null' and at others it does not, which again tells us that it would have blown off long time ago when the input had been set to 'null'.
> 1. org.eclipse.ui.internal.PartTester.testEditor(IEditorPart) which gets called > before an editor is opened blows up if the editor input is 'null', hence there > can't be any useful/working client out there that sets 'null'. Based on this, it looks like the proposed change is safe. Let's do it for M1. Dani, do you want to release the change when HEAD is open again?
>Dani, do you want to release the change when HEAD is open again? Sure. We (Text, JDT have it open already) is this true for Platform UI as well?
(In reply to comment #22) > >Dani, do you want to release the change when HEAD is open again? > Sure. We (Text, JDT have it open already) is this true for Platform UI as well? I just sent a message to platform-ui-dev saying that HEAD is open for 3.6.
Fixed in HEAD. Available in builds > N20090623-2000.
