method .is("*") returns different results for visual objects an concept

markbacker commented 1 year ago

Version of jArchi, Operating System

Archi: 5.0.2.202304051112
Platform: windows 10
Javascript engine: GraalVM
jArchi plugin: 1.4.0.202303211027

Expected Behaviour

A test with .is("*") should return the same result for both visual objects as for concepts.

So far, I've seen this behavior only with the selector "*". This didn't happen in a previous release. Don't know the jArchi version that introduced this error

Actual Behaviour

The test with a visual object returns false. The test with the same object selected in the model tree returns true

Steps to Reproduce the Behaviour (with attached test files/script)

/**
 * test .is()
 * 
 * select an application-component on a view and run the script
 * next select the application-component in the model tree and run the script
 * 
 */
console.log(`Result from .is("*") = ${$(selection).is("*")}`)
console.log(`Result from .is("application-component") = ${$(selection).is("application-component")}`)

Phillipus commented 1 year ago

Not sure about this.

The relevant part of the code is this:

https://github.com/archimatetool/archi-scripting-plugin/blob/6d4be55648639af1782804620035dd24744191a1/com.archimatetool.script/src/com/archimatetool/script/dom/model/SelectorFilterFactory.java#L43-L52

This means that If the object is a diagram object (visual object) then it returns false, and true if a concept (in the model tree)

@jbsarrodie WDYT?

markbacker commented 1 year ago

But why different behavior for the "*" selector?

The test with an application-component selected on a view gives: Result from .is("*") = false Result from .is("application-component") = true Result from .is("element") = true

Phillipus commented 1 year ago

Because the is function ultimately calls the code I quoted above. The "*" selector only returns true for concepts, views and folders.

I'm not sure whether this should be changed or not.

jbsarrodie commented 1 year ago

@jbsarrodie WDYT?

This is the intended behavior: is() check a collection against a selector. Thus collection.is(selector) returning true implies that collection.filter(same_selector) is not empty.

The intended behavior for "" selector is to return only ArchiMate concepts and "primary" containers (folders and view). The reason for that is that we want some selector to return actionable content and not thousands of visual objects (they can be accessed by other means). That's what "" is for. So a collection containing only visual objects won't match the "" selector, and in this case `is("")` returns false.

In fact, the real question should be why "application-component" (or "element", or any specific selector) works on visual objects. The reason for this is that as soon as you've selected some visual objects (related to ArchiMate concepts), in almost all cases, you'll want to check the type of the underlying concept and not the visual object itself (and same is true for properties), so we've made it easier for people, and in this case jArchi checks against .concept and not the visual object. But the drawback is that we had to make sure that $(selector) (which basically is model.find(selector)) doesn't return also visual objects, so we've implemented it as model.find(selector).filter("*")

markbacker commented 1 year ago

Thanks for the clear explanation. This also helps in understanding the reasoning behind why visual objects do have properties. And true, it does give shorter code.

Having said that, I don't think I agree with this choice, and I don't think it makes it easier for people. For me, this is confusing. I have made many mistakes in using object and object.concept ...

Now I know how to solve the error in my script. I will use the condition if ($(obj).is(selector) || selector == "*")

Phillipus commented 1 year ago

Having said that, I don't think I agree with this choice, and I don't think it makes it easier for people. For me, this is confusing. I have made many mistakes in using object and object.concept

How do you think it could be implemented differently?

markbacker commented 1 year ago

I would prefer that the API is explicit about whether you are working with a visual object or an object.

obj.prop() is only applicable for objects; visual objects don't have properties. To access the object, you have to use the .concept method
Just like only visual objects have the method .fontname().
If you use obj.id and obj.prop(), you can be sure you are getting the values of an object (and if you mistakenly pass a visual object, you get an error on the .prop() method).

And for the .is() method.

.is("*") returns everything
.is("element") return all elements (objects and visual objects)
.is("concept") returns all elements and relations (a bit confusing with the .concept method. A more explicit name would be simply "element and relation")
to explicitly choose for object or visual objects there are also selectors for .is("object") (all objects in the model tree, equal to the "*" selector now) and .is("visual-object"). Combining these with the previous selectors makes all/most selections possible
and a selector .is("diagram-object") to select diagram-model-note, group, etc.

For me this is more consistent and less prone to errors.

archimatetool / archi-scripting-plugin