Description Andrew Gvozdev 2013-10-03 17:55:23 EDT

AbstractCPropertyTab.canSupportMultiCfg() was introduced in bug 406545. It returns "true" to maintain compatibility with existing implementations. However, it would be more natural to default to single configuration mode.

There was a little bit of discussion about the default of this method in Gerrit https://git.eclipse.org/r/#/c/16770/4 which I cite and continue discussion here.

/**
 * Method to be implemented by inherited classes to indicate whether or not the tab supports displaying
 * and editing settings for multiple configuration selection.
 * 
 * Default implementation returns true.
 * 
 * @return true if the tab supports multiple configurations, false otherwise
 * @since 5.7
 */
public boolean canSupportMultiCfg() {
	return true;
}
>> AG> I think "false" could be better default here in the long run. When developers start developing a new tab they are more likely to develop/test a single configuration version first. Most will stop right there and not even realize that it needs to be tested in multi-cfg mode. Exceptions ensue. Would you agree? What is your reasoning making "true" the default?
> SB> My point is that having this return 'false' by default would cause unchanged code who inherit that class to have multiple configuration unexpectedly disabled.
> Basically, you might be right that retuning 'false' would be a more natural choice, but it would break computability with existing code, so this is why I think that it should return 'true', since it is the behavior that was implied by the previous API.
This is a valid point. I think there are advantages and disadvantages doing that. I see some arguments that would mitigate compatibility issue:
- The impact will be small as this functionality is not often used by average user and this impacts only third party implementations which are not many. Also, the implementations were discouraged as ICPropertyProvider (which AbstractPage used to implement) is marked as @noextend @noimplement.
- Where it is used it will be clearly visible (contrary to the other way around).
- It is trivial to fix downstream.
- This is a new release and additions/refinements of API are expected. This is not really an API change but we can put it in API change list.

In general I lean to be in favor of making the change for this function to default to "false". Any more arguments?