lf-lang / vscode-lingua-franca

Lingua Franca extension for Visual Studio Code
Other
4 stars 3 forks source link

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

Closed cmnrd closed 10 months ago

cmnrd commented 1 year ago

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.

lhstrh commented 1 year ago

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 commented 1 year ago

@a-sr or @soerendomroes?

soerendomroes commented 1 year ago

It is a VS Code problem that we are aware of. https://github.com/kieler/klighd-vscode/issues/135

NiklasRentzCAU commented 1 year ago

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 commented 1 year ago

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

NiklasRentzCAU commented 1 year ago

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 commented 1 year ago

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

NiklasRentzCAU commented 1 year ago

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 commented 1 year ago

The issue persists.

soerendomroes commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

Thanks for the pointer! I added the dependency in https://github.com/lf-lang/lingua-franca/pull/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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

The default expanded state is set here.

a-sr commented 11 months ago

I implemented an "initially expand all" option in an old fork: https://github.com/lf-lang/lingua-franca/commit/80bf6c968c57310752bc5c931cac88bbabaf5c3e Feel free to use it.

cmnrd commented 11 months ago

Thanks! I integrated this change with https://github.com/lf-lang/lingua-franca/pull/2018.

lhstrh commented 11 months ago

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!!