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

Make "Show all Details" in the diagram settings persistent #103

Closed
cmnrd opened this issue Mar 6, 2023 · 18 comments · Fixed by lf-lang/lingua-franca#2018
Closed

Make "Show all Details" in the diagram settings persistent #103

cmnrd opened this issue Mar 6, 2023 · 18 comments · Fixed by lf-lang/lingua-franca#2018
Labels
enhancement New feature or request

Comments

@cmnrd
Copy link
Contributor

cmnrd commented Mar 6, 2023

I find it quite annoying that the rendered diagrams switch back to "Hide all Details" whenever I edit an LF file. It would be great if the settings are remembered and all details remain expanded once I clicked on "Show all Details" even after I edit the LF program.

@cmnrd cmnrd added the enhancement New feature or request label Mar 6, 2023
@lhstrh
Copy link
Member

lhstrh commented Mar 6, 2023

I'm unsure whether this is a VS Code only problem or whether this also is the case in Epoch, but let me tag @a-sr and @soerendomroes because they would know...

@lhstrh
Copy link
Member

lhstrh commented May 6, 2023

@a-sr or @soerendomroes?

@soerendomroes
Copy link
Collaborator

soerendomroes commented May 6, 2023

It is a VS Code problem that we are aware of.
kieler/klighd-vscode#135

@NiklasRentzCAU
Copy link

I just tried looking into this issue: I can reproduce it with the current released extension in the VS Code marketplace, I cannot reproduce it by launching the language server from LF's and KLighD's current main branches. There everything behaves as expected.
Interestingly, in the main branch setup, all reactors are initially expanded, whereas in the installed extension all are initially collapsed, has something changed there on your side causing this different behavior? Then that may fix it on its own after the next release. Otherwise this different behavior between release and dev-setup does not let me debug this issue. Please re-check after the next release if this still exists, then I will have a look at it agin.

@lhstrh
Copy link
Member

lhstrh commented May 30, 2023

When are you planning to do the next release, @NiklasRentzCAU?

@NiklasRentzCAU
Copy link

The next release will be in conjunction with the first Maven release, so I'll aim for no later than next week as I only wait for us officially claiming the de.cau.cs.kieler domain on Maven Central.

@cmnrd
Copy link
Contributor Author

cmnrd commented May 31, 2023

Great! Please notify us when the release is there, then we can update our gradle configuration to pull from maven central.

@NiklasRentzCAU
Copy link

Just stumbled across this again, just for the documentation here as well: The Maven release was published. To further look into this issue, please re-check this with the new release.

@cmnrd
Copy link
Contributor Author

cmnrd commented Aug 3, 2023

The issue persists.

@soerendomroes
Copy link
Collaborator

soerendomroes commented Sep 21, 2023

As far as I understand, this should be reproducible with the following LF program:

target C

reactor test {
    output b: time

    reaction (startup) -> b {= =}
}

main reactor {
    
    c = new test()
    
}

I try the following to reproduce this: I open the program, expand the reactor, and duplicate the reaction inside the reactor.
As a result, the reactor remains expanded.
Can someone please add steps to reproduce this? The example I added to the referenced issue no longer reproduces the issue with the prerelease version of LF from the marketplace.

Update: It seems to be some race condition and the problem will occur if adding removing a reaction multiple times.

@NiklasRentzCAU
Copy link

Your language server is missing a dependency to de.cau.cs.kieler.klighd.incremental, which is currently necessary for correct animations and keeping track of graph elements during updates. I hacked that into the jar in my LF VS Code extension and it fixed the problem for me. Please include it as a dependency in your build system and it should fix this issue.

cmnrd added a commit to lf-lang/lingua-franca that referenced this issue Sep 22, 2023
cmnrd added a commit to lf-lang/lingua-franca that referenced this issue Sep 22, 2023
@cmnrd
Copy link
Contributor Author

cmnrd commented Sep 22, 2023

Thanks for the pointer! I added the dependency in lf-lang/lingua-franca#2018. Indeed, this seems to stabilize the diagram generation.

However, there is still some "weirdness" with respect to the "Show all Details" option. Consider this program:

target Cpp

reactor Foo {
  input foo: void
  reaction (foo) {==}
}

main reactor {
  foo = new Foo()
  reaction (startup) -> foo.foo {= =}
}

I take the following steps:

  1. Click show all details. The diagram renders correctly.
  2. Remove (or comment) the content of the main reactor.
  3. Add the content back. The diagram shows the Foo reactor fully expanded.
  4. Remove (or comment) the content of the main reactor.
  5. Add the content back. The diagram shows the Foo reactor contracted, hiding its internals.

I tried this several times, and each time the Foo reactor was expanded when adding it back for the first time and then contracted for the second time.

@NiklasRentzCAU
Copy link

For me this is consistent in that the foo-reactor is always collapsed when re-adding the content. Which is what I would expect anyways - the previously expanded graph element is fully removed from the KLighD-structure and thus any expansion state is disregarded. The new "Foo" reactor will be in its default expansion state, which seems to be collapsed in LF.
The inconsistency for you could be explained with Java's Garbage Collector cleaning the weak hash map remembering the expansion state in your code here: https://github.com/lf-lang/lingua-franca/blob/4aa386da34dacaabc557036355183f29aabf0dbb/core/src/main/java/org/lflang/diagram/synthesis/action/MemorizingExpandCollapseAction.java#L55-L56

The "Show all Details" is not an option, but rather an action expanding all current elements. Newly added elements will be collapsed by default. If you want a "show all details" option keeping everything open that would be possible in the synthesis as well, forcing new elements with a new "expanded" default.

@cmnrd
Copy link
Contributor Author

cmnrd commented Sep 22, 2023

Ok, that makes sense. I am Ok with accepting the "weirdness" as a feature and not a bug ;)

Also, thanks for your explanation on options vs. actions. I do think it would make sense for us to introduce an option that controls the default expansion state. Do you have any pointers on how this could be achieved?

@soerendomroes
Copy link
Collaborator

The default expanded state is set here.

@a-sr
Copy link
Contributor

a-sr commented Sep 22, 2023

I implemented an "initially expand all" option in an old fork: lf-lang/lingua-franca@80bf6c9
Feel free to use it.

@cmnrd
Copy link
Contributor Author

cmnrd commented Sep 22, 2023

Thanks! I integrated this change with lf-lang/lingua-franca#2018.

@lhstrh
Copy link
Member

lhstrh commented Sep 22, 2023

Your language server is missing a dependency to de.cau.cs.kieler.klighd.incremental, which is currently necessary for correct animations and keeping track of graph elements during updates. I hacked that into the jar in my LF VS Code extension and it fixed the problem for me. Please include it as a dependency in your build system and it should fix this issue.

Thanks for tracking this down, Niklas!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants