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

RFC: for color of edges and ports. #3

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

soerendomroes
Copy link

This should also serve as a demonstration to show what a short RFC that is mainly a discussion about a few lines of code. Please have mercy on my spelling (even though it is mentioned in the guide, I did not carefully consider what to put in here).

Notes to the process:

I spend a lot of time reading the documentation. Maybe we would shorten it a little?

@soerendomroes soerendomroes changed the title RFC for color of edges and ports. RFC: for color of edges and ports. Feb 8, 2024
@soerendomroes
Copy link
Author

I am a little unsure how such an RFC would continue.

In this case, it would be reasonable to just implement a solution and display the outcome to visualize how this would affect the general look and feel of the diagram.

Are there any general arguments against options for more color?

@soerendomroes
Copy link
Author

@a-sr Do you mind taking a look at this?

@lhstrh If I remember correctly, I should tag anyone that might be interested in this RFC and make them a reviewer. Currently, I think I do not have the rights to add reviewers to this PR. Should I just tag people, should I be able to add reviewers, or is either annoying?

@cmnrd cmnrd requested a review from a-sr February 22, 2024 09:25
Copy link
Contributor

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to write the first RFC (even though the process is not established yet)! I think this is a great topic for an RFC.

I don't fully understand the proposal yet. Is the idea to have a fixed color for all output ports and a fixed color for all input ports? Or can we change color per port? How does color work in conjunction with connections? Is this just about coloring the highlighted connection, or about assigning colors to connections generally? I think a sketch of the diagram would be really helpful for understanding the proposal.

text/0000_colored-input-output.md Outdated Show resolved Hide resolved
text/0000_colored-input-output.md Outdated Show resolved Hide resolved
@cmnrd
Copy link
Contributor

cmnrd commented Feb 22, 2024

@lhstrh If I remember correctly, I should tag anyone that might be interested in this RFC and make them a reviewer. Currently, I think I do not have the rights to add reviewers to this PR. Should I just tag people, should I be able to add reviewers, or is either annoying?

I think tagging people is fine. The shepherd can add reviewers explicitly if needed (I'll take on this role for this PR). Also, we can only add organization members as reviewers, so will have to resort to tagging for external reviewers anyway.

Also tagging @mkhubaibumer

@cmnrd cmnrd self-assigned this Feb 22, 2024
@soerendomroes
Copy link
Author

The edge selection style can be changed by either changing what the boldLineSelectionStyle does or by creating a new method to set the desired styling.
Maybe something like this:
_kRenderingExtensions.setSelectionForeground(r, Colors.DODGER_BLUE);

cdn_cache_demo_pop_selection_color

Note that this will only highlight the corresponding elements that were clicked.
Additionally, highlighting other components such as the corresponding port is possible and could be implemented in the synthesis by changing the callback method to do something based on the eContainer inside the handled KRendering.

@soerendomroes
Copy link
Author

The LF-Logo uses orange (rgb(247, 148, 30) if I am not mistaken) and a blue that may be too dark rgb(0, 49, 100). In the image above I used dodger blue (which is the SCCharts highlighting color if I am not mistaken) rgb(30, 144, 255).

@soerendomroes
Copy link
Author

In VS Code we could also use the highlighting color of the current color scheme. I think that this would only work, if the diagrams would use the VS Code color scheme, so this might be future work here.

@soerendomroes
Copy link
Author

Here is a small list of all diagram elements that I know of that can be clicked and need interaction with suggested interactions:

startup/shutdown/timer/reset

  • Highlight all outgoing edges together with element

reaction/reaction label/mutation/mutation label

  • Mark incoming edges?

reactors/reactor label

  • Just highlight the reactor

action/action label

  • Mark all incoming edges

port/port label

  • Highlight all connected edges

edge

  • Highlight all edges/ports that it leads to only stopping on non-expanded elements

model models

  • Just as reactors/edges

reaction order edge

  • Just as a normal edge?

dependency edge:

  • Just as a normal edge?

Please add additional elements or selection interactions that I missed or that you have a different opinion about.

@soerendomroes
Copy link
Author

@OmerMajNition do you have further suggestions for this?
@lhstrh I remember that you wanted to visualize some upstream dependencies using the selection highlighting. What exactly do you want to visualize?

@lhstrh
Copy link
Member

lhstrh commented Jun 26, 2024

@lhstrh I remember that you wanted to visualize some upstream dependencies using the selection highlighting. What exactly do you want to visualize?

My idea was that when you interact with (e.g., Alt + Click) an element (e.g., a reaction, a connection, a port) you would could light up all components that exert causal influence on it or vice versa -- basically all causality chains that it is a part of. This would give you a sense of what other parts of the system it interacts with. I realize, however, that this could quickly become complicated/overwhelming as this would conservatively have to include paths that go through subsequent reactions in the same reactor (because these could interact via state variables. I guess we should ask ourselves whether we care more about soundness or completeness of the feature...

@OmerMajNition
Copy link

Agree with Marten, changing color of whole causality chain would get complex. One simple thing that we can allow is, user should be able to select multiple lines at a time (ctrl + click a connection). Next selection shouldn't cancel previous selection. This would let users manually select desired data path.

@soerendomroes
Copy link
Author

Question from @cmnrd : Should we add an option for "Colored highlightling" to the sidebar?

Currently, I think this is not necessary, since I do not know when we would need a highlighting that is not recognizable (since it is only a thick black stroke/line) instead of a black/gray stroke/line (which may already be thick).
Moreover, I want to reduce the number of "just in case" options in the sidebar.

Has anyone opinions on this?

@soerendomroes
Copy link
Author

To implement this, we need a way to get the the actual element associated with a Rendering that we clicked on:

EObject element = r.eContainer();

And depending on whether this is a KEdge or KPort or whatever (KLabel and KText currently have no selection highlighting), we want to do highlighting.
What I am currently missing implementation wise is how we can get the KEdge or better the rendering for the KEdge that is connected to the port we want to highlight.
When debugging this, it seems that the edges of a port are always null.
Maybe @NiklasRentzCAU (when he is well again) or @a-sr knows how to do this.

@NiklasRentzCAU
Copy link

To implement this, we need a way to get the the actual element associated with a Rendering that we clicked on:

EObject element = r.eContainer();

And depending on whether this is a KEdge or KPort or whatever (KLabel and KText currently have no selection highlighting), we want to do highlighting. What I am currently missing implementation wise is how we can get the KEdge or better the rendering for the KEdge that is connected to the port we want to highlight. When debugging this, it seems that the edges of a port are always null. Maybe @NiklasRentzCAU (when he is well again) or @a-sr knows how to do this.

According to my knowledge this is exactly how you should be able to get the (incoming and outgoing) edges of a port, via its edges field. This is derived automatically by the KGraph Ecore model and should be opposite to the edges' sourcePort resp. targetPort property, respectively. Have you looked into the model to verify that the one direction works and the other does not?

The rendering of that edge should then be accessible from that edge, so that should not be your problem.

@soerendomroes
Copy link
Author

I am doing this wrong. I am only changing the static selection style of elements defined in the synthesis, why is ok for the standard approach if I want to highlight the element that I clicked.

To highlight elements that were not clicked, I need to catch and forward the SelectAction to the server and then handle which elements should really be selected based on this click.

@soerendomroes
Copy link
Author

soerendomroes commented Aug 6, 2024

I suggest implementing this the following way:
Since we do not just want to highlight whatever we clicked on, we need to add interactivity. This means that there should be an action that is triggered on click on each element. Sprotty already has this SelectAction implemented. Hence, we just need to catch it on the server side to have something handle it.
This and this guide exemplary show what needs to be implemented to have an action on the VS Code and on the LS side. We, however, only need the server side. Hence, we need to implement a ISprottyActionHandler and register it. Registering the implementation class, e.g. SelectionHandler as a service interface as the detailed in the guide, allows implementing a handle method that can create the highlighting.

@NiklasRentzCAU has a better idea:

We can add an action as it was done for the SelectRelatedAction here.

This means we add and implement an action that is added to all elements that should be selectable in the synthesis and register it here together with the other action.

What is missing it how one could alt+click to select multiple elements. The implementation in osgiviz only selects one element and resets the selection on click.

The switch case should for LF not look at the view model element but has to consider what "real" LF model element might be inside to it to find the correct elements.

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

Successfully merging this pull request may close these issues.

5 participants