Basic dropdown widget integration

[Frizi]

Pull Request Description

Implements https://www.pivotaltracker.com/n/projects/2539304/stories/184023445

Added a dropdown widget to graph node for all span tree nodes that have tag values present. When an option is selected, the controller receives a partial expression update, which targets specific crumbs of the expression (similar to how edge endpoint updates work).

trim-dropdown.mp4

Important Notes

Right now the dropdown widget is recreated every time the node is edited, including a dropdown option being selected. This causes it to close every time. I wanted to get around that by diffing span trees, but I wasn't able to do it in useful way. Additionally, current implementation of node input expression view heavily relies on being reinitialized from scratch every time. This led to more necessary changes than I was comfortable with for this task. I believe it will be easier to implement it as part of more complete widget support, especially after dynamic data support, as we will have proper widget type information.

Checklist

Please include the following checklist in your PR:

• The documentation has been updated if necessary.
• All code conforms to the
  Scala,
  Java,
  and
  Rust
  style guides.
• All code has been tested:
  • Unit tests have been written where possible.
  • If GUI codebase was changed: Enso GUI was tested when built using BOTH
    ./run ide build and ./run ide watch.

[wdanilo]

This should be understandable by the user (user != power user). This sentence is really hard to parse. Can we make it a little bit more elaborate and easier to understand, please?

[wdanilo]

Instead of such Enum, I'd allow any "widget" to be registered on nodes. Just like visualisations. We should allow for custom widgets in the future (as we allow for custom visualisations). We can leave it for now, but in such a case, we should write in the comment that this is a temporary solution that will be removed in <...> task.

[Frizi]

I'm going to add a note in the comment in this specific case. But in general I don't like putting comments about uncertain future into the code. The current dynamic widgets design doc lists all initial widget types explicitly, so this is why I went with an enum to begin with, as that was the pragmatic choice.

Given that we have dynamic visualizations, I agree that this is likely to change at some point. But I believe we don't yet have any specific tasks planned for that.

[wdanilo]

The design doc lists all the things in order to make it easy to understand. However, the design in the code should be generic, as we know that we will have user-defined widgets sooner than later. Changing such a fundamental assumption later would make a lot of changes in the code, so it's better to do it in the code beforehand. I'd like to make it the extensible way before all the widgets tasks are finished, so just please take care of that when designing the arch here :)

[JaroslavTulach]

I got the static drop down widgets working on the Text.trim example. Selecting a value for an argument works, but how do I "unselect an argument value"? In the following picture

I have got wrong value for what (tracked in pivotal) - shouldn't the user be able to click and revert the what value back to default?

Other than that, I am really glad the drop down feature is coming. It really improves the usability of the IDE.

[hubertp]

I've done similar QA and have similar comments to Jaroslav - essentially I would like to come back to the default value without deleting the node completely and starting from scratch. How would I do that?

[hubertp]

Not a Rust expert but I find this explicit return rather unexpected. I preferred the previous style even if it was more verbose.

[Frizi]

I've seen and used this specific pattern (wrapping match in Some and returning None in specific branch) many times before, my impression is that it is fairly common. But I agree that for short match blocks like this it is probably not worth. In this specific case, it is allowing the expression to fit into a single line. Given that I'm going to add another parameter here anyway, I might split it into multiple lines and get back to the fully implicit return form.

[hubertp]

@Frizi are the names somehow specially formatted already or is that customized for Location?

Notice that in the above example we insert fully qualified names which later leads to errors.

[JaroslavTulach]

@Frizi are the names somehow specially formatted already ... Notice that in the above example we insert fully qualified names which later leads to errors.

According to the original design the idea was:

Each entry in the tagValues array shall represent a valid Enso value.
E.g. if the entry is evaluated as an Enso code, it shall be valid. That means each value
can represent a number, a quoted string or fully qualified name
of an atom constructor applicable as an argument to the function invocation.
The IDE is expected to deal with the entries:

• Shorten them appropriately for user presentation.
• Display just parts of the text.
• Decide which of the fully qualified names to import.

[Frizi]

My understanding is that the fully qualified names delivered in tagValues should always be valid to insert as is into the code. The instances where this is currently not true are a bug. For simplicity, I don't want to modify imports. The visual shortening is not done yet, as the rendered code line cannot differ from actual in-file code (except minor trimming like for skip/freeze macros). Eventually we will move away from displaying the raw code, and there will be widgets rendered instead. It is technically possible to display the names shortened inside the dropdown already. I'd still prefer to do it together with the in-node widget and use the same logic for both.

It is not possible to deselect the dropdown option right now after selecting it, but It sounds fairly easy to add for arguments that have hasDefault set to true. I'll add it. Without it, you had to edit the node and remove the argument text manually (with ctrl-click on the node).

[hubertp]

Been playing with it a bit more and it appears to slightly break when more parameters are present ?

Notice how names of parameters are now gone but the selection dot for dropdowns are there

[Frizi]

@hubertp the parameters are not gone. There are _ symbols in place. The dots slightly cover them so it's not very clear, but those are temporary anyway. That means there indeed are three arguments that can be autocompleted there.

[hubertp]

@hubertp the parameters are not gone. There are _ symbols in place. The dots slightly cover them so it's not very clear, but those are temporary anyway. That means there indeed are three arguments that can be autocompleted there.

I see, yeah, I had to zoom in the screenshot. But why are there _ symbols in place rather than the names of parameters?
Like here

[Frizi]

Good question. The dropdowns fills in the argument AST position with the given value. It is always treated as a positional argument. That means all arguments in front of it are automatically filled with _, as you need something there to get the argument position right.

Using argument names directly leads to all sorts of wrong parsing. I don't think that this syntax is in well supported in IDE now, and certainly it is out of scope of this PR.

[hubertp]

When removing the dropdown could we come back to what was before (i.e. not _)? Or is that due to the same problem as discussed above?

Kazam_screencast_00005.mp4

[Frizi]

It is similar reason. Outright removing an argument in first position isn't possible. I am not sure why but it is explicitly disallowed by the spantree logic. This behaves exactly the same as disconnecting an edge would. When a function has more arguments and you unselect the last one, it will be completely removed.