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

[RFCs]: Magnition Proposal - Localised Layout Options #4

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

Conversation

mkhubaibumer
Copy link

@mkhubaibumer mkhubaibumer commented Feb 9, 2024

rendered

This PR proposes RFCs for the following feature:

  • Localized Layout Options

This PR proposes RFCs for the following two features:

- Localized Layout Options
- Template typename passing with inheritance

Signed-off-by: mkhubaibumer <[email protected]>
@soerendomroes
Copy link

Hi, I suggest one RFC per PR otherwise the discussion might be hard to follow.

@soerendomroes
Copy link

Regarding localized layout options:

I think the main interface for this might be the layout options view (not the sidebar) for klighd-vscode kieler/klighd-vscode#83. This view should have the possibility to serialize the changes, which would be implemented via a service interface on the server side.

Whether the option serialization would take a separate file could be language specific and it would work as long as the option file is loaded in the synthesis.

My take would be to contribute to klighd-vscode first and see whether a simple tree view with all layout options set is enough for your use case. If it is not, the client could be extended for a different kind of interaction using the klighd-vscode LSP messages build for the new view.

@cmnrd
Copy link
Contributor

cmnrd commented Feb 22, 2024

Thanks for submitting this PR @mkhubaibumer! Could I ask you to split the two RFCs into two PRs? This makes it a lot easier for us to review and discuss them individually. Also, it seems like a bunch of additional files were added to the diff of this PR.

@mkhubaibumer
Copy link
Author

@soerendomroes @cmnrd sure. I'll split this in 2 PRs asap

@mkhubaibumer mkhubaibumer changed the title [RFCs]: Magnition Proposal [RFCs]: Magnition Proposal - Localised Layout Options Feb 23, 2024
@mkhubaibumer
Copy link
Author

@cmnrd @soerendomroes @lhstrh I've split the PR into 2.
Please review and share your feedback on the proposal

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! I think this is an interesting idea. Do you have a concrete idea how this could be integrated with the LF language? One obvious solution that comes to my mind is adding annotations within LF code using our attribute syntax. So we could expose some (or all) of the available options as attributes and allow for local overrides.

Another important question is feasibility on the KIELER side. @soerendomroes @a-sr Does the framework only consider global rendering options, or is there support for local options too?

Also, I wonder if filtering could be an alternative solution to your problem. If you can somehow select which specific reactors to render, you could better select the appropriate rendering options. Would that make sense?

@mkhubaibumer
Copy link
Author

@cmnrd that's an interesting idea! But would lingua-franca be open to generating annotations in code by changing layout option for certain reactor instantiations

Its much easier to do it from diagram then write these annotations in code in my opinion

@cmnrd
Copy link
Contributor

cmnrd commented Feb 23, 2024

That is something we should put up for discussion in the broader team, but personally, I am open to that. Doing it in the code and also updating the code automatically certainly has some downsides, but I think the main advantage would be that we still have the diagrams representing only the code and there would be no external metadata that gets out of sync. However, before we get into the details of this, I think it would be most important to understand the feasibility on the side of the diagram backend.

@petervdonovan
Copy link
Contributor

Another important question is feasibility on the KIELER side. soerendomroes a-sr Does the framework only consider global rendering options, or is there support for local options too?

I agree that this is a big open question and also not a question that I am well-prepared to answer quickly.

But would lingua-franca be open to generating annotations in code by changing layout option for certain reactor instantiations

For this the big open question is not whether we can add the annotations in the code (we can do that, it is easy) but rather what the graphical UI would look like. UI design is a complex issue that is outside my expertise and I am worried that it could get pretty difficult to support this without causing a lot of confusion for non-expert users.

@a-sr
Copy link

a-sr commented Feb 26, 2024

In our diagram framework, we distinguish two types of configuration options. Synthesis options affect how the diagram is created (e.g. if state variables are visible); these are located in the side bar. Layout options are set implicitly by the synthesis and affect only the layout/arrangement of the diagram.
For the latter, LF already supports overriding the defaults (set by the synthesis or ELK) by annotating model elements (lf-lang/lingua-franca#1951). E.g. @layout(option="port.side", value="NORTH")

Yet, if you are interested in synthesis options here, this will require an adjustment of the LF diagram synthesis because it treats the settings in the side bare as a global configuration, e.g. if (getBooleanValue(SHOW_PORT_NAMES)). This would require an adjustment to something like if (getBooleanValue(SHOW_PORT_NAMES) || reactor.hasAnnotation("portnames")). In short: yes, it is feasible.

In terms of usability, if writing annotations is not desired, one could think of adding them via a context menu for model elements, similar to structure-based editing. However, this is not yet support in Klighd but @soerendomroes could say more to this topic.

@cmnrd
Copy link
Contributor

cmnrd commented Feb 27, 2024

Interesting. Thanks for the explanation. I actually completely forgot about the @layout annotation. @mkhubaibumer do these in code annotations address your use-case, or do you require GUI support? Are the layout options sufficient, or would you also need support for synthesis options?

@soerendomroes
Copy link

We are planning to add structure-based editing into Klighd and will rework the interactive layout constraint framework, which was a very similar workflow to what you may be envisioning. For me, the question remains, whether layout options are enough for you or whether you want to change synthesis options

@lhstrh
Copy link
Member

lhstrh commented Mar 11, 2024

@cmnrd that's an interesting idea! But would lingua-franca be open to generating annotations in code by changing layout option for certain reactor instantiations

Its much easier to do it from diagram then write these annotations in code in my opinion

The advantage of annotations is that it gives you a way to serialize (i.e., save) layout options. If not done that way, what would be an appropriate place to save them? Do sprotty or klighd perhaps already have an established mechanism for this, @soerendomroes?

@soerendomroes
Copy link

@lhstrh We are currently serializing them if possible. This might be in the next klighd/klighd-vscode release, maybe in the one shortly after that. For interactively setting layout constraint to constrain the topology of the layered layout. We are currently we are trying to serialize a constraint if the given language has a serializer, otherwise, we only work on the view model (KGraph) such that this changed will not be persisted and will be gone on reload.

The synthesis options are usually saved in the browser for a corresponding language (which is the way VS Code usually handles things).

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.

6 participants