Undo/Redo

[Phillipus]

I've started work on this in the "dev" branch.

Each import operation should be performed as an undoable command to be put on the command stack for the target model.

This means that we won't have to ask the user to save the model first and we can remove the temporary reset of the command stack.

This issue is just a placeholder to track any problems that might be found for Undo/Redo of the whole import operation.

[Phillipus]

Turns out this is a tad more complicated than I expected... 90% there... ;-)

[Phillipus]

Yes!!!

Undo/Redo implemented.

There may be bugs so need to check multiple undo and redo actions...

[jbsarrodie]

I've tested and it works so far... (waiting a bit before closing, just in case...)

[jbsarrodie]

waiting a bit before closing, just in case...

found a bug while testing edge case 1 :-(

Edge case 1

• Model B as already been imported into A
• Some relationship's ends has been changed
• Let's assume this relationship was used in a view "V" coming from B (and only this view), but removed since
• We merge again B into A with "update" option not set
• "V"is restored (because this is a merge and "V" no more exist), but because "V" contains a relationhip whose ends have changed, this relationship's ends are restored (thus it is updated while update mode is off)

Now, undo the model import: the relationship's ends changes are not rolled back

Note: if before re-importing model B into A, there are some views containing the relationship (ie. views that are impacted by the relationship's end been restored), then after undo everything is ok.

[Phillipus]

Can you summarise this in simple steps to create model A and B?

[jbsarrodie]

Use these models: Test Merge.zip

1. Open "Model A+B changed"
2. Look at the top level Capability "NEW CAPA": it is the target end of a composition relationship involving another capability comming from B
3. Import "Model B" with update mode disabled
4. "NEW CAPA" is still there (of course), but is no more the target end of a composition relationship because this original relationship were used in view from B which were not in model A and has thus been imported, causing some "forced-changes" to the relationship.
5. Undo
6. The relationhip's target end should be back to "NEW CAPA" but is not.

[Phillipus]

OK. If you could check the logic of the code with me...

In ConceptImporter#importConcept we have this:

if(shouldUpdate() || createdNewConcept) {
    // Relationship ends
    if(importedConcept instanceof IArchimateRelationship) {
        setRelationshipEnds((IArchimateRelationship)importedConcept, (IArchimateRelationship)targetConcept);
    }

It seems that calling setRelationshipEnds in all cases fixes the issue. But this will change the relationship ends when the user doesn't want them to be changed when update is off?

[jbsarrodie]

But this will change the relationship ends when the user doesn't want them to be changed when update is off?

It already changes the relationship when update is off (for this very specific edge case).

OK. If you could check the logic of the code with me...

I'll check the code and see exactly what happens. This might take some time as I have to switch to another topic :-(

[Phillipus]

I think the cause is in the main Archi code at DiagramModelArchimateConnection#reconnect()

if(source instanceof IDiagramModelArchimateComponent && target instanceof IDiagramModelArchimateComponent) {
    IArchimateConcept srcConcept = ((IDiagramModelArchimateComponent)source).getArchimateConcept();
    IArchimateConcept tgtConcept = ((IDiagramModelArchimateComponent)target).getArchimateConcept();
    
    IArchimateRelationship relationship = getArchimateRelationship();
    
    if(relationship.getSource() != srcConcept) {
        relationship.setSource(srcConcept); 
    }
    if(relationship.getTarget() != tgtConcept) {
        relationship.setTarget(tgtConcept);
    }
}

When a new View is imported the connections are made and then the underlying source and target concepts are re-assigned,

Need to look at ViewImporter#createConnections() - perhaps we need to keep a hold of previous concept ends. Investigating.

[Phillipus]

In ViewImporter the connections are not put on the undo/redo stack because they will be automatically removed on undo of creating or updating a new View.

But...if DiagramModelArchimateConnection#reconnect() is quietly changing the concept source/target ends then I think we do need to put this on the stack.

[Phillipus]

Yes, this is the cause of the problem. Now working on a fix...

[Phillipus]

Before I fix this, a question - are there any other possible scenarios where:

1. A relationship's end(s) might change
2. "Update" is off when importing

If there could be a chance of this happening then perhaps we should call setRelationshipEnds() in all cases? But on the other hand this might not be wanted under other circumstances?

[jbsarrodie]

are there any other possible scenarios where:

I don't think so, this is really an edge case.

this might not be wanted under other circumstances

Exactly, this really should not happen, and when it happens (edge case) we really want to warn the user at the end.

[Phillipus]

OK, thanks, I have a fix for it. I just need to refine it...

[Phillipus]

...should be fixed now.