Improve dropdown arrows placement #9909

vitvakatu · 2024-05-09T13:30:55Z

Pull Request Description

Hilariously tricky thing to implement. Besides placing arrows below constructor names, we also position them under values in argument placeholders.

We also added a new feature to the widget tree: the allowAsLeaf property. See provided documentation.

working.arrow.placement.mp4

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

vitvakatu · 2024-05-09T13:32:46Z

app/gui2/src/components/GraphEditor/widgets/WidgetSelection.vue

+      // Find the top-most PropertyAccess, and return its rhs id.
+      // It will be used to place the dropdown arrow under the constructor name.
+      let node = props.input.value
+      while (node instanceof Ast.Ast) {
+        if (node instanceof Ast.PropertyAccess) return node.rhs.id
+        if (node instanceof Ast.App) node = node.function
+        else break
+      }


Sometimes constructors have arguments, but call resolution happens only in WidgetFunction, which has a lower priority than WidgetSelection. I think this code will cover the majority of cases when we have a constructor call, but perhaps there are some common cases that I missed.

Sometimes constructors have arguments, but call resolution happens only in WidgetFunction, which has a lower priority than WidgetSelection. I think this code will cover the majority of cases when we have a constructor call, but perhaps there are some common cases that I missed.

It will also catch all normal function calls (for example, in node opreator1.select_columns [operator2.first] the arrow will be under first - I'm not sure if it's expected. On the other hand, it's rather an advanced usage).

Yep, that’s the case. I don’t think we can solve it without expensive operations to determine if we have a constructor call or a function call. Perhaps we can analyze each node on a higher level, preparing all the necessary information in advance and only fetching it in WidgetFunction, but for now I suggest to leave it as it is.

vitvakatu · 2024-05-09T13:35:49Z

app/gui2/src/components/GraphEditor/widgets/WidgetSelection.vue

+    <Teleport v-if="arrowLocation" :to="arrowLocation">
+      <SvgIcon v-if="isHovered" name="arrow_right_head_only" class="arrow" />
+    </Teleport>
+    <SvgIcon v-else-if="isHovered" name="arrow_right_head_only" class="arrow" />


You may wonder why to duplicate instead of using <Teleport :disabled='...'>. The reason is that :to should point at a valid mounted reference at the time of its mounting. Here, however, the destination would only be available after mount. In other words, you can’t have <Teleport :disabled="bla" :to="undefined">, Vue will bail out. I choose not to state it in the code, as the comment will be copied to application html tree, which I find ugly.

I choose not to state it in the code, as the comment will be copied to application html tree, which I find ugly.

In production builds, the comments are removed: https://vuejs.org/api/application.html#app-config-compileroptions-comments If you want, you may set the config to remove also in dev builds, but perhaps comments may be useful there?

In either case, please put here a comment.

Thanks, good to know, added a comment there.

vitvakatu · 2024-05-09T13:40:24Z

app/gui2/src/providers/widgetRegistry.ts

@@ -205,6 +205,14 @@ export interface WidgetOptions<T extends WidgetInput> {
  // A list of widget kinds that will be prevented from being used on the same node as this widget,
  // once this widget is used.
  prevent?: WidgetComponent<any>[]
+  /** Allow to select this widget as the leaf of the widget tree for particular widget input.
+   * `true` by default. The widget marked with `false` *must* have a child,


The second sentence is kinda obvious, but I still want to write it down just in case.

farmaazon

I like the implementation, but I think the docs/names should be improved.

farmaazon · 2024-05-10T07:34:03Z

app/gui2/src/components/GraphEditor/widgets/WidgetSelection.vue

+      // Find the top-most PropertyAccess, and return its rhs id.
+      // It will be used to place the dropdown arrow under the constructor name.
+      let node = props.input.value
+      while (node instanceof Ast.Ast) {
+        if (node instanceof Ast.PropertyAccess) return node.rhs.id
+        if (node instanceof Ast.App) node = node.function
+        else break
+      }


Sometimes constructors have arguments, but call resolution happens only in WidgetFunction, which has a lower priority than WidgetSelection. I think this code will cover the majority of cases when we have a constructor call, but perhaps there are some common cases that I missed.

It will also catch all normal function calls (for example, in node opreator1.select_columns [operator2.first] the arrow will be under first - I'm not sure if it's expected. On the other hand, it's rather an advanced usage).

farmaazon · 2024-05-10T07:37:53Z

app/gui2/src/components/GraphEditor/widgets/WidgetSelection.vue

+    <Teleport v-if="arrowLocation" :to="arrowLocation">
+      <SvgIcon v-if="isHovered" name="arrow_right_head_only" class="arrow" />
+    </Teleport>
+    <SvgIcon v-else-if="isHovered" name="arrow_right_head_only" class="arrow" />


I choose not to state it in the code, as the comment will be copied to application html tree, which I find ugly.

In production builds, the comments are removed: https://vuejs.org/api/application.html#app-config-compileroptions-comments If you want, you may set the config to remove also in dev builds, but perhaps comments may be useful there?

In either case, please put here a comment.

farmaazon · 2024-05-10T07:41:50Z

app/gui2/src/components/GraphEditor/widgets/WidgetSelectionArrow.vue

+
+<template>
+  <div ref="teleportTarget" class="WidgetSelectionArrow">
+    <NodeWidget :input="innerInput" allowEmpty />


Why do we allowEmpty here, while also not allowing as leaf?

Good catch, an artifact from previous implementation

app/gui2/src/providers/__tests__/widgetRegistry.test.ts

farmaazon · 2024-05-10T07:54:34Z

app/gui2/src/providers/widgetRegistry.ts

+  /** Allow to select this widget as the leaf of the widget tree for particular widget input.
+   * `true` by default. The widget marked with `false` *must* have a child,


I think the terminology is not quite right here: in my understanding of child term, a widget could actually have children, but still be discarded when allowAsLeaf is false.

The best example is WidgetHierarchy - usually picked as last resort for AST tree. This tree may still have children, but those will make different input for widgets, so the Hierarchy's input will match no other widget - making it being discarded when allowAsLeaf will be false.

I think this could be renamed to wrapperOnly or something like that, and the meaning is "if true, this widget will be matched only when at least one another widget is guaranteed to be matched for the same input". Or better :)

I think your understanding is correct, and so is my comment – the important words of the first sentence are for particular widget input. I don’t think wrapperOnly and inversion of boolean have a strong preference here, so I left it as it is. wrapperOnly has the same problem – if widget is wrapperOnly, it still can be selected as a non-wrapper if it has no children with the same input. However, I liked your explanation if true, this widget will be matched only when at least one another widget is guaranteed to be matched for the same input; and reused it. (inversing the boolean)

app/gui2/src/providers/widgetRegistry.ts

farmaazon · 2024-05-10T08:02:27Z

app/gui2/src/providers/widgetRegistry.ts

+      // If we found a perfect match, we can return immediately, as there can be no better match.
+      // If we found a leaf match, we can safely return – there will be another leaf later down the tree.
+      // One caveat here is that if the widget marked with `allowAsLeaf = false` has no children. Then we
+      // can pick it here, and it will effectively become leaf.


I would somehow merge the first two sentences. And I have an impression, that the caveat does not explain too much - after all, the allowAsLeaf is about guarantees, the widget may do whatever it wants.

I don’t think merging is possible without murdering the grammar, but I removed the part about caveat and rephrased it.

vitvakatu added 8 commits April 22, 2024 17:39

wip

f298588

wip

0db963f

wip

f9b1307

Merge branch 'develop' into wip/vitvakatu/arrows-positions

7fdcbff

wip

9687a37

Finish the implementation

241bcde

minor fixes

a2f5624

formatting and linting

aba363a

vitvakatu added the -gui label May 9, 2024

vitvakatu self-assigned this May 9, 2024

formatting

f9887e6

vitvakatu commented May 9, 2024

View reviewed changes

vitvakatu marked this pull request as ready for review May 9, 2024 13:40

vitvakatu requested review from Frizi, farmaazon and kazcw as code owners May 9, 2024 13:40

vitvakatu added the CI: No changelog needed Do not require a changelog entry for this PR. label May 9, 2024

farmaazon approved these changes May 10, 2024

View reviewed changes

vitvakatu added 3 commits May 10, 2024 14:59

Address review comments

90e3d21

Merge branch 'develop' into wip/vitvakatu/arrows-positions

7c4b729

Fix bug with props not being updated

6a2aef2

vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label May 10, 2024

mergify bot merged commit 4376a5a into develop May 10, 2024
37 checks passed

mergify bot deleted the wip/vitvakatu/arrows-positions branch May 10, 2024 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve dropdown arrows placement #9909

Improve dropdown arrows placement #9909

vitvakatu commented May 9, 2024

vitvakatu May 9, 2024

farmaazon May 10, 2024

vitvakatu May 10, 2024

vitvakatu May 9, 2024

farmaazon May 10, 2024

vitvakatu May 10, 2024

vitvakatu May 9, 2024

farmaazon left a comment

farmaazon May 10, 2024

farmaazon May 10, 2024

farmaazon May 10, 2024

vitvakatu May 10, 2024

farmaazon May 10, 2024

vitvakatu May 10, 2024

farmaazon May 10, 2024

vitvakatu May 10, 2024

		/** Allow to select this widget as the leaf of the widget tree for particular widget input.
		* `true` by default. The widget marked with `false` must have a child,

Improve dropdown arrows placement #9909

Improve dropdown arrows placement #9909

Conversation

vitvakatu commented May 9, 2024

Pull Request Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farmaazon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment