Bug 453251

Comment 1 William Collins 2014-11-25 18:13:05 EST

Created attachment 248932 [details]
Proposed patch

Comment 2 Alexander Nyßen 2014-11-27 08:11:12 EST

(In reply to William Collins from comment #1)
> Created attachment 248932 [details]
> Proposed patch

I infer that the core of your problem is that there is no hook method to create the flyout composite. This could be added for 3.10 (because its an API extension). However, I see not benefit in creating a FlyoutPaletteCompositeBase. Instead, you can simply overwrite FlyoutPaletteComposite if you want to create your own.

Comment 3 William Collins 2014-11-27 10:11:28 EST

Hi Alexander,

Thank you for your response. 

Yes I primarily wish to introduce a hook method to provide a custom implementation of the flyout composite. 

I introduced the FlyoutPaletteCompositeBase to factor out the methods of FlyoutPaletteComposite that the GraphicalEditorWithFlyoutPalette specifically depends upon and to provide any classes extending FlyoutPaletteCompositeBase common access to the externalViewer and graphicalControl fields. 

You are absolutely right however in pointing out that we could simply derive our custom palette composite class from FlyoutPaletteComposite and override and replace every field and method in that class. We pretty much have to do that anyway with our proposed patch. I just thought this might be a nice interim step in providing a more extensible architecture for the palette but I can live without it :-)

Best Regards
William Collins

Comment 4 William Collins 2014-12-02 14:28:37 EST

Created attachment 249112 [details]
Updated proposed patch

Comment 5 Alexander Nyßen 2014-12-03 11:21:37 EST

I could not consume that patch, but as the proposed fix was simply the introduction of a factory method I coped without. That is, I now added a method that is responsible for the creation of the flyout palette composite, so clients can overwrite it. 

Pushed all changes to origin/master. Resolving as fixed in 3.10.0M4.

Comment 6 Lars Heinemann 2014-12-10 06:22:43 EST

Alexander,

is there any chance to get that into Luna SR2 as well?

Thanks,
Lars

Comment 7 Alexander Nyßen 2014-12-10 07:25:29 EST

(In reply to Lars Heinemann from comment #6)
> Alexander,
> 
> is there any chance to get that into Luna SR2 as well?
> 
> Thanks,
> Lars

Unfortunately not, as it includes an API extension, which cannot be included in a service release.

Comment 8 Rob Stryker 2014-12-10 08:35:03 EST

> Unfortunately not, as it includes an API extension, which cannot be included
> in a service release.

Can you please point to some documentation on this? I've seen countless eclipse projects add a protected method in a service release without issue. 

Eclipse's guidelines on evolving APIs (https://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_Classes) indicates that adding a protected method that does not need to be implemented by a subclass is Binary compatible in all cases except the unlikely case where an already-existing subclass has already declared a method with the same signature. 

This enhancement is also being requested by an adopter project who needs it.

Comment 9 Alexander Nyßen 2014-12-10 11:54:11 EST

(In reply to Rob Stryker from comment #8)
> > Unfortunately not, as it includes an API extension, which cannot be included
> > in a service release.
> 
> Can you please point to some documentation on this? I've seen countless
> eclipse projects add a protected method in a service release without issue. 
> 
> Eclipse's guidelines on evolving APIs
> (https://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_Classes)
> indicates that adding a protected method that does not need to be
> implemented by a subclass is Binary compatible in all cases except the
> unlikely case where an already-existing subclass has already declared a
> method with the same signature. 
> 
> This enhancement is also being requested by an adopter project who needs it.

Adding a method is not a breaking API change, and its even binary compatible, your are right. But its still an API change, and it would enforce us to do a minor rather than a service release for Luna SR2 (following the version numbering guidelines http://wiki.eclipse.org/Version_Numbering). 

While in principle we would have the option to promote a minor instead of a service release with Luna SR2 as long as we announce it before RC1 (see https://wiki.eclipse.org/SimRel/Simultaneous_Release_Policy_FAQ), this is generally not encouraged, and it would have a couple of implications: 

1) we would have to schedule a release review for Luna SR2 (see: http://wiki.eclipse.org/Development_Resources/HOWTO/Release_Reviews) which could be skipped in case of a service release
2) we would have to adjust our Mars release plan and would have to increment all Mars versions from 3.10 to 3.11 to indicate we have another development stream there.

In the overall, this effort does not seem to be justified for this change. Especially, as the GraphicalEditorWithFlyoutPalette is not really a core part of the API: 

"This class serves as a quick starting point for clients who are new to GEF. It will create an Editor with a flyout palette. The flyout palette will only be visible when the palette view is not open. IMPORTANT This class should only be used as a reference for creating your own EditorPart implementation. This class will not suit everyone's needs, and may change in the future. Clients may copy the implementation."

Comment 10 Rob Stryker 2014-12-10 12:40:53 EST

The document reads as follows:

The minor segment number must be incremented when a plug-in changes in an "externally visible" way. Examples of externally visible changes include binary compatible API changes, significant performance changes, major code rework, etc. 

While technically adding one method that is protected is a "binary compatible API change", I do not think it "forces" you to increment the minor segment. You always have the option to use discretion and not increment it. 

The other two items mentioned in that paragraph are "significant performance changes" and "major code rework".  I think the addition of one protected method does not rise to this level. 

You also have the option, in SR2, to mark the method as internal-use only and not API (use at your own risk). If the API Tools are preventing you from this, you also have the option to add an API exclusion marker so that API Tools doesn't complain during your build reports. 

I agree with you that bumping SR2 by a minor would be more trouble than it's worth, especially with the hassle of also having to bump master. I do not endorse that action. 

I do, however, endorse an exception being filed with your API Tools to exclude this change,  marking the new method as internal-use only (at your own risk), continue along with the service release path, and find a way to assist your adopter products when it isn't dangerous or misleading to other clients to do so. 

Remember, these guidelines are meant to assist framework providers and their users, but they are not meant to be followed blindly. If you had multiple API changes that added up to a significant amount, I would agree that a bump of minor would be in order.   But I believe an exception can be made in this case without creating any risk to other clients   I've seen it done in other eclipse projects, and the patch here is very low risk.

Comment 11 Alexander Nyßen 2014-12-11 02:54:10 EST

> The minor segment number must be incremented when a plug-in changes in an
> "externally visible" way. Examples of externally visible changes include
> binary compatible API changes, significant performance changes, major code
> rework, etc. 

Adding a protected API method is an externally visible change. And, as quoted by yourself, the minor segment must be incremented in such a case. I am aware of technical ways to bypass the API tools, but I don't won't to apply them, as GEF adopters have always been able to rely on the fact that GEF does not violate API compatibility constraints. And I want to preserve this policy (which is also the reason why GEF4 is yet released only with provisional API).

Did you consider releasing a GEF-patch for Luna SR2 with your own code base? That way, you could rely on the added API (which will be included in Mars) without imposing a risk on other adopters.