Bugzilla – Full Text Bug Listing
| Summary: | Why is the method moveShape final? | ||
|---|---|---|---|
| Product: | [Modeling] Graphiti | Reporter: | Michael Wenz <michael.wenz> |
| Component: | Core | Assignee: | Project Inbox <graphiti-inbox> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | matthias.gorning |
| Version: | 0.8.0 | Flags: | michael.wenz:
juno+
|
| Target Milestone: | 0.9.0 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | Juno M2 theme_round_offs | ||
|
Description
Michael Wenz
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) |