Description saisaket potluri 2017-11-15 17:28:04 EST

Hello, 

While developing a custom error support provider, I noticed that the validFor(StatusAdapter) method is not being called at all. We noticed this when the error supporter's createSupportArea was being called even though the validFor we provided returned false. 

The ErrorDialog creates the actual error support area. It uses the validFor(Status) instead of validFor(StatusAdapter). Going up the class hierarchy, it turns out the parent validFor(StatusAdapter) is being called and always returning true, explaining why createSupportArea is being called when it shouldn't. 

This is a confusing interface for the developer, providing an extension point (i.e. validFor(StatusAdapter)) and not calling it all. Currently, there are two validFor's that are provided and this is bad design since they both do the same logic but with different parameters. Indeed, AbstractStatusAreaProvider should have two overloaded methods like this, similar to what it does with createSupportArea:
 
                // The one we override.
                abstract public boolean validFor(StatusAdapter statusAdapter);
               
                @Override
                public boolean validFor(Status status) {
                      return validFor(new StatusAdapter(status));
                }

I consider better design, since StatusAdapter might contain extra information, but this might not be directly backwards compatible because since users will have to override the abstract validFor(StatusAdapter) which is not being used in the ErrorDialog code. If you want it to be backwards compatible, you might have to switch which method is abstract, since validFor(Status) is being used in ErrorDialog. 

 
I am going off this based on the code in the github repo https://github.com/eclipse/eclipse.platform.ui/.