Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature proposal] More paste options #60

Open
mbd-dbc-dk opened this issue Mar 24, 2014 · 12 comments
Open

[feature proposal] More paste options #60

mbd-dbc-dk opened this issue Mar 24, 2014 · 12 comments

Comments

@mbd-dbc-dk
Copy link
Contributor

The docs state that:

Diagram elements may be cut, copied and pasted in Views. There are, however, certain constraints on how this works:

  • If an element is pasted into a View from the same model where the element already exists in that View then a new model element and a new diagram element are created for the View. The new element is a copy of the original. Any connections are also newly created as copies.
  • If an element is pasted in a View from the same model where the element does not already exist then a new diagram element is created for the View and the original model element is referenced. This is equivalent to dragging the element from the Model Tree into the View. Any connections are also referenced.

It seems, that in the "mixed" case, where some parts of the selection is already present in the view, others are not, Archi defaults to treat the whole selection as a "copy". So, if a element A and B are already in the view (with some releations), and I try to paste element A, B and C, with all releations into the model, I get a completely new set of elements, that are all a copy of the elements, so I now have A, B, A', B' and C' and all its relations.

It would be nice with an option to control the action. For my example, I would want to be able to at least have an option of pasting, such that existing elements in the view was ignored, and new elements in the view was just added.

So, in my case, insteaf of (A, B) + paste (A, B, C) => (A, B, A', B', C'), i would like to hold e.g. shift or alt down, and get (A, B) + alt-paste(A, B, C) => (A, B, C).

The usecase is "updates"; where I have a view (1) that holds the structure of e.g. a host. This host is also present in another view (2) together with other stuff. I now find that I need to add an element to the host in view (1), e.g. an additional infrastructure function, or similar, and I wish to add this addition to view (2) too. Currently I have to only copy+paste the new element from view (1), and then manually add the relations present in view (2). If I was able to copy and then have Archi only paste the "new" elements and reletations from the selection, (compared to existing content of view 1), it would be rather nice.

As usual: If you know of a way to accomplish this currently, feel free to let me know.

@mbd-dbc-dk
Copy link
Contributor Author

After thinking a bit more about this, it really is a "merge" operation I would like to have. I still think it makes sense to make is as a specialized paste operation.

I had a look at the code, and it seem somewhat tricky to retrofit on the code, but not impossibly.

The current approach is to check, before command creation, for any overlaps between the existing elements on the diagram and the ones in the CopySnapShot (selection). Everything is determined based on the first match, and set in a bool. Afterwards, there are created commands for pasting all objects, then all releations. If the bool is set, all paste and relation paste is actually copy operations. For relations, it is even "worse", as relations are only copied, if both source and target is in the selection.

It should be possibly to change this something like this:

  • check for each element in the copysnapshot if it has a model representation already, if yes, store a refence to it (or all found, if multiple). If not found, store a null.
  • for all elements in the selection, if the element is already represented in the diagram, use that, i.e "don't do a paste". Else, create a new element in the diagram, but reference the existing element in the model, i.e. "don't do a copy". If the element have been deleted from the model, since the selection was made, create a new element.
  • for all connections in the selection, check if the target and source elements are part of the diagram, after pasting/merging the elements. If they are, connect using them references to connection already present in the model. If the relation have been deleted from the model since the selection was made, create a new releation.

There is probably many loose ends. So tests are needed. A pointer to current tests would be great :-)

@Phillipus
Copy link
Member

Something important to consider, and this is something that led me to come up with the current way of doing it, is:

What happens if the user deletes elements/relations in the model after copying them to CopySnapShot. Any references to them will now no longer be valid.

[Edit - I see you mentioned it already]

(CopySnapShot tests are in the com.archimatetool.editor.tests plugin)

@mbd-dbc-dk
Copy link
Contributor Author

Semantically, if I copy something, then delete it, then paste it, I would expect to be in a situation where "nothing really happened". This is, I assume, the ideal position, although not all programs work this way.

So, if I copy something in Archi, then delete the stuff, then paste it, I would expect the stuff to be recreated when pasted. :-)

But I don't really understand how this influences this discussion? The code, as it is now, seems to default to "If any one element is missing or already part of the target diagram, then copy everything from the selection into the target diagram and into the model by making new instances". What I want is "Don't copy elements (make new instances) that already exists in the target diagram, but use them instead, along with the associated element in the model. Only make new instances, if the element is no longer part of the model.".

My assumption is that you know the model at the time of the paste? (It was something else, if I wanted to do this at the time of the copy! :-).

Perhaps that is your point: CopySnapShot exists because of the possibility that the user would remove something that was currently copied to the clipboard? I agree with that, but I don't see it as going against the feature? (I get a feeling I have totally misunderstood your post).

Its a merge, really. Is there anything in the way Archi works/represents its data that makes the feature or approach impossible?

@Phillipus
Copy link
Member

This whole area can certainly be improved.

I'm trying to remember what was a problem around the whole Cut/Copy/Paste thing when I did it. I think it was in the "Cut" action. A "Cut" is a "Copy" action, and when the "Paste" occurs the originals are deleted. But they might have already been deleted...

Certainly the whole Cut/Copy/Paste algorithm needs to be described in detail with these scenarios in mind.

@mbd-dbc-dk
Copy link
Contributor Author

Note for self: You can't reuse a relation if one of the source or target objects is not reused. Of course.

@mbd-dbc-dk
Copy link
Contributor Author

Hi Phil

I spent most of the day(!) trying to figure out how CopySnapshot works. I have documented my findings in the wiki: https://github.com/Phillipus/archi/wiki/Selecting,-Copying,-Duplicating-and-Merging

Your comments would be welcome. I understand if it is too messy for you to work with, but I obviously hope it is not, and that you might be able to comment on it - see if it makes sense.

My plan is to sketch out how I want to support a merge operation, then implement it on this branch: https://github.com/mbd/archi/tree/merge-on-paste-option-feature - for now, I just added a lot of documentation (that I hope is correct and useful).

@Phillipus
Copy link
Member

Hi Mads,

that's very comprehensive! I haven't added any comments on it at this stage since all notes are welcome and useful. I suppose the key to all of this is to remember, as you have, that a diagram node is a wrapper (with decorative properties like colour and font) around an ArchimateElement object and a diagram connection is a wrapper around a Relationship class, and there may be more than one of these diagram objects.

@mbd-dbc-dk
Copy link
Contributor Author

Hi Phil

Thanks for responding. It would be great for me, if you could clarify a single thing: Why are Diagram Relations ignored when the selection is copied to the CopySnapshot instance? I suppose there must be some sort of reason, but I can't figure out what it is...?

@Phillipus
Copy link
Member

The answer is "users". ;-)

If I remember correctly, the very attempt first Copy/Paste only copied and pasted selected objects. Then user feedback said "wouldn't it be great if it also automatically copied all associated connections as well, then I wouldn't have to select them!" Ok...I did this...and then I moved quickly onto some other feature and hoped that Copy/Paste would be forgotten as it's so horrible to implement...you know how these things go...then one day in the future someone comes along and notices that the implementation sucks and offers to look into it... ;-)

As it says in the header of the code:

This is truly horrible code. FIXME !!!!!

@mbd-dbc-dk
Copy link
Contributor Author

Hehe... users - can't live with them, can't live without them ;-)

Well, I have to ask: Wouldn't it have been more "correct" to change the selection tool to default to also select these things instead? Or, perhaps, provide multiple copy actions, where one could automatically "enlarge" the selection to include the associated relations?

My "problem" with the current approach is that there is no visual indication between what the user selects, and what actually gets copied.

Would it be OK/better with an approach where the user could choose not to "enlarge" the selection during copy? So, Archi could default to the current behaviour for Ctrl+C, but e.g. Alt+C could copy the selected elements into CopySnapshot only?

@Phillipus
Copy link
Member

I think the best behaviour is to copy and paste only that which was selected and copied, this is standard behaviour for these types of tool. Archi's current behaviour is wrong. There are a few other selection marquee tools to select connections as well.

@mbd-dbc-dk
Copy link
Contributor Author

I have to agree about the current situation.

I think a lot of the complexity in the CopySnapshot code is due to it both handling "enlarge the selection, using a few slightly fuzzy heuristics", as well as "provide a deep clone of the selection, while maintaining the relation to the original elements", as well as "create commands for paste into any diagrams, but do different things depending on wheter the underlying Archimate Elements are already present in the target".

I hate to suggest this, but it might be appropriate to try and seperate some of this into seperate classes, to allow for easier to follow code, and better testability?

My current idea is to implement a merge operation on top of the existing code (I need something to show off for the many hours I have spent on this ;-) ). If it works well, I can create a new branch where I try to "clean up" the code, re your conclusion, and eventually you can review it for inclusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants