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

Fix list editor panics during insertion #6540

Merged
merged 18 commits into from
May 17, 2023

Conversation

Frizi
Copy link
Contributor

@Frizi Frizi commented May 3, 2023

Fixes #6522

  • Fixed occasional panics when inserting or moving elements around.
  • Improved mouse events handling in list editor to behave correctly during box selection.
  • Removed vectorEditor feature flag, enabling the list editor permanently.
  • Replaced node background shape with Rectangle and removed unnecessary separation between hover area and background. Switched node hover state to use new mouse events system.
  • Adjusted size of hover area of the list editor such that it is harder do accidentally insert a new element and easier to drag the nodes around.
  • Fixed mouse down event propagation inside list editor when clicking without dragging. This allows dropdowns nested within list items to work correctly.
    image
    image
    image

Important Notes

The mouse handling changes involve an unfortunate huge hack, where we enable mouse events on the mouse shape during box selection. That way we know for sure that no other shape will be able to receive mouse enter event. Then the list editor widget is modified to only actually respond to events when its background is hovered. We will definitely want a more proper way to handle mouse event contention, but it's definitely out of scope for current bugfixing.

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,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@Frizi Frizi added the CI: No changelog needed Do not require a changelog entry for this PR. label May 3, 2023
@Frizi Frizi marked this pull request as ready for review May 4, 2023 12:02
app/gui/view/graph-editor/src/component/node.rs Outdated Show resolved Hide resolved
Comment on lines -530 to +513
let is_close_y = pos.y > -gap && pos.y < size.y + gap;
let is_close_y = pos.y > 0.0 && pos.y < size.y;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change? The gap was there to display + button when the mouse is cldse to the edge of lit editor. Without this change it will not be visible, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is visible, the area is just intentionally made a little smaller. The gap was extending the active bounding box in both X and Y axis. Y axis extension wasn't desirable, as it made the insertions way too easy to do accidentally, especially when attempting to drag the node.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, but dragging the node by clicking above / below list editor should be hardly supported / unsupported. I mean, there is just a few pixels for that and assuming someone would be able to find these pixels is bad. I prefer the drag areas to be bigger to easier initiate dragging / insertion of elements. Of course, currently there is problem with node dragging because we don't have the icons on the left, but it is not solvable by making this are smaller IMO.

Copy link
Contributor Author

@Frizi Frizi May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will adjust that when working on new node design, as we will have slightly more area to work with. For now, it behaves much better that way. It is still not hard to drag the elements, you simply drag the text anywhere. Also, using gap to extend drag area vertically was a little surprising to me. I would prefer that to be a separate configuration input, if we need one.

@Frizi Frizi requested a review from wdanilo May 5, 2023 00:36
@jdunkerley
Copy link
Member

Quick feedback from trying this branch. The Vector Dropdowns now work :).

But has a detrimental affect on being able to connect links.

2023-05-05_10-25-44.mp4

@jdunkerley
Copy link
Member

Stuff from my trying to use this build today:

  • As above dragging to establish a connection is hard.
  • Ctrl clicking into edit a node, then moving the cursor by clicking doesn't seem to work.
  • If a Vector is pointed at a non-vector (e.g. single value or a function) then you just get a dot.

image

- Seemed to get extra dots on things that aren't vectors.

image

- The + to add the first item doesn't seem to work. - Comma's seems to be sometimes visible and sometimes not.

image

@Frizi
Copy link
Contributor Author

Frizi commented May 8, 2023

I have fixed the issues with edge dragging. My previous event resume implementation have broken the mouse down events in new event system. Now it works again and nodes can be connected as expected, including inside list editor. Ctrl clicking/edit mode is also fixed with that.

image

@vitvakatu
Copy link
Contributor

QA Report 🔴

  1. Adding new elements to the empty vector is not working – the grey circle is unclickable.

Also, split does not have the first argument inside the array, see the video:

2023-05-09.17-15-56.mp4
  1. When dragging the edge over the vector editor, the cursor behaves weirdly, changing the shape to plus when between placeholders. Clicking on a free space with a plus cursor uses the nearest placeholder, but removing the connection also removes the element from the vector. It is asymmetrical behavior, I think dropping a new element with a plus shape should add the element, not reuse the nearest one.
    Moreover, it requires exceptionally gentle positioning of the mouse pointer – a slight movement of a few pixels to the side completely changes the target port, which can confuse users. ("Damn, I hate this pixel-hunting just to connect the edge to the element").
2023-05-09.17-50-18.mp4

@sylwiabr
Copy link
Member

@Frizi what is the status? I would love to test it :)

Frizi added 8 commits May 15, 2023 16:20
…same node without recursion.

Better logic for determining when to use list editor and type-based inference for its default value. Improved hover areas for element insertion points, removed button shape in empty list.
@Frizi Frizi force-pushed the wip/frizi/list-editor-stabilization branch from e51737c to a08f7ac Compare May 15, 2023 14:20
@vitvakatu
Copy link
Contributor

QA Report 🔴

In general, looks much nicer. I like how dropping the edge to the vector looks and works now.

There are still issues that I spotted, but they might be fixed separately, I suppose:

  1. On the node table.rename_columns Map.from_vector, the Map.from_vector part is not visible.
Screenshot 2023-05-15 at 20 39 41
  1. Adding elements to the vector after Map.from_vector adds Boolean.True. That is probably incorrect (it's probably an engine issue, though).
  2. Connecting the edge to this vector is hard. It's hard to find a free place. It seems like the free place is right on top of the square bracket. That seems weird. In the video, you can see that I try to target that point for a very long time. Again, pixel-hunting is depressing.
2023-05-15.20-42-49.mp4
  1. Sometimes (almost every time for me), dropping the edge there adds two elements.
2023-05-15.20-43-21.mp4
  1. After recording the previous video, I removed the first element of the vector, and the cursor got stuck in the red cross state. Hovering the node again fixed it, so it is not that important.
Screenshot 2023-05-15 at 20 43 49
  1. There is a place where the cursor turns green on this node, and pressing LMB produces bizarre code modifications. I don't see the same behavior for other functions, like String.split.
2023-05-15.20-44-33.mp4

@Frizi
Copy link
Contributor Author

Frizi commented May 17, 2023

PR is now ready for another QA round.

@vitvakatu
Copy link
Contributor

QA passed 💚

@Frizi Frizi added CI: Ready to merge This PR is eligible for automatic merge CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Keep up to date Automatically update this PR to the latest develop. labels May 17, 2023
@mergify mergify bot merged commit dcdba8d into develop May 17, 2023
@mergify mergify bot deleted the wip/frizi/list-editor-stabilization branch May 17, 2023 18:53
Procrat added a commit that referenced this pull request May 22, 2023
…t-rename

* develop:
  Widgets, Vector as Column, Viz Fixes and Rename Columns (#6768)
  Implement simple variants of `parse` for the Database backend (#6731)
  Enable `require-jsdoc` lint and add two lints related to React (#6403)
  Decimal/Integer .round and .int #6654 (#6743)
  Set suggestion reexports when serializing the library (#6778)
  Fix file uploading node expression. (#6689)
  Using WarningsLibrary to query for warnings (#6751)
  Implement `cast` for Table and Column (#6711)
  Display Initializing project... message when initializing project (#6661)
  Only send suggestions updates when type changes (#6755)
  sbt runEngineDistribution --debug to ease debuggging (#6745)
  Display "modified at" column on the cloud dashboard (#6687)
  Meta.meta Integer . methods (#6740)
  Show spinner while loading directory (#6714)
  Add cloud dashboard to changelog (#6688)
  Fix list editor panics during insertion (#6540)
  Update bug-report.yml
  Remove project create form (#6710)
  Change full-screen visualisation shortcut to shift-space (#6663)
Procrat added a commit that referenced this pull request May 22, 2023
* develop: (30 commits)
  Widgets, Vector as Column, Viz Fixes and Rename Columns (#6768)
  Implement simple variants of `parse` for the Database backend (#6731)
  Enable `require-jsdoc` lint and add two lints related to React (#6403)
  Decimal/Integer .round and .int #6654 (#6743)
  Set suggestion reexports when serializing the library (#6778)
  Fix file uploading node expression. (#6689)
  Using WarningsLibrary to query for warnings (#6751)
  Implement `cast` for Table and Column (#6711)
  Display Initializing project... message when initializing project (#6661)
  Only send suggestions updates when type changes (#6755)
  sbt runEngineDistribution --debug to ease debuggging (#6745)
  Display "modified at" column on the cloud dashboard (#6687)
  Meta.meta Integer . methods (#6740)
  Show spinner while loading directory (#6714)
  Add cloud dashboard to changelog (#6688)
  Fix list editor panics during insertion (#6540)
  Update bug-report.yml
  Remove project create form (#6710)
  Change full-screen visualisation shortcut to shift-space (#6663)
  Revert "Show spinner when opening/creating a project (#6321)" (#6712)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Keep up to date Automatically update this PR to the latest develop. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
5 participants