Skip to content

Commit

Permalink
Delete nested connections instead of hiding them
Browse files Browse the repository at this point in the history 
* This solves the issue of trying to hide connections to connections that are also hidden
  • Loading branch information
@Phillipus
Phillipus committed Nov 22, 2016
1 parent 4549c6e commit c22f4cd
Show file tree
Hide file tree
Showing 10 changed files with 120 additions and 437 deletions.
162 changes: 13 additions & 149 deletions ...himatetool/editor/diagram/commands/CreateNestedArchimateConnectionsWithDialogCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,21 @@
import com.archimatetool.editor.diagram.dialog.NewNestedRelationDialog;
import com.archimatetool.editor.diagram.dialog.NewNestedRelationsDialog;
import com.archimatetool.editor.diagram.dialog.NewNestedRelationsDialog.SelectedRelationshipType;
import com.archimatetool.editor.model.DiagramModelUtils;
import com.archimatetool.editor.preferences.ConnectionPreferences;
import com.archimatetool.model.IArchimateElement;
import com.archimatetool.model.IArchimateFactory;
import com.archimatetool.model.IArchimateRelationship;
import com.archimatetool.model.IDiagramModelArchimateComponent;
import com.archimatetool.model.IDiagramModelArchimateConnection;
import com.archimatetool.model.IDiagramModelArchimateObject;
import com.archimatetool.model.IDiagramModelConnection;
import com.archimatetool.model.IDiagramModelObject;
import com.archimatetool.model.IFolder;
import com.archimatetool.model.util.ArchimateModelUtils;

/**
* Compound Command to create new connections/relations between parent and childObjects in nested objects.
* This is used when the user drags objects into a parent Archimate object and nested connections/relations to child are required.
* Compound Command to create new relations between parent and childObjects in nested objects.
* This is used when the user drags objects into a parent Archimate object and nested relations to child are required.
*
* Cases are:
* 1. User drags newly created child object(s) into the parent where there is _no_ relationship.
* Ask via a dialog what user wants. If yes, create relationship & connection.
* Ask via a dialog what user wants. If yes, create relationship.
* This is when creating new child objects from palette or from DnD from model tree.
* 2. User drags child object(s) into parent where there is a relationship - check if there is a connection, if not make one.
* This is when user drags child objects on diagram or from tree into parent and a relationship exists but no connection.
* 3. Cases 1 & 2 can happen together if more than one child is dragged into parent.
*
* @author Phillip Beauvoir
*/
Expand All @@ -61,11 +52,8 @@ public CreateNestedArchimateConnectionsWithDialogCommand(IDiagramModelArchimateO

@Override
public void execute() {
// Add new relationships/connection if user wants them
createConnectionDialogCommands();

// Add missing connections (may have been deleted by user)
createNewConnectionCommands();
// Add new relationships if user wants them
createRelationshipDialogCommands();

super.execute();
}
Expand All @@ -90,7 +78,7 @@ public boolean canRedo() {
/**
* Child Objects that don't have a relationship set with parent - ask user if they want one
*/
void createConnectionDialogCommands() {
void createRelationshipDialogCommands() {
// Gather suitable child objects
List<IDiagramModelArchimateObject> childObjectsForDialog = new ArrayList<IDiagramModelArchimateObject>();
for(IDiagramModelArchimateObject childObject : fChildObjects) {
Expand All @@ -106,7 +94,8 @@ void createConnectionDialogCommands() {
if(dialog.open() == Window.OK) {
EClass eClass = dialog.getSelectedType();
if(eClass != null) {
Command cmd = new CreateRelationshioAndDiagramArchimateConnectionCommand(fParentObject, childObjectsForDialog.get(0), eClass);
Command cmd = new AddRelationshipCommand(fParentObject.getArchimateElement(),
childObjectsForDialog.get(0).getArchimateElement(), eClass);
add(cmd);
}
}
Expand All @@ -120,7 +109,8 @@ else if(childObjectsForDialog.size() > 1) {
List<SelectedRelationshipType> selectedTypes = dialog.getSelectedTypes();

for(SelectedRelationshipType selType : selectedTypes) {
Command cmd = new CreateRelationshioAndDiagramArchimateConnectionCommand(fParentObject, selType.childObject, selType.relationshipType);
Command cmd = new AddRelationshipCommand(fParentObject.getArchimateElement(),
selType.childObject.getArchimateElement(), selType.relationshipType);
add(cmd);
}
}
Expand Down Expand Up @@ -154,102 +144,17 @@ boolean canAddNewRelationship(IDiagramModelArchimateObject parentObject, IDiagra

return false;
}

/**
* Create Commands for child objects that don't have connections and need new ones
*
* TODO A3: If O1--C1--O2. C1 is also connected to parent.
* O1 or O2 is added to parent - should add connection from C1 to parent?
* Or should it be only when O1 AND O2 are added to parent?
*
*/
void createNewConnectionCommands() {
IArchimateElement parentElement = fParentObject.getArchimateElement();

// Check connections between parent and child objects that are being dragged in
for(IDiagramModelArchimateObject childObject : fChildObjects) {
IArchimateElement childElement = childObject.getArchimateElement();

for(IArchimateRelationship relation : parentElement.getSourceRelationships()) {
if(relation.getTarget() == childElement && DiagramModelUtils.isNestedConnectionTypeRelationship(relation)) {
// And there's not one already there...
if(!DiagramModelUtils.hasDiagramModelArchimateConnection(fParentObject, childObject, relation)) {
add(new CreateDiagramArchimateConnectionCommand(fParentObject, childObject, relation));
}
}
}
}

// Check connections between parent and child connections of objects
for(IArchimateRelationship relation : parentElement.getSourceRelationships()) {
if(relation.getTarget() instanceof IArchimateRelationship) {
IDiagramModelArchimateConnection dmc = findConnection((IArchimateRelationship)relation.getTarget());
if(dmc != null) {
if(!DiagramModelUtils.hasDiagramModelArchimateConnection(fParentObject, dmc, relation)) {
add(new CreateDiagramArchimateConnectionCommand(fParentObject, dmc, relation));
}
}
}
}

for(IArchimateRelationship relation : parentElement.getTargetRelationships()) {
if(relation.getSource() instanceof IArchimateRelationship) {
IDiagramModelArchimateConnection dmc = findConnection((IArchimateRelationship)relation.getSource());
if(dmc != null) {
if(!DiagramModelUtils.hasDiagramModelArchimateConnection(fParentObject, dmc, relation)) {
add(new CreateDiagramArchimateConnectionCommand(fParentObject, dmc, relation));
}
}
}
}
}

private IDiagramModelArchimateConnection findConnection(IArchimateRelationship relation) {
for(IDiagramModelObject dmo : fParentObject.getChildren()) {
for(IDiagramModelConnection dmc : dmo.getSourceConnections()) {
if(dmc instanceof IDiagramModelArchimateConnection) {
if(((IDiagramModelArchimateConnection)dmc).getArchimateRelationship() == relation) {
return (IDiagramModelArchimateConnection)dmc;
}
}
}
for(IDiagramModelConnection dmc : dmo.getTargetConnections()) {
if(dmc instanceof IDiagramModelArchimateConnection) {
if(((IDiagramModelArchimateConnection)dmc).getArchimateRelationship() == relation) {
return (IDiagramModelArchimateConnection)dmc;
}
}
}
}

return null;
}

static class CreateRelationshioAndDiagramArchimateConnectionCommand extends CompoundCommand {

CreateRelationshioAndDiagramArchimateConnectionCommand(IDiagramModelArchimateObject sourceObject,
IDiagramModelArchimateObject targetObject, EClass relationshipType) {

IArchimateRelationship relationship = (IArchimateRelationship)IArchimateFactory.eINSTANCE.create(relationshipType);

Command cmd = new AddRelationshipCommand(sourceObject.getArchimateElement(), targetObject.getArchimateElement(), relationship);
add(cmd);

cmd = new CreateDiagramArchimateConnectionCommand(sourceObject, targetObject, relationship);
add(cmd);
}
}


static class AddRelationshipCommand extends Command {
private IArchimateElement source;
private IArchimateElement target;
private IArchimateRelationship relationship;
private IFolder folder;

public AddRelationshipCommand(IArchimateElement source, IArchimateElement target, IArchimateRelationship relationship) {
public AddRelationshipCommand(IArchimateElement source, IArchimateElement target, EClass relationshipType) {
this.source = source;
this.target = target;
this.relationship = relationship;
this.relationship = (IArchimateRelationship)IArchimateFactory.eINSTANCE.create(relationshipType);
}

@Override
Expand Down Expand Up @@ -279,45 +184,4 @@ public void dispose() {
relationship = null;
}
}

/**
* Create New Connection Command based on existing relation
*/
static class CreateDiagramArchimateConnectionCommand extends Command {
IDiagramModelArchimateConnection connection;
IDiagramModelArchimateComponent source;
IDiagramModelArchimateComponent target;
IArchimateRelationship relationship;

CreateDiagramArchimateConnectionCommand(IDiagramModelArchimateComponent source, IDiagramModelArchimateComponent target, IArchimateRelationship relationship) {
this.source = source;
this.target = target;
this.relationship = relationship;
}

@Override
public void execute() {
connection = IArchimateFactory.eINSTANCE.createDiagramModelArchimateConnection();
connection.setArchimateRelationship(relationship);
connection.connect(source, target);
}

@Override
public void redo() {
connection.reconnect();
}

@Override
public void undo() {
connection.disconnect();
}

@Override
public void dispose() {
connection = null;
source = null;
target = null;
relationship = null;
}
}
}
77 changes: 77 additions & 0 deletions ....editor/src/com/archimatetool/editor/diagram/commands/DeleteNestedConnectionsCommand.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/**
* This program and the accompanying materials
* are made available under the terms of the License
* which accompanies this distribution in the file LICENSE.txt
*/
package com.archimatetool.editor.diagram.commands;

import java.util.ArrayList;
import java.util.List;

import org.eclipse.gef.commands.Command;
import org.eclipse.gef.commands.CompoundCommand;

import com.archimatetool.model.IDiagramModelArchimateObject;
import com.archimatetool.model.IDiagramModelConnection;

/**
* Compound Command to delete nested connections between parent and childObjects in nested objects.
* This is used when the user drags objects into a parent Archimate object and nested connections to child are removed.
*
* @author Phillip Beauvoir
*/
public class DeleteNestedConnectionsCommand extends CompoundCommand {

IDiagramModelArchimateObject fParentObject;
List<IDiagramModelArchimateObject> fChildObjects;

public DeleteNestedConnectionsCommand(IDiagramModelArchimateObject parentObject, IDiagramModelArchimateObject childObject) {
fParentObject = parentObject;
fChildObjects = new ArrayList<IDiagramModelArchimateObject>();
fChildObjects.add(childObject);
}

public DeleteNestedConnectionsCommand(IDiagramModelArchimateObject parentObject, List<IDiagramModelArchimateObject> childObjects) {
fParentObject = parentObject;
fChildObjects = childObjects;
}

@Override
public void execute() {
createDeleteCommands();

super.execute();
}

// These should return true always because sub-commands are only created on execute()

@Override
public boolean canExecute() {
return true;
}

@Override
public boolean canUndo() {
return true;
}

@Override
public boolean canRedo() {
return true;
}

/**
* Child Objects that have connections
*/
void createDeleteCommands() {
for(IDiagramModelArchimateObject child : fChildObjects) {
for(IDiagramModelConnection connection : child.getTargetConnections()) {
if(connection.getSource() == fParentObject) {
Command cmd = DiagramCommandFactory.createDeleteDiagramConnectionCommand(connection);
add(cmd);
}
}
}
}

}
47 changes: 2 additions & 45 deletions ...tool.editor/src/com/archimatetool/editor/diagram/editparts/AbstractConnectedEditPart.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*/
package com.archimatetool.editor.diagram.editparts;

import java.util.ArrayList;
import java.util.List;

import org.eclipse.draw2d.ConnectionAnchor;
Expand Down Expand Up @@ -113,12 +112,12 @@ protected Adapter getECoreAdapter() {

@Override
protected List<IDiagramModelConnection> getModelSourceConnections() {
return getFilteredModelSourceConnections();
return getModel().getSourceConnections();
}

@Override
protected List<IDiagramModelConnection> getModelTargetConnections() {
return getFilteredModelTargetConnections();
return getModel().getTargetConnections();
}

public ConnectionAnchor getSourceConnectionAnchor(ConnectionEditPart connection) {
Expand Down Expand Up @@ -179,46 +178,4 @@ protected void refreshConnectionAnchors() {
((EditPart)editPart).refresh();
}
}

// =================================== Filtering ====================================================

public List<IDiagramModelConnection> getFilteredModelSourceConnections() {
return getFilteredConnections(getModel().getSourceConnections());
}

public List<IDiagramModelConnection> getFilteredModelTargetConnections() {
return getFilteredConnections(getModel().getTargetConnections());
}

/**
* See if any connections are filtered out
* @param originalList
* @return A list of filtered connections
*/
private List<IDiagramModelConnection> getFilteredConnections(List<IDiagramModelConnection> originalList) {
IConnectionEditPartFilter[] filters = getRootEditPartFilterProvider().getEditPartFilters(IConnectionEditPartFilter.class);
if(filters != null) {
List<IDiagramModelConnection> filteredList = new ArrayList<IDiagramModelConnection>();

for(IDiagramModelConnection connection : originalList) {
boolean add = true;

for(IConnectionEditPartFilter filter : filters) {
add = filter.isConnectionVisible(this, connection);

if(!add) { // no point in trying the next filter
break;
}
}

if(add) {
filteredList.add(connection);
}
}

return filteredList;
}

return originalList;
}
}
2 changes: 0 additions & 2 deletions ...imatetool.editor/src/com/archimatetool/editor/diagram/editparts/ArchimateDiagramPart.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@
public class ArchimateDiagramPart extends AbstractDiagramPart {

public ArchimateDiagramPart() {
// Add a Nested Connection Filter
addEditPartFilter(new NestedConnectionEditPartFilter());
}

@Override
Expand Down
Loading

18 comments on commit c22f4cd

@jbsarrodie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a issue with this commit. This breaks the behavior putted in place to solve Issue 26: Relation not copied if nested objects copied and pasted. We have to look at this.

@Phillipus
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I don't know what to do then.

@jbsarrodie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking at it. Maybe I could revert and manage the hidding of connections to connections... (or add all model relations between nested elements to the CopySnapshot?)

@Phillipus
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to review CopySnapshot, I think. Hiding connections is a bad idea, for various reasons. If a connection is not there, then it should be deleted.

@jbsarrodie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, the more I think about it, the more I think hiding connections is a good idea. I played a bit with the latest version and it is rather frequent (at least for me) to try different layouts for my views. This leads me to nest and un-nest things quite often. The issue with latest behavior is that I no more have the same connections when un-nesting because I loose connections to connections, and other connections appear (because they are in the model, but were not in my view). So I think I'll try to revert this commit and solve original issue.

@Phillipus
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is when the model connections update occurs - getModelConnections methods. When moving one box, it only updates for that box, not the connecting boxes and connections.

@jbsarrodie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is when the model connections update occurs - getModelConnections methods. When moving one box, it only updates for that box, not the connecting boxes and connections.

I don't get you ?

@Phillipus
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AbstractConnectedEditPart#getModelSourceConnections() AbstractConnectedEditPart#getModelTargetConnections()
ArchimateRelationshipEditPart#getModelSourceConnections()
ArchimateRelationshipEditPart#getModelTargetConnections()

These are only called for the box when it is moved and nested. So only that box's connections are re-created and re-drawn. These methods are not called for any associated connections or boxes, so there are hanging connections.

@jbsarrodie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I understand now (I tested some code change yesterday and was blocked by this).

I'm now thinking about doing the filtering in a way similar to Viewpoints: create a NestedConnectionEditPartFilter that would implement IConnectionEditPartFilter and hide a connection based on the hiden/shown "state" of its source/target (through some calls to DiagramModelUtils.shouldBeHiddenConnection())...

What do you think about it ?

@Phillipus
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that some actions - move a child into/out of parent, delete box or connection, add new connection, box, etc would then require to trigger an update of more than one EditPart. Most of these actions only trigger one update for the one EditPart concerned. Propagating all of the necessary updates to several EdiParts manually could be complicated, and expensive. The reason it works in the VP filter is that changing the VP then reloads the whole GEF model.

@jbsarrodie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so I might be trying to solve two different things which requires two different approaches:

  • A connection to another connection that is being hiden/removed because of nesting: we should delete it as it makes no sens to hide it. In addition, this is very unlikely to happen in real life because this would mean nothing in ArchiMate.
  • A connection that is being "converted" to a nesting: I think we should keep it and hide it, because this is the semantic explanation of the nesting. Deleting this connection and create new ones (potentially more) when unesting doesn't allow to keep/understand the meaning behind.

So my conclusion is that: when nesting we should delete all potential connections to connections, but hide the connection which is used as a base (meaning) for nesting.

@jbsarrodie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

I've implemented my proposed behavior in the new branch 'hide-nested-connections'.

While working on it I've noticed some strange behavior (that existed before, even in Archi 3.x): when nesting A into B while a connection C already exists between them, all existing relationship (and not only the one related to C) are converted to hidden connections, which doesn't make sens... I'm working on a fix.

@jbsarrodie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just discovered another good reason to create hidden connections when nesting: if we don't do so, then the Validator will list (most of) the relationships as not used in views, leading user (like me 2min ago) to think he can delete them... to discover just after doing so that now there's nesting without proper relationship...

@Phillipus
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that can be solved by changing logic of calculating an "implicit" relationship. I still think that hiding connections may lead to unforeseen problems. I think of a connection as a visual thing, not a semantic thing.

@jbsarrodie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think of a connection as a visual thing, not a semantic thing.

IMHO when nesting, I really think it's good to know which relationship was nested. Hidding connection is (at least for the moment) the best way to do it. That's why my proposal is to keep hidden connections in this case (but only this one: connections to connections have to be removed when nesting because there's no semantic link to the nesting itself).

@Phillipus
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used to have a method that determined a semantic "implicit" connection by (1) Is A a graphical child of B? (2) If yes, does A have a relationship to B in the model of correct type? (3) Yes - then it's an implicit connection. This could be put back.

@jbsarrodie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't work from a semantic point of view because as a user I might want to use (e.g.) aggregation is some viewpoints but assignment is other viewpoints. If I use your rule, I might make wrong conclusions.

In addition, I would even go further: when nesting an element inside another one while there exist some relationships in the model but no connection in the view, I would ask the user to choose which relationship to associate to this visual nesting (and then create appropriate hidden connection) or even create another one (from another kind).

@jbsarrodie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my "hide-nested-connections" branch can be merged on master for the upcomming beta (I've just rebased it to your latest commit). I guess some more tests are needed to be sure nothing is broken, but at least it passes my personnal tests.

Please sign in to comment.