search

archimatetool / archi

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

#809: Six new connector label positions #810

Closed Santeri-Salminen closed 2 years ago

Santeri-Salminen commented 2 years ago

Proposal for adding new connector label positions.

Phillipus commented 2 years ago

Thanks for the PR, it seems to be well organised.

I have some minor comments:

  1. AbstractDiagramConnectionFigure.java - remove the unnecessary import com.archimatetool.model.IProperty;
  2. ArchiConnectionEndpointLocator - no need to change Connection to AbstractDiagramConnectionFigure
  3. 'ArchiConnectionEndpointLocator` the new constructor should be added rather than replacing the original one in case a third-party is using it (it's public)
  4. The label strings are inconsistent - perhaps better would be "Source Above", "Source Center", "Source Below", "Middle Above", "Middle Center", "Middle Below", "Target Above", "Target Center", "Target Below".
  5. The licence header in ArchiConnectionMiddleLocator should be as in AbstractDiagramConnectionFigure

ArchiConnectionMiddleLocator extends ConnectionLocator but we use ArchiConnectionEndpointLocator (replacement for ConnectionEndpointLocator) for some cases and I'm wondering why we use that special class, and if the new ArchiConnectionMiddleLocator might be missing something from it? @jbsarrodie ?

jbsarrodie commented 2 years ago

Hi,

(related to #809)

Well, before looking at this code proposal, I must admit that I'm not in favor of such change. Most of what you provide can be done by tuning the label expression (using some newlines or ${wordwrap} for example). So I think this new set of choices make it more complex for the user in fact. IMHO, we should either stay are we are now (start | middle | end) or implement the real thing (movable label).

Phillipus commented 2 years ago

Hi,

(related to #809)

Well, before looking at this code proposal, I must admit that I'm not in favor of such change. Most of what you provide can be done by tuning the label expression (using some newlines or ${wordwrap} for example). So I think this new set of choices make it more complex for the user in fact. IMHO, we should either stay are we are now (start | middle | end) or implement the real thing (movable label).

OK, that's a fair comment.

jbsarrodie commented 2 years ago

or implement the real thing (movable label).

As shared in some occasion, I think we could leverage note and label expression. It is already possible to create a note with transparent border and background, connect it to some concept, and use a label expression to display the name of the concept. We have almost all pieces needed to create movable labels. I think the only missing pieces are: being able to make the connection transparent, and have a way to reference the concept's label and not just the concept's name.

Santeri-Salminen commented 2 years ago

Thanks for the comments! My main driver was the Middle positioning because it's always on top of the connector, and effectively blocks the visibility of short connectors. Label expression doesn't help here (at least to my understanding) because the label has explicit background color instead of being transparent, so newlines will also hide the connector line.

I agree that my current proposal makes the drop-down menu too complex. What about having a separate drop-down menu just for vertical positioning? The current position menu would remain unchanged, and the new menu wouldn't need to be used at all if the default vertical positioning is ok. It could have options Default, Above, Center, and Below, for example.

Freely movable labels would be great, of course, but that's not something I'm able to implement.