Bugzilla – Full Text Bug Listing
| Summary: | [hotbug] CNF Server View taking over other extensions' label provider! | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP ServerTools | Reporter: | Rob Stryker <stryker> | ||||||||||
| Component: | wst.server | Assignee: | Rob Stryker <stryker> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Angel Vera <arvera> | ||||||||||
| Severity: | major | ||||||||||||
| Priority: | P3 | CC: | david_williams, raghunathan.srinivasan | ||||||||||
| Version: | 3.2 | Flags: | david_williams:
pmc_approved+
raghunathan.srinivasan: pmc_approved+ arvera: pmc_approved? (naci.dai) deboer: pmc_approved+ arvera: pmc_approved? (neil.hauge) arvera: pmc_approved? (kaloyan) arvera: review+ |
||||||||||
| Target Milestone: | 3.2 RC4 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Linux | ||||||||||||
| See Also: | https://git.eclipse.org/r/108962 | ||||||||||||
| Whiteboard: | PMC_approved | ||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 170451 [details]
Changes the default return to the empty string
The patch is that if server tools does not know what to do with this object (it's not a ModuleServer or an IServer or any of the other initializing objects), then it should return the empty string to give *other* label providers a chance to provide for this object.
The only danger I can see here, by looking at the code, is that a ModuleServer or IServer object which currently manages to slip through the label provider code and is labeled "-" will now most likely be labeled with a toString(), but since no ModuleServer or IServer objects should be returning "-" (which we would have seen by now and would be a severe bug), I see this as the correct fix which should be made as soon as possible.
1. Affiliation JBoss / Red Hat 2. Be clear on which release you want this bug to be fixed in, or "last workable" date, if requesting a patch. 3.2 Final, aka ASAP. 3. Justify why this is a hotbug. Note that "our users don't like the bug" is not the type of reason that gets much attention. The motivation we are looking for to justify a P1-like priority is, basically, "this bug blocks our adoption of WTP" (explaining why, of course). Anyone who provides extensions to this server view may (depending on plugin load order etc) have the servertools label provider take over functionality. This prohibits extensions from being properly displayed. Also, in the event that some ModuleServer or IServer object does not satisify the prior if statements, it's name will be returned as "-", which is not right at all. And this blocks adoption / use of the server's view. The patch looks good and is simple enough. Perhaps the return of a .toString in the object is more appropriate than a "-". I don't see anye problem with NPE or unexpected behaviour in this case (with the exception of the one already metnioend by Rob) Rob, I assumed you have tested this behaviour with and without adopters implementing label providers. Can you attach a screen shot of how it would look (after the patch) when there is no adopter that can respond to the label provider request? In other words, a screen shot of when the .toString is being used on a object. Created attachment 170623 [details]
Requested Screenshot
To be honest, I had not tested without any label provider because that would seem to me 100% to be an error on whoever programmed that extension. However attached is the screenshot in which a new plugin provides two CustomObjects under each Iserver, but has a label provider which returns nothing.
As shown in the screenshot, it turns out the behaviour is not a toString(), but rather the empty string. While I see this as unfortunate, I also see this as 100% an error on the coder of this plugin.
In order to experience this behaviour, a plugin provider would need to provide a navigatorContent in their plugin.xml and extension points. This navigatorContent would expressly outline the content provider and the label provider. The plugin provider would then have to have his content provider return some new unknown object (CustomObject), but then have his label provider expressly return null on that.
So I still think the 100% best thing to do is to apply the patch anyway. Perhaps we could also petition the CNF (common nav. framework) to return a toString in the event that nobody returns any value, but I still see this as a much less issue.
Leaving an object with no string in this fashion is clearly a programming error. But blocking label providers that have real data to return by having the server view's label provider override with a "-" is clearly a much bigger issue. In the first one (no label) the plugin provider can fix it by... ... ... returning a label ;) In the second, the plugin provider really has very little recourse that I've seen to ensure his label provider goes first.
To be a little bit more clear, the reason the adopter has very little recourse is because the Server View's navigatorContent is marked as follows:
priority="highest"
This means even if a plugin writer puts his also on highest, there is no guarantee that his "highest" will beat the server view's "highest". In fact, this may even be dependent on plugin load order (not sure at all) and could be non-deterministic.
Created attachment 170624 [details]
An example plugin project
Attached is an example plugin project, where the plugin writer returns CustomObjects but has his label provider ignore them.
Thanks for trying it out. We now know that it wouldn't call the toString as we thought it would. I agree with the fact that it would be a good enhancement for CNF view. Displaying an empty string in the UI is of no use and it is probably safer to do a toString on the object, that is outside of the scope of this defect. Your patch is simple and as with small risk. Thanks for the extra testing * Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such. This is a hotbug requested by an adopter. The bug prevents extensibility of the Servers view and thus it prevents the proper adoption and implementation the adopters function. * Is there a work-around? If so, why do you believe the work-around is insufficient? no workaround * How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? No junit test available for this area, manual testing of the patch was done by the adopter in several scenarios (with their extension point and without the extension point (refer to comment #4)) * Give a brief technical overview. Who has reviewed this fix? Angel Vera reviewed the changes. The change is to return an empty string in the case where the object is not resolved recognized by a label provider, so that other label providers can provide their implementation * What is the risk associated with this fix? Minimal, no compilation problems expected. No runtime problems expected. In the past returning a "-" when the object was not resolved by our label provider, but now we will be returning an empty string when objects are not resolved. In those cases adopter need to provide their own labels. Seems very important. Very safe. changes committed to HEAD changes released to HEAD New Gerrit change created: https://git.eclipse.org/r/108962 |
Created attachment 170450 [details] The broken view (Exclamation marks always make things seem more important). We have extensions inside the server view, and the server tools' label provider is overriding some of our extensions. This is because for some reason, the label provider returns "-" as a last resort. In many cases, this means the server tools' module / server label provider is returning a value for things like Filesets, Cluster Groups, and all sorts of other things it should not be aware of. The Common Navigator framework takes any response from a label provider to mean that this label provider is the proper one for this object. In this case, my label providers never even get asked for text because the server tools' provider responds first. This blocks adoption of WTP pretty drastically for us. We would need this fixed by 3.2 final. Attached is an example of what this looks like to our users.