Make some fields in the FlyoutPaletteComposite protected rather than …

[feilimb]

Make some fields in the FlyoutPaletteComposite protected rather than private, in order to allow for the possibility of a sub-class overriding the layout method(s) and accessing the various controls and states required.

Background

In our application, we wish to make use of (our own) version of the GraphicalEditorWithFlyoutPalette, which we achieve by copying the original implementation of that class (as is suggested in the class header block comment). This class in turn creates a FlyoutPaletteComposite via a method which can be over-ridden (GraphicalEditorWithFlyoutPalette.createPaletteComposite()).

We wish to create our own FlyoutPaletteComposite which is a sub-class of the stock FlyoutPaletteComposite, but make some modifications in the layout logic. Currently the code in the stock layout method contains references to internal class member variables which are private, but in order to implement our layout modifications - we need access to these class member variables from the parent.

This pull request simply makes some of those member variable protected to allow a subclass further control in a modified layout algorithm.

[ptziegler]

I have a hard time visualizing how those fields would be used in your application. But if exposing them helps you out, then that is good enough for me.

[feilimb]

I have a hard time visualizing how those fields would be used in your application. But if exposing them helps you out, then that is good enough for me.

:D thank you sir!

[azoitl]

Sorry I was ill and could not comment earlier. I must say I'm not very happy with this PR. Just making several core fields protected is normally not a good idea. I'm mostly concerned with paletteContainer, graphicalControl, and sash.

Can you better describe what you need to adjust. For the first two there are creation functions and setters. Would that be a better way. For all of them would a getter be better?

[feilimb]

In our application we wish to include another SWT component inside the FlyoutPaletteComposite, and as such we need to tweak the layout algorithm of FlyoutPaletteComposite to support our new component. Our additional component within, incidentally represents an 'overview ruler', and is at the location indicated via the blue arrow in this screenshot:

In order to achieve this, we have effectively had to copy/paste the entire implementation of FlyoutPaletteComposite to allow modification of the layout. Our implementation does not extend FlyoutPaletteComposite .

The idea behind this PR was to allow a new version of our implementation, which extends FlyoutPaletteComposite - and allows us to modify the layout algorithm and still have access to the UI components / some fields within the base class. This would be massively better than us having a 'copy/paste' implementation, which includes code which is meant to be non-API (ie. there are a bunch of import statements at top of class which have internal in their package names - and we would rather not have this situation).

If it is more agreeable to have getters, rather than protected member variables, maybe I could try to come up with an alternative in that regard - or if you have any other ideas to achieve what we are trying to do.

[azoitl]

I totally agree that a "copy/modify" approach is the worst. I'm only concerned of making to much protected. We had issues with that in the recent two years.

Especially for sash and paletteContainer there would be two factory methods: createSash() and createPaletteContainer() if these where protected so that you can overwrite them would this help you to implement your proposed solution?

PS: BTW I find it cool that you are doing a Ladder Diagram editor. If you would allow us I would like to collect GEF Classic usages like yours so that people have a reference.
PPS: we definitely don't want you to disclose any confidential information. I just want to get to a solution where you can solve your problem and keeps GEF Classic maintainable and extensible.

[feilimb]

I think in order to modify the layout for our implementation we essentially need everything referenced within [FlyoutPaletteComposite.layout(boolean)], to allow the 'insertion' of the additional overview ruler component which we add, but continue to use the stock layout algorithm - but with some tweaks to add in our component.

This essentially means we need access to the fields which I made protected in the first version of this PR, and not just the sash, paletteContainer and graphicalControl, also eg. things like 'paletteWidth' and 'dock' etc.

If the createPaletteContainer(), createSash() methods were protected (rather than private), then I guess for those fields I could get hold of a reference to them, via an override of those methods. This would still leave some other fields as mentioned above however, it's a bit tricky to determine the best course..

[ptziegler]

I apologize for the rushed approval and in hindsight I have to agree with @azoitl. A lot of those fields that are have become accessible are just asking for trouble.

The worst offender, in my opinion being the mutable dock field. This would allow subclasses to change the position of the palette to NORTH or SOUTH, which is just not supported. So I'm tempted to revert this commit before the M3 so that this doesn't come near downstream projects, let alone the next release.

Given that I now have a rough idea of what you want to accomplish, I would propose an approach similar to the Eclipse MessageDialog, where subclasses can contribute their own classes via a protected createCustomArea() method. This could then be considered during the layout phase to place it next to the sash.

[ptziegler]

I've created a proof-of-concept via #692, which creates a green "bar" right next to the sash. Is this similar to what you want to implement? The only thing a subclass would need to do is to override createCustomArea(), return their own "bar" and then the layout method would do the rest.

[feilimb]

@ptziegler that looks fantastic, I'm busy this weekend but on Monday morning I can try it on our side as a test implementation and see if it fits the bill. Thanks again I will report back here on Monday

[azoitl]

@feilimb sorry that I didn't got your initial idea. I would definitely not implement the ruler as part of the palette. The palette can be detached and can also be opened as own view. The the ruler would not be where it should be or?

[feilimb]

@feilimb sorry that I didn't got your initial idea. I would definitely not implement the ruler as part of the palette. The palette can be detached and can also be opened as own view. The the ruler would not be where it should be or?

I think you probably hit the nail on the head :) Note: I tried out Patrick's PR #692 and added a comment there just this morning.

I guess with our current bespoke implementation we are 'piggy-backing' on the palette composite to get an overview ruler to appear, but I am starting to think now that perhaps the ability to create a custom area (ie. overview ruler) which visually appears between the editor area and the palette, could perhaps come from the class above - which creates the palette composite, ie 'GraphicalEditorWithFlyoutPalette' or something along those lines.