Change element type

[etienne-sf]

Hello,

This is a Pull Request, as discussed in Issue #694 ([Feature Request] Change element type).

This issue has been closed a being covered by #663, but it is not, as this last one is about relationship, whereas the #694 is about element type. I may not reopen it.

In this PR:

• A "Refactor" menu is added into the diagram context menu, and in the Tree View menu. They have been developped based on the other existing actions/commands. In particular, based on the Delete action of the Tree View menu.
• This "Refactor" menu contains submenu, one per element type (Business, Application...). Each of these submenus contain the items of this type.
• The Undo and Redo works for both
• There is no control on the existing relationship for this element. As a consequence, and to be fully operationnal, the #663 issue should be also implemented. This would allow to change relationship's type, when a relationship becomes "illegal" based on the new element type.
  • I can work on the #663 implementation. But I'll start that only if you think you'll accept these two PR.
• The job is done by creating an ArchimateElement of the good type, copying into it the id, name, description and properties from the source one. The replacing the element in all DiagramModelArchimateObject that contain the old one. The new element is created in the same folder (if the element is of the same type), or in the root of the folder's type otherwise
  • A JUnit test checks that the old instance doesn't exist any more in the model
• A tricky and sensitive part is the ability to create an instance of an element type, based on its name, by using Java reflection. It's not very difficult, but it will fail if a name is changed. So a JUnit test checks that instance creation works for every element type.
• It's of course possible to change the type for several elements in the same operation (multiple selections)
• JUnit tests have been added and integrated in the existing test suites. They check the execute, undo and redo actions. And I confirm the good integration in the whole maven build, as I checked that with a failing test.
• The last commit from the archi master has been merged

Changed after the command from the previous and now closed PR (699):

• Element instances are now created by the IArchimateFactory, instead of Java reflection
• Features are now copied from the source element to the target one, that is for execution, undo and redo. No change on diagram objects, as these objects are unchanged (only its archimate element is set)
• About storing the color: I had the same think as you. So this the code works, but is currently deactivated, thanks to the CHANGE_COLOR_TO_DEFAULT_IF_ELEMENT_TYPE_CHANGES constant set to false.
• Added the icons for each element type, in the context and tree menu
• I didn't remove the .ignore file. If I do, eclipse wants to commit around 27000 files, that I'm sure you don't want in git. Why do you want to remove it? (perhaps you have your own ignore file, but it is not commited in git)

NB:

• Thanks to your documentation (very clear), I was able to implement and execute JUnit tests, and embed them in the whole build. I check this, as a change in my code has made the JUnit test to fail. And I was happy to double-check it, before raising an issue on them, as this issue was mine ;)
• I also was able to build a Product, and I use it now for my own Archimate work.

Etienne

[Phillipus]

Thanks for this draft PR.

Looking at ChangeElementTypeCommand:

I don't think I would implement it this way. Normally in a Command the state of all affected objects is stored before calling Execute so that Undo can roll back to those objects in that state. Your implementation does things differently by performing another Execute when Undo is called.

This means that you have to store the elementId to get the original object and also the model in order to call ArchimateModelUtils.getObjectByID().

(Also, I don't think you should use a class ArchimateObjectModelProperties to store state, and no need to store the colour.)

I think if I were to do this it would be more in line with the jArchi implementation which restores the previous objects on Undo rather than creating new objects each time.

[Phillipus]

I didn't remove the .ignore file. If I do, eclipse wants to commit around 27000 files, that I'm sure you don't want in git. Why do you want to remove it? (perhaps you have your own ignore file, but it is not commited in git)

Your .gitignore for the root of all projects is:

#Ignore build files, in all subfolders 
bin/
icons/
target/

**/.project
**/*.jar
**/*.log

.project and .jar files should not be ignored as these are part of each project. bin is already in each project's .gitignore file. Perhaps you are using a maven build? When I do a maven build I run a script that copies the source to a new folder then builds in that folder so I don't mess my source folder with target folders. But it's up to you, it just means that I can't merge any commit with it in,

[jbsarrodie]

I think if I were to do this it would be more in line with the jArchi implementation which restores the previous objects on Undo rather than creating new objects each time.

I agree, that's even mandatory to keep ids, or else this is not a real undo as we expect and undo to come back to the exact previous state.

For the sake of simplicity ,here is a copy of what I wrote in #694

Might be worth looking at this issue for a reminder of previous discussions about it.

So again, here are the key points, based on the implementation done in my jArchi scripts set:

• User should first decide what to do with invalid relationships. Only two options here: abort or convert to associations (and then keep track of previous type in a comment added into the documentation attribute).
• If 'abort', then the whole operation should succeed only if all elements can be converted. This means that as soon as converting one element leads to unvalid relationship, we should roll-back the whole operation.
• The new feature should be able to also change type of relationships. But this adds additional checks (junctions)
• The "new" concept should have a new id and not reuse the old one as this is no more the same concept (this is to avoid side effects on model import feature, coArchi...)

Btw, "refactor" is not really a good menu name (sounds too technical to me). A simple "Change type..." menu with per layer sub-menus seems better.

In addition, there is something which can't be implemented in jArchi but can in a native implementation: when converting multiple concepts in a single operation, intermediate states might be invalid while the target state is valid. In jArchi, this leads to (temporarily) invalid relationships to be converted to association.

For example, take a Business Object A which is composed by another Business Object B. If I select A and B and want to convert them to Data Objects, I should get Data Object A is composed by Data Object B, because composition is allowed between two Data Objects. In the jArchi implementation, because I first convert A, I have an intermediate state in which A is a Data Object and B is still a Business Object. This is invalid, thus I am forced to first convert the composition to association.

What we should do in a native implementation is to first convert all elements, an only after, convert invalid relationship to association.

[etienne-sf]

Thanks for this review!

I respond point by point hereafter

Etienne

[etienne-sf]

Perhaps you are using a maven build? When I do a maven build I run a script that copies the source to a new folder then builds in that folder so I don't mess my source folder with target folders. But it's up to you, it just means that I can't merge any commit with it in,

Yes, I'm using a maven build.

I don't understand why you don't want this ignore files: it doesn't hurt, and it protects git from containing files that should not be there. But it's up to you, of course.

I removed it.

[etienne-sf]

Thanks for this draft PR.

Looking at ChangeElementTypeCommand:

I don't think I would implement it this way. Normally in a Command the state of all affected objects is stored before calling Execute so that Undo can roll back to those objects in that state. Your implementation does things differently by performing another Execute when Undo is called.

This means that you have to store the elementId to get the original object and also the model in order to call ArchimateModelUtils.getObjectByID().

(Also, I don't think you should use a class ArchimateObjectModelProperties to store state, and no need to store the colour.)

I think if I were to do this it would be more in line with the jArchi implementation which restores the previous objects on Undo rather than creating new objects each time.

Yes, that's right. Currently, I store the id. This allows the execute/undo/redo in an easy way, as all executions are identical. But I need to call getObjectByID, which browse through the model.

So your idea make the code a little quicker and a little more complex, I think.

I'll give it a try.

Etienne

[etienne-sf]

jbsarrodie wrote:

User should first decide what to do with invalid relationships. Only two options here: abort or convert to associations (and then keep track of previous type in a comment added into the documentation attribute).

Hum, I only partially agree with this one.

There is a third choice, which is the current one: let the association be invalid.

I see several reasons for that. Ok, they are not all very very good. But once grouped, it makes some sense. And also see after, what I propose.

• It makes the functionality more complex to use. How to manage this, when several items are selected, of several types
• It makes the code really more complex than it is now. Or perhaps there is already a portion of code the defines the valid relati
• without this PR, I still need to change some objects. And (I'm sorry for that) I won't use jArchi. It's too complex to learn how to install it, learn a new language, a new API. So I would export the model data, change the element's type, then reimport.
• It may also happen that the relationship is that bad: perhaps the user will change other element's type, just after, making the relationship be correct again
• There still is a validator, that allows to check valid and invalid relationship.

So an idea would be to:

• Let it be simple, to start with, that is: let this PR as is.
• Develop the #663 (UI for changing relation type). I can work on this one, but I must finish some work on my other open source project.
• Allow a Right Click on the Validator view, to allow quick correction of wrong relationships. This would be done in the #663 implementation. Perhaps a warning message could lead to the display of this view

This would be partial implementation, but will add two important features into archi.

What do you think ?

Etienne

[Phillipus]

A quick comment on this one:

Allow a Right Click on the Validator view, to allow quick correction of wrong relationships.

I don't want to add this kind of functionality to the Validator. I prefer to keep the Validator as it is now, just reporting the issues rather than turning it into a fix-it thing. :-)

[jbsarrodie]

There is a third choice, which is the current one: let the association be invalid.

Absolutely not: Archi, as an an ArchiMate modelling tool can't include invalid relationships. Period.

It makes the functionality more complex to use. How to manage this, when several items are selected, of several types

It's just a matter of asking the user what to do once: either rollback if the target state is invalid, or convert to association. That's how the current script work and this has proven to be a good approach.

It makes the code really more complex than it is now.

Nobody said this PR was easy. Why do you think we didn't implement it knowing that a working and easy solution exist in jArchi.

without this PR, I still need to change some objects.

Not our problem but yours

And (I'm sorry for that) I won't use jArchi. It's too complex to learn how to install it, learn a new language, a new API.

Come on. It's only a matter of installing the plugin through the plugin manager and copying a script which already exist.

There still is a validator, that allows to check valid and invalid relationship.

This is only meant to be used to discover some issues related to manual edits, or model screwed by external plugins. That's not a argument to purposefully create invalid models.

So an idea would be to:

• Let it be simple, to start with, that is: let this PR as is. What do you think ?

You don't seem to understand what a good product is. That's absolute non sens to propose a PR which doesn't address all this. It seems to me that you in fact just want to adresse your own issue without actually contributing to Archi. As we first explained with Phil, contributing implies lots of things, including to first agree on the feature scope.

@Phillipus I'm afraid we'll loose energy an time if there is not a clear commitment to work on the whole feature, at least on par with the jArchi implementation done in my script set. FWIW, this is not an urgent feature...

[etienne-sf]

Hum,

I feel that you're disappointed there. I'm sorry for that. Please note that I was just presenting ideas.

About jArchi : please also not that I'm not the only one to think like this. For instance, stefankroes did the same comment, in the #265 issue: https://github.com/archimatetool/archi/issues/265#issuecomment-537808993

About "this is not an urgent feature". Yes of course: archi is an excellent tool. But please note that, as it's a free tool, lots of beginners start with it. This allows to test ArchiMate. It was the case for me, and for lots of other architects around me. And beginners do beginner's errors, like creating a BusinessActor instead of a BusinessRole, for instance. So, IMHO, the capacity to correct this kind of error is important for such a free tool. It's a driver powerful enough to pay a for a licence.

To sum up: I'll work on this PR, in order to respect the requirements you defined, here above

Like stated before, I also work on another open source project. So I'll be back in some weeks, I guess.

Etienne

[jbsarrodie]

I feel that you're disappointed there. I'm sorry for that.

Yes, I was.

Please note that I was just presenting ideas.

The way I read it was: "I need this feature and don't care about how it fits in Archi, so I won't do more"

About jArchi : please also not that I'm not the only one to think like this. For instance, stefankroes did the same comment, in the #265 issue: #265 (comment)

His comment was mostly based on "jArchi is not publicly available at this point.", while it was. The thing is that people don't even read what's written on the plugins page. I can imagine that not everyone wants to setup eclipse and compile the plugin, but in this case there's the donation route, and frankly, if someone is not able to give 1$ to get a plugin and install a community contributed script, then that's simply not our target user and he/she should buy a commercial tool with 2000€/day consultant and some support.

That being said, @Phillipus I think we should update the list of provided scripts in the binary version to include my script pack. This would be one less step for those who get the plugin through paypal one time donation or patreon.

About "this is not an urgent feature". Yes of course: archi is an excellent tool. But please note that, as it's a free tool, lots of beginners start with it. This allows to test ArchiMate. It was the case for me, and for lots of other architects around me. And beginners do beginner's errors, like creating a BusinessActor instead of a BusinessRole, for instance. So, IMHO, the capacity to correct this kind of error is important for such a free tool.

I know, but this is already covered by a jArchi script and there are more urgent features. Remember that only Phil and (a bit of) me work on Archi.

It's a driver powerful enough to pay a for a licence.

So it's a driver powerful enough to donate 1$ for jArchi.

To sum up: I'll work on this PR, in order to respect the requirements you defined, here above

Great. Let's be clear, my goal is not to push you out, only to make it clear that any new feature in Archi must be first discussed as a whole before even thinking about code. That's only when we are all clear on the feature that we can all move on. It seems we are now aligned and that's good.

So I'll be back in some weeks, I guess.

In the mean time I'll clarify even more the behavior I have in mind so we can all agree on it or tune it if needed.

[etienne-sf]

Thanks for your reponse, Jean-Baptiste

[Phillipus]

When it comes to arguing about paying a $1 for a plug-in it's time to move on...

Closed.