Community
Participate
Working Groups
In DefaultMoveShapeFeature the method moveShape is final. Does this really make sense, especially because internalMove can be overridden?
Would like to target for Juno
final public void moveShape(IMoveShapeContext context) { preMoveShape(context); moveAllBendpoints(context); internalMove(context); postMoveShape(context); } The inner method calls can be overriden. If one does not want to use this structure it does not make sense to use the default feature at all. Then one can inherit directly from AbstractMoveShapeFeature and define moveShape completely. Making this method final blocks misuse of the class. I would keep it as is.
(In reply to comment #2) > final public void moveShape(IMoveShapeContext context) { > preMoveShape(context); > moveAllBendpoints(context); > internalMove(context); > postMoveShape(context); > } > The inner method calls can be overriden. If one does not want to use this > structure > it does not make sense to use the default feature at all. Then one can inherit > directly from AbstractMoveShapeFeature and define moveShape completely. > Making this method final blocks misuse of the class. I would keep it as is. But there is much more functionality in DefaultMoveShapeFeature that in AbstractMoveShapeFeature and parts of it might be usefull while you would still want to add additional functionality for which you would need to override moveShape (this was the scenario I saw). What kind of misuse do you see?
You can tweak the default move via overriding preMoveShape/postMoveShape/moveAllBendpoints/internalMove. So the sequence of operations is enforced by the class but still you can override any suboperation. If you would allow overriding moveShape as well, the user can forget to move the bendpoints for example etc. So the class is perfectly extensible (all four methods can be overridden) but also makes it hard to forget to move the bendpoints or do it only after the shape is moved etc
Ok, agreed that this gives a kind of guidance. But this emerged drom a scenario where a not final moveShape method would have been an easy solution (I had to think a while to recall): The requirement was to do a check if the move was allowed; the check is expensive (in this case a check if a file has been checked-out). You would not want to do this check in the general pre-execution hook and also not in the canMoveShape method because they are called very often (each time the user moves the mouse). It is also not possible to do this in the preMoveShape, because there you cannot cancel. The currently used solution (workaround?) was to override moveAllBendpoints, do the check there and throw an exception in case of the condition not beeing fulfilled to end processing the feature further. In case of the condition being true, a super call is done. A nicer solution would be to override moveShape, do the check there and only do the super call in case the condition is fulfilled. Another solution I can think of would be the introduction of a return value in preMoveShape with a cancel value and moveShape checking that. Open to other ideas as well...
fixed. simply removed the final modifier.
Corrected theme and verified change
Bookkeeping: Set target release
Part of Graphiti 0.9.0 (Eclipse Juno)
