Community
Participate
Working Groups
Build Identifier: 20110519-0100 In Eclipse 3.7, there is now an assert. This BREAKS the setInput API. Previously we were allowed to set input to NULL without any problems. Our application is large and often sets the editor input to NULL. It is not easy for us to no longer pass in NULL. I don't uunderstand why null is not allowed in the setter. When EditorPart is first created, by default the editor input member is NULL. so it's a valid state! protected void setInput(IEditorInput input) { Assert.isLegal(input != null); editorInput = input; } Reproducible: Always Steps to Reproduce: 1. Try to set the editor input to NULL :-)
This seems to have been added in 3.6 actually. See bug 81514.
(In reply to comment #0) > This BREAKS the setInput API. Previously we were allowed to set input to NULL The Eclipse API guidelines (http://www.eclipse.org/articles/article.php?file=Article-API-Use/index.html) are very clear about this: Null parameters. Do not pass null as a parameter to an API method unless the parameter is explicitly documented as allowing null. This is perhaps the most frequently made programming error. The Javadoc of org.eclipse.ui.part.EditorPart.setInput(IEditorInput) as of 09/24/2002: /** * Sets the input to this editor. * * @param input the editor input */ ==> null was and is not allowed. That it worked for you was pure luck.
Hi, Thanks for the explanation. But as it seems, it is perfectly valid for EditorPart to have a null input (it is initialized in this state after all). Up to 3.5, null was allowed - whether intentional or not, I don't know. I agree with you - the API should have documented it. My major concern, which i'm sure you can see, is you have now BROKEN existing applications that used to set null for this and it was allowed. For example, our application now crashes every time we close our application file (which is represented as an editor input). When we close our file, we set null to indicate the editor has nothing. EditorInput.setInput () is not a package internal method; it is a public method so as such I would expect, as a developer, that the behavior of a public API stays consistent. To suddenly disallow a once previous value (whether erroneous or not) is clearly a break in the behavior. That is a bit irresponsible. (In reply to comment #2) > (In reply to comment #0) > > This BREAKS the setInput API. Previously we were allowed to set input to NULL > The Eclipse API guidelines > (http://www.eclipse.org/articles/article.php?file=Article-API-Use/index.html) > are very clear about this: > Null parameters. Do not pass null as a parameter to an API method unless the > parameter is explicitly documented as allowing null. This is perhaps the most > frequently made programming error. > The Javadoc of org.eclipse.ui.part.EditorPart.setInput(IEditorInput) as of > 09/24/2002: > /** > * Sets the input to this editor. > * > * @param input the editor input > */ > ==> null was and is not allowed. That it worked for you was pure luck.
> To suddenly disallow a once previous value (whether erroneous > or not) is clearly a break in the behavior. That is a bit irresponsible. What matters is the contract and the contract is given by the Javadoc. Just because it worked for you doesn't mean it's right. Otherwise you could claim that calling internal code worked and now it's broken.
(In reply to comment #4) > > To suddenly disallow a once previous value (whether erroneous > > or not) is clearly a break in the behavior. That is a bit irresponsible. > What matters is the contract and the contract is given by the Javadoc. Just > because it worked for you doesn't mean it's right. Otherwise you could claim > that calling internal code worked and now it's broken. I generally agree with you Dani but on the other hand what was the reason to introduce the "Assert.isLegal(input != null);" line?
(In reply to comment #5) > (In reply to comment #4) > > > To suddenly disallow a once previous value (whether erroneous > > > or not) is clearly a break in the behavior. That is a bit irresponsible. > > What matters is the contract and the contract is given by the Javadoc. Just > > because it worked for you doesn't mean it's right. Otherwise you could claim > > that calling internal code worked and now it's broken. > > I generally agree with you Dani but on the other hand what was the reason to > introduce the "Assert.isLegal(input != null);" line? I'd had to dig out all the details again but AFAIR there were ugly issues later when people asked for the input and expected it to be not null as specified in the API. Think of whether you launch a rocket with a bomb and hope it doesn't explode or take measures that the rocket doesn't even launch.
The comment in EditorPart.java for the editorInput clearly says it's ok for editorInput to be NULL (see pasted code below) In fact it is initialized to null. This is inconsistent with what you're telling me? A user can still call getInput () and get a null editor input. Should you initialize it to something non-null? Like a dummy, no-op editor input? If EditorInput was initializing this to something non-null, then I could more easily accept that NULL is not valid. That is what I'm being forced to do now for our application since we cannot use null. I have to create some dummy representation for no editor input. /** * Editor input, or <code>null</code> if none. */ private IEditorInput editorInput = null; Internal/package internal code is different; you clearly stipulate it could change so we know full well that if we decide to use it, we will not be surprised if it changes between versions. Anyways, I guess i didn't make it clear that I totally understand you :-) I agree that if the Eclipse API guidelines say so, then setInput () was actually wrong all this time. I agree that just because it could be done, it doesn't mean it was right :-) Absolutely. But the point i was trying to make is the practical side of this all. You've shipped many versions already with this loophole and suddenly taken it away. You are bound to break somebody's application. Could you have deprecated this and introduced a new api instead? That would have been less shocking than having the application throw up after an api call that used to work. Eclipse is a great platform to work with - and there are many users, not just one or two that can be affected. I enjoy Eclipse very much which is why i'm taking the time to gripe about this. I'm griping out of passion :-) Jonathan (In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > > To suddenly disallow a once previous value (whether erroneous > > > > or not) is clearly a break in the behavior. That is a bit irresponsible. > > > What matters is the contract and the contract is given by the Javadoc. Just > > > because it worked for you doesn't mean it's right. Otherwise you could claim > > > that calling internal code worked and now it's broken. > > > > I generally agree with you Dani but on the other hand what was the reason to > > introduce the "Assert.isLegal(input != null);" line? > I'd had to dig out all the details again but AFAIR there were ugly issues later > when people asked for the input and expected it to be not null as specified in > the API. Think of whether you launch a rocket with a bomb and hope it doesn't > explode or take measures that the rocket doesn't even launch.
Yes, the private field has a comment that talks about 'null' but it should be set to something useful right after the part is created via init(...) which also doesn't allow 'null'. BTW: note the comment of IEditorPart.getEditorSite(): there you see that if 'null' is allowed it is documented. I do see your point too but the changes were made more than two years ago with no complaints so far and they were made to make the framework more save.
While doing a search, I found that the platform defines NullEditorInput class already. We'll use that then to represent "no editor input" Yeah, we just upgraded to 3.7 so just ran into the issue now. Due to dependencies on other cross-site teams, we cannot upgrade often. (In reply to comment #8) > Yes, the private field has a comment that talks about 'null' but it should be > set to something useful right after the part is created via init(...) which > also doesn't allow 'null'. BTW: note the comment of > IEditorPart.getEditorSite(): there you see that if 'null' is allowed it is > documented. > I do see your point too but the changes were made more than two years ago with > no complaints so far and they were made to make the framework more save.
(In reply to comment #9) > While doing a search, I found that the platform defines NullEditorInput class > already. We'll use that then to represent "no editor input" This is not API, so please don't open a bug report when this class gets deleted in 3.8 ;-).
don't worry. after i wrote my last comment, i realized this class was in the internal packages so I am not going to use it. We'll create our own class for this purpose. cheers! (In reply to comment #10) > (In reply to comment #9) > > While doing a search, I found that the platform defines NullEditorInput class > > already. We'll use that then to represent "no editor input" > This is not API, so please don't open a bug report when this class gets deleted > in 3.8 ;-).
