Manhattan routing improvement#376
Conversation
gabrieldulca
force-pushed
the
routing-improvement
branch
3 times, most recently
from
6a84c43 to
2817d55
Compare
There was a problem hiding this comment.
Thanks for this great improvement.
I have a couple of comments but most of them are non-critical and related to linting and codestyle.
In general please avoid unnecessary empty lines and double check if all newly created files have a copyright header.
I think implementing this feature directly in the change-bounds tool can lead to a couple of issues especially if a routing style other than Manhattan is used.
I suggest to refactor the auto-rerouting behavior into a separate class which is optionally injected into the change bounds tool. This way the feature is only activated if explicitly bound by a client implementation.
|
|
||
| import { ElementAndRoutingPoints } from "../tools/change-bounds-tool"; | ||
|
|
||
| export class SaveModelEdgesAction implements Action { |
There was a problem hiding this comment.
Please rename to ChangeRoutingPointsAction.
| constructor(public newRoutingPoints: ElementAndRoutingPoints[]) { } | ||
| } | ||
|
|
||
| export class SaveModelKeyboardListener extends KeyListener { |
There was a problem hiding this comment.
Do we really need this KeyListener? CTRL + s triggers a SaveModelAction which persists the entire model state anyways
| public static final String LAYOUT = "layout"; | ||
| public static final String VALIDATE_LABEL_EDIT_ACTION = "validateLabelEdit"; | ||
| public static final String SET_LABEL_EDIT_VALIDATION_RESULT_ACTION = "setLabelEditValidationResult"; | ||
| public static final String SAVE_MODEL_EDGES = "saveModelEdges"; |
There was a problem hiding this comment.
Rename to CHANGE_ROUTING_POINTS
| import com.eclipsesource.glsp.api.action.Action; | ||
| import com.eclipsesource.glsp.api.model.ElementAndRoutingPoints; | ||
|
|
||
| public class SaveModelEdgesAction extends Action{ |
There was a problem hiding this comment.
Rename to ChangeRoutingPointsAction
| return "SaveModelEdgesAction [newRoutingPoints=" + newRoutingPoints + "]"; | ||
| } | ||
|
|
||
|
|
| EList<GPoint> routingPoints = edge.getRoutingPoints(); | ||
| routingPoints.clear(); | ||
| routingPoints.addAll(elementAndRoutingPoints.getRoutingPoints()); | ||
|
|
| } | ||
|
|
||
| private void addDefaultActions() { | ||
| defaultActions.add(new ApplyLabelEditOperationAction()); |
There was a problem hiding this comment.
Seems like this was accidentally removed during merging. Please add again.
| defaultActions.add(new SetModelAction()); | ||
| defaultActions.add(new SetOperationsAction()); | ||
| defaultActions.add(new SetPopupModelAction()); | ||
| defaultActions.add(new SetEditLabelValidationResultAction()); |
There was a problem hiding this comment.
Seems like this was accidentally removed during merging. Please add again.
| defaultActions.add(new ReconnectConnectionOperationAction()); | ||
| defaultActions.add(new RerouteConnectionOperationAction()); | ||
| defaultActions.add(new LayoutAction()); | ||
| defaultActions.add(new ValidateLabelEditAction()); |
There was a problem hiding this comment.
Seems like this was accidentally removed during merging. Please add again.
| // Spacing unit used for rerouting in case of collision | ||
| private SPACING: number = 5; | ||
|
|
||
| constructor(protected tool: ChangeBoundsTool, protected edgeRouterRegistry: EdgeRouterRegistry, protected manhattanRouter: ManhattanEdgeRouter) { |
There was a problem hiding this comment.
Is the manhattanRouter field even used?
If so: Please remove and retrieve the corresponding router for each edge from the registry individually (since the routing type can differ from edge to edge)
gabrieldulca
force-pushed
the
routing-improvement
branch
from
07008a3 to
65d1c32
Compare
tortmayr
force-pushed
the
routing-improvement
branch
from
65d1c32 to
1fe16a9
Compare
2 participants
Manhattan routing has been improved so that edges don't go through elements anymore.
However if 2 elements are too close to each other the edge will be routed through one of them. An issue will be opened for this.