search

archimatetool / archi

Archi: ArchiMate Modelling Tool
https://www.archimatetool.com
MIT License
946 stars 269 forks source link

Add support for specialization in other Archi's features #740

Closed jbsarrodie closed 2 years ago

jbsarrodie commented 3 years ago

See #705 for the core support for specialization and images

This issue is used to keep track of the work needed to make specialization find its way into other Archi's features (excluding coArchi and jArchi).

Here's a list of things to do or check (will be updated when needed):

For a better description of each item, see this wiki page

jbsarrodie commented 3 years ago

Just added Label Expression to the list.

Phillipus commented 3 years ago

Model import feature

This is in branch specialization-modelimporter and needs testing.

CSV export/import

Not sure what needs to be done for this.

Copy/Paste (inside same or different models)

Working in same model. But if between different models, Profiles and Profile refs are not pasted as it can get extremely complicated (would be like the model importer)

Format Painter

Working. But any images are not pasted if pasting to a different model (again, that gets very complicated adding images to target ArchiveManager, undo/redo, etc)

Potential impact on model templates

Not aware of any impact.

Potential impact on Canvas template

Not aware of any impact.

Label Expression

What do we want to do here?

jbsarrodie commented 3 years ago

CSV export/import

Not sure what needs to be done for this.

I think I would add a Specialization column on elements.csv and relations.csv. This columns should be the last one (in case someone uses the columns order, but who would do such thing nowadays?) and should contain the name of the specialization of the element or relationship.

Label Expression

What do we want to do here?

I was thinking about ${specialization} that would return the name of the specialization of the concept (if any).

In fact the goal is to be able to reproduce the default guillemets notation, so in fact we need a bit more than that because if we use a label such as...

${name}
<<${specialization}>>

...we'll end up we something like...

Some Concept
<<>>

...if the concept has no specialization.

So I see two options:

Potential impact on model templates

Not aware of any impact.

Potential impact on Canvas template

Not aware of any impact.

I think none but I have to test.

Format Painter

Working. But any images are not pasted if pasting to a different model (again, that gets very complicated adding images to target ArchiveManager, undo/redo, etc)

I have to test and see if this is intuitive.

Copy/Paste (inside same or different models)

Working in same model. But if between different models, Profiles and Profile refs are not pasted as it can get extremely complicated (would be like the model importer)

I have to better document my idea which is to set the specialization if one exist in the target model with the same id or the same name (if no match on id).

Model import feature

This is in branch specialization-modelimporter and needs testing.

I have to test. I think there's an underlying issue: if I import a model containing the definition of one specialization which has the same name of a specialization already defined in the target model, then they should be considered the same.

In fact, from an ArchiMate point of view, the specialization is identified by its name (that's the only thing visible to the user). But in a tool, we need to have a technical id or else renaming a specialization would impact lots of concepts. So:

Phillipus commented 3 years ago

I think there's an underlying issue: if I import a model containing the definition of one specialization which has the same name of a specialization already defined in the target model, then they should be considered the same.

Name should be used as an equivalent of the id when importing/merging/pasting (I can elaborate on this and provide examples if needed)

I just don't think that's possible, or at least very difficult to implement and manage,

jbsarrodie commented 3 years ago

I just don't think that's possible, or at least very difficult to implement and manage,

My feeling is that I'm not explaining it well and that this should not be complicated, so that's why I have to check the code for model import and see myself if/how it could be done. Of course, maybe I'm totally wrong and there's a blocking point I just don't see for the moment.

jbsarrodie commented 3 years ago

that's why I have to check the code for model import and see myself if/how it could be done.

This seems possible I've pushed a first attempt (see this commit). It needs a bit more work but I'll continue tomorrow.

jbsarrodie commented 3 years ago

This seems possible I've pushed a first attempt (see this commit). It needs a bit more work but I'll continue tomorrow.

Ok, I've worked a bit more on this and I think it is possible (as shown in shared code).

While working on it, I faced a bug that I thought was due to my changes, but in fact I can reproduce it with your code (the one from specialization-modelimporter branch):

My analysis:

It seems we should use ArchimateModelUtils#findProfilesUsage() to get the list of concepts attached to the original profile and then add a command similar to ConceptImporter.UpdateProfilesCommand.

While trying to uderstand the logic behind, this seems due to the fact that, for concepts, we use ModelImporter#updateObject() which copies attributes of importedObject into targetObject, while for profiles, ProfileImporter#updateProfile() relies on ModelImporter#cloneObject() which creates a new eObject.

This made me wonder if we could not simplify the code a bit to have a mode generic implementation of updateObject() that would be inspired by EcoreUtil.Copier#copy() and would work on (almost) any object.

Phillipus commented 3 years ago

This made me wonder if we could not simplify the code a bit to have a mode generic implementation of updateObject() that would be inspired by EcoreUtil.Copier#copy() and would work on (almost) any object.

No, don't use EcoreUtil.Copier#copy() directly. It will make a deep copy with references to other objects that we don't want. That's why I made updateObject() shallow.

It might be better to use a modified version of updateObject() that copies the Profiles other info.

jbsarrodie commented 3 years ago

No, don't use EcoreUtil.Copier#copy() directly. It will make a deep copy with references to other objects that we don't want. That's why I made updateObject() shallow.

I was thinking about a simplified version of copy that would just update all EStructuralFeature:

public copyInto(EObject eObject, EObject copyEObject)
    {
      if (eObject == null || copyEObject == null)
      {
        return null;
      }
      else
      {
          EClass eClass = eObject.eClass();
          for (int i = 0, size = eClass.getFeatureCount(); i < size; ++i)
          {
            EStructuralFeature eStructuralFeature = eClass.getEStructuralFeature(i);
            if (eStructuralFeature.isChangeable() && !eStructuralFeature.isDerived())
            {
              if (eStructuralFeature instanceof EAttribute)
              {
                copyAttribute((EAttribute)eStructuralFeature, eObject, copyEObject);
              }
/*
              else
              {
                EReference eReference = (EReference)eStructuralFeature;
                if (eReference.isContainment())
                {
                  copyContainment(eReference, eObject, copyEObject);
                }
              }
*/
            }
          }
        }

        return copyEObject;
    }

Of course this won't do 100% of the job but would help us not forget to update some EFeature...

...And this made me think about a potential case in Archi 4.8.1:

This is because ModelImporter#updateObject() only updates Name, Documentation, Properties and Features. So I guess this issue exists in the following cases:

So which approach do you prefer? extending ModelImporter#updateObject() to support these exact cases, or make it a bit more generic by using (a variant of) my copyInto() function?

Phillipus commented 3 years ago

Yes, those cases are missing. We do need a generic copy() function that doesn't have to be maintained. Something like copyInto() would do it, as long as it was Undo-able.

jbsarrodie commented 3 years ago

Yes, those cases are missing. We do need a generic copy() function that doesn't have to be maintained. Something like copyInto() would do it, as long as it was Undo-able.

Ok, I'm on it. I'll first provide a generic fix based on master (ie. the code without specialization/profiles) that we'll backport here when tested and agreed.

So, I'm creating a dedicated issue for this topic.

Phillipus commented 3 years ago

I'll first provide a generic fix based on master

Is there any point? We won't be releasing Archi 4.8.2 so any changes will be part of 4.9.

Also, there is some refactoring in specialization-modelimporter branch that is useful

jbsarrodie commented 3 years ago

Is there any point? We won't be releasing Archi 4.8.2 so any changes will be part of 4.9.

Well, I prefer working on something "simple" first and then upgrade to support profiles. It just seems incremental and easier for me this way. (this doesn't mean it will take a long time)

Edit: I was not aware of some refactoring done in specialization-modelimporter. I'll work on this branch.

Phillipus commented 3 years ago

was thinking about ${specialization} that would return the name of the specialization of the concept (if any).

I've implemented (with unit tests) the base implementation of this in the working branch.

Playing devil's advocate for a moment...

Do we need anything more complex than that? Does it matter if a user sees <<>> if no Specialization is set? Perhaps they didn't want to set one and so the label expression is not needed? Will users really use something like ${concat:${concat:\n<<:${specialization}}:>>}? ;-)

jbsarrodie commented 3 years ago

Does it matter if a user sees <<>> if no Specialization is set?

Not very user friendly

Perhaps they didn't want to set one and so the label expression is not needed?

If someone doesn't want to use specialization images but instead want to use the guillemet notation, he/she will certainly set the label expression on all elements of the view as this can be done quickly (select all and edit the label). At least, that's ahow I manage such cases.

Will users really use something like ${concat:${concat:\n<<:${specialization}}:>>}? ;-)

I have seen worse than that ;-)

But people can also create a model property named Specialization Label, set its value to ${concat:${concat:\n<<:${specialization}}:>>}, and then, on each visual object they can simply use $model{property:Specialization Label}...

Phillipus commented 3 years ago

OK. I just wanted to hear the use case. :-)

Gonna need help with concat :cat: 🐈

jbsarrodie commented 3 years ago

Gonna need help with concat

No pb.

BTW, might be best to name it condcat for conditional concatenate ;-)

jbsarrodie commented 3 years ago

BTW, concat (or condcat) makes it possible to create conditionnal labels as that's a basic if then

Phillipus commented 3 years ago

🤯

jbsarrodie commented 3 years ago

BTW, concat (or condcat) makes it possible to create conditionnal labels as that's a basic if then

Thinking about it... maybe we should really go this way and define ${if:condition:text} where text is returned if condition evaluate to a non empty string

So ${name}${concat:${concat:\n<<:${specialization}}:>>} could be simplified to ${name}${if:${specialization}:\n<<${specialization}}>>} which is easier to understand.

Phillipus commented 3 years ago

🤯 🤯 🤯 🤯 🤯 🤯 🤯 🤯 🤯

Phillipus commented 3 years ago

While working on it, I faced a bug that I thought was due to my changes, but in fact I can reproduce it with your code (the one from specialization-modelimporter branch):

* Create a model A which only contains the definition for a specialization (default one for Business Actor is perfect)

* Create a model B and import A into it.

* In B, add a new Business Actor with the specialization imported from A.

* Try to import A again into B while allowing update, you'll get an error similar to this one:
  ![image](https://user-images.githubusercontent.com/5757396/123939073-0958c100-d998-11eb-8fe7-c27fa9df6e32.png)

My analysis:

* When importing the second time, the profile object will be re-created because we allowed updates.

* `ProfileImporter` takes care of updating the list of profiles for the model, but it doesn't update already existing concepts (in targetModel) that were referencing the now superseeded profile.

It seems we should use ArchimateModelUtils#findProfilesUsage() to get the list of concepts attached to the original profile and then add a command similar to ConceptImporter.UpdateProfilesCommand.

While trying to uderstand the logic behind, this seems due to the fact that, for concepts, we use ModelImporter#updateObject() which copies attributes of importedObject into targetObject, while for profiles, ProfileImporter#updateProfile() relies on ModelImporter#cloneObject() which creates a new eObject.

This made me wonder if we could not simplify the code a bit to have a mode generic implementation of updateObject() that would be inspired by EcoreUtil.Copier#copy() and would work on (almost) any object.

Should be fixed. See https://github.com/archimatetool/archi/issues/746

achampion commented 3 years ago

Any reason for ${type} not to return the specialization (if there is one), this is how I think of specialization - a new specialized type. You could extend the expression, e.g. ${type:base} if you explicitly want the base type. Both of these will always return something.

achampion commented 3 years ago

Perhaps I wasn't clear. Yes I understand ${type} is currently returning the base type of the object but if it is specialized why wouldn't ${type} return the specialized type name? E.g. a specialization of the parent concept "Business Actor" to the new concept "Threat Agent" (from the standard "Specialization is a simple and powerful way to define new elements or relationships based on the existing ones.").

I was suggesting the ${type:base} or ${type:parent} would always return the core type. Whereas ${type} would return the specialized type if one is defined or the base type. (Perhaps parent more aligns to the standard which describes "Parent Concept" and "Specialized Concept").

On Wed, Jul 28, 2021 at 12:57 AM Phil Beauvoir @.***> wrote:

Any reason for ${type} not to return the specialization (if there is one), this is how I think of specialization - a new specialized type. You could extend the expression, e.g. ${type:base} if you explicitly want the base type. Both of these will always return something.

We already use ${type}:

See https://github.com/archimatetool/archi/wiki/Label-Expressions

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/archimatetool/archi/issues/740#issuecomment-888032726, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBZ2TOYRMWYMO2K7JQEEJTTZ6L4VANCNFSM47IGIODQ .

achampion commented 3 years ago

One other thought is perhaps use shell (e.g. bash) like parameter substitutions as an alternative to if or concat, e.g.: From bash:

${parameter+alt_value}    - If parameter set, use alt_value, else use null string.

So if you optionally want to print <<${specialization}>> after ${name}

${name}${specialization+\n<<${specialization}>>}

Note: there is also ${param-default} which provides a default value if param isn't set, e.g. if you want to print <<${specialization}>> if it exists or <<${type}>> if it doesn't:

<<${specialization-${type}}>>
Phillipus commented 3 years ago

Note: The specialization and specialization-modelimporter branches have been rebased onto the latest master branch.

jbsarrodie commented 3 years ago

Any reason for ${type} not to return the specialization (if there is one), this is how I think of specialization - a new specialized type.

I personnaly prefer to have two separated expression because in some cases one could want to get access to the real (i.e. ArchiMate) type instead of the specialization. This also matches jArchi API.

You could extend the expression, e.g. ${type:base} if you explicitly want the base type. Both of these will always return something.

That could be an option, but we'll then end up with:

And this seems (at least to me) less user friendly and explicit than:

Of course, this makes it a bit more complexe to get either type or specialization:

(assuming an new ${if:condition:string:string} generix expression)

One other thought is perhaps use shell (e.g. bash) like parameter substitutions as an alternative to if or concat

I like the idea (I used to do a lot of shell scripting). I have to think about it a bit more to see if it doesn't make it more complexe for a typical user (usually more the kind of people that is ok with Excel formulas not not so much with scripting).

jbsarrodie commented 3 years ago

I like the idea (I used to do a lot of shell scripting). I have to think about it a bit more to see if it doesn't make it more complexe for a typical user (usually more the kind of people that is ok with Excel formulas not not so much with scripting).

After some more thought, I don't think it would be useful because this shell notation assumes the first part is the name of a variable while with labels this is not the case: it looks similar for simple case (type, documentation, specialization) but not for expressions taking arguments. This also means that we would have to implement it for each expression, which is a lot more work than a unique ${if...}.

So I think I'll go for some conditionnal expressions such as:

@Phillipus : I'll implement them myself

Phillipus commented 3 years ago

@jbsarrodie Keep in mind if more complex expressions might slow things down. Every action performed in Archi will refresh the label expression for every visible object (and Models Tree).

jbsarrodie commented 3 years ago

@jbsarrodie Keep in mind if more complex expressions might slow things down. Every action performed in Archi will refresh the label expression for every visible object (and Models Tree).

Of course, but I don't think it will be worse than some expressions I already tested (element's label referencing expressions snippets stored as model properties). Testing only if condition is empty (and no other math based condition) should make it easier for most of them because in fact I don't mind the exact content of condition most of the time and will just have to test if the keyword (if, ifnull, nvl) is followed by two : or not.

jbsarrodie commented 3 years ago

So I think I'll go for some conditionnal expressions such as:

  • ${if:condition:stringIfConditionNotNull:stringIfConditionNull} : generic if/then/else structure
  • ${if:condition:stringIfConditionNotNull} : simplified if/then structure
  • ${nvl:stringToCheck:stringIfNull} : returns stringToCheck if it is not null or stringIfNull otherwise

These 3 have been implemented in commit 847beeca02a024b1470ad1bf275f7b941995a453

  • ${ifnull:condition:stringIfConditionNull} : simplified if-not/then structure

This one won't be implemented (I don't see much value in it)

I've added a first check based on String.contains() to see if keywords (if, nvl) are there, so that we don't use Matcher if not needed.

jbsarrodie commented 3 years ago

To keep track of recent offline discussions with Phil about CSV

In commit 765260b4802dee0f65c05127d05a38d9486f152b, @Phillipus wrote

We should perhaps have a new file for Specialization containing ID, Name in case we add further info such as Documentation and Properties

This will be needed in the future when we'll have more informations attached to a profile/specialization. But for the moment (MVP) we only need a way to know which specialization is attached to a concept.

This could be done either by adding the name of the specialization to elements.csv and relationships.csv, or by adding the id of the specialization and add a specialization.csv. Because a specialization can always be identified by its name and the concept type it refers to, we can safely use this name and not a technical id. This has two advantages: easier to read and use for people, and no need for a new file yet.

Phillipus commented 3 years ago

Thanks for the conditional expressions!

We need some unit tests for them. I can do the unit tests if you can provide some examples of usage.

Do you think the expressions will have much impact on refresh times?

jbsarrodie commented 3 years ago

We need some unit tests for them. I can do the unit tests if you can provide some examples of usage.

Here's my test model for this: Test for specialization, if and nvl.zip

It contains a single view with 3 label expression to test, each with two objects (one with specialization, one without) : image

Do you think the expressions will have much impact on refresh times?

I don't think so because I've made sure we actually use RegExp only if we are sure the expressions are used. Maybe we could even use this trick elsewhere.

But at some point it would be good to have a real, testable way to mesure impact of new expressions.

Phillipus commented 3 years ago

Maybe we could even use this trick elsewhere.

    private String renderWithExpression(IArchimateModelObject object, String formatExpression, String defaultText) {
        if(!StringUtils.isSet(formatExpression)) {
            return defaultText;
        }

But, yeah, on each renderer we could do a match to test it.

Edit: there is an overhead in String#contains which may be about the same as calling Matcher#find in the simpler renderers.

Phillipus commented 3 years ago

But at some point it would be good to have a real, testable way to mesure impact of new expressions.

I did this when we first created the label renderers. I had a huge diagram with hundreds of label expressions. I added a timer to check the results. Time overhead was not too bad. As long as people don't have too many Views open it should be OK. Anyway, Apple have a new M1 chip now, maybe we can persuade them to add Archi label renderer support... ;-)

Phillipus commented 3 years ago

I've pushed a new commit with unit tests for the renderer. It makes it easier to test all kinds of cases should the need arise.

If you want to add any more tests you can add them to IfRendererTests (that's not a hint to add more, just if you are curious)

See https://github.com/archimatetool/archi/wiki/JUnit-Tests for how to run them in Eclipse.

Phillipus commented 3 years ago

reverted...fixing a bug...before pushing again...

Phillipus commented 3 years ago

reverted...fixing a bug...before pushing again...

...done

Phillipus commented 3 years ago

@jbsarrodie It's getting harder to manage these branches and I think there's no going back now. I suggest:

Agree?

jbsarrodie commented 3 years ago

Agree?

Yes

Phillipus commented 3 years ago

@jbsarrodie It's getting harder to manage these branches and I think there's no going back now. I suggest:

* merge `specialization` into master

* merge `html-report2` into master

* delete `html-report` (old branch)

* set `specialization` to the same commit as master and continue development work in that

* rebase `specialization-model-importer` onto master

Agree?

Done. Some other branches have been rebased/cherry picked too,

Phillipus commented 3 years ago

CSV import/export is done now in branch specialization-csv. Just needs to be tested.

Phillipus commented 2 years ago

@jbsarrodie There are now three branches that need testing. I've re-based them onto latest master which uses a target of Eclipse 4.21 so you'll have to do the reload target thing...

specialization-csv - CSV import and export specialization-model-importer - Model importer copy-paste - Format painter copies the image to an object if it is in a different model, and Copy/Paste copies images and specializations if the target is a different model (if a target specialization exists use that, else create a new one)

Phillipus commented 2 years ago

...just found a bug in copy-paste...that will need fixing first.

Phillipus commented 2 years ago

and Copy/Paste copies images and specializations if the target is a different model (if a target specialization exists use that, else create a new one)

Temporarily removed this. It's not as simple as I first thought.

Phillipus commented 2 years ago

Temporarily removed this. It's not as simple as I first thought.

And...it's back again. That was tricky - undo/redo can have some hidden gotchas... 🤓

jbsarrodie commented 2 years ago

copy-paste - Format painter copies the image to an object if it is in a different model, and Copy/Paste copies images and specializations if the target is a different model (if a target specialization exists use that, else create a new one)

I've just tested it and for me it works as expected. Now I only need to test new jArchi features...

smileham commented 2 years ago

Hey chaps, (fabulous work on Specialisations thus far!) will there be any support for Specialisations in the "Search Filters" functionality, it would be great if we could filter on our specialisations here image

Maybe as a "third tier" list under the "parent" concept, e.g. under "Artifact > Database (icon), File (icon)"? image

Phillipus commented 2 years ago

@smileham Where do you think they should appear? Alongside the concepts, or in a separate menu list?