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

Malfunctioning edge after detaching source end #6772

Closed
2 tasks
Tracked by #6834
kazcw opened this issue May 18, 2023 · 24 comments · Fixed by #7041
Closed
2 tasks
Tracked by #6834

Malfunctioning edge after detaching source end #6772

kazcw opened this issue May 18, 2023 · 24 comments · Fixed by #7041
Assignees
Labels
--bug Type: bug p-high Should be completed in the next sprint

Comments

@kazcw
Copy link
Contributor

kazcw commented May 18, 2023

Discord username

No response

What type of issue is this?

Permanent – Occurring repeatably

Is this issue blocking you from using Enso?

  • Yes, I can't use Enso because of this issue.

Is this a regression?

  • Yes, previous version of Enso did not have this issue.

What issue are you facing?

Under some (common) conditions, an edge that has been detached from its source node cannot be attached to anything, and is stuck in a broken state.

Expected behaviour

After detaching an edge from a source, it should be possible to attach the source end to an output port.

How we can reproduce it?

  1. Choose an edge that would cause errors in the graph if detached, e.g. a node a method is invoked on.
  2. Click on the edge near the target side, to disconnect it from its source.
  3. Wait for the graph to show errors.
  4. (Once the errors show, you can see the edge slightly disconnect from the target node; this indicates the broken state has been reached.)
  5. Click on some output port to attempt to reattach the edge: Nothing happens.

Screenshots or screencasts

vokoscreenNG-2023-05-18_16-19-54.webm

Enso Version

develop (a32a2ea)

Browser or standalone distribution

Standalone distribution (local project)

Browser Version or standalone distribution

standalone

Operating System

Linux

Operating System Version

No response

Hardware you are using

No response

@kazcw
Copy link
Contributor Author

kazcw commented May 18, 2023

I encountered this while debugging #6707. It is not a problem with the edge view; I think after reevaluation, the port the edge is supposed to be attached to no longer exists. However, in #6707 I'll improve how the graph editor displays this case: If an edge is found to be invalid, it would be less confusing for it to disappear than to stick around in a zombie state.

Also, I think this is not a (recent) regression. I remember encountering it during a Book Clubs some time ago, but didn't find how to reproduce it at the time.

@kazcw kazcw added the -gui label May 18, 2023
@kazcw
Copy link
Contributor Author

kazcw commented May 19, 2023

It is repeatable most of the time but occasionally works as expected.

@Akirathan
Copy link
Member

I can't reproduce this in a simple case. Would you attach the project where you have spotted this, I could also try it there on the same node as you.

@kazcw
Copy link
Contributor Author

kazcw commented May 19, 2023

I can't reproduce this in a simple case. Would you attach the project where you have spotted this, I could also try it there on the same node as you.

It happens for me in a fresh Orders.

@kazcw
Copy link
Contributor Author

kazcw commented May 19, 2023

I'll take this, as I've already done some debugging

@kazcw kazcw assigned kazcw and unassigned farmaazon May 19, 2023
@kazcw kazcw moved this from ❓New to ⚙️ Design in Issues Board May 19, 2023
@kazcw kazcw changed the title Source-detached edges can't be reattached if detaching resulted in errors Malfunctioning edge after detaching source end May 19, 2023
@enso-bot
Copy link

enso-bot bot commented May 20, 2023

Keziah Wesley reports a new STANDUP for today (2023-05-19):

Progress: Implemented view invalidation batching (#6630). Identified the root cause of edge bug. It should be finished by 2023-05-22.

Next Day: Next day I will be working on the #6772 task. Design and implement solution.

@farmaazon
Copy link
Contributor

During a call, we came up with a solution: the edges should be represented by AST IDs (+ additional info if they are half-detached connections to some placeholders), not by span-tree crumbs. In the long term, we remove span tree altogether - see #6834

@Frizi Frizi assigned Frizi and unassigned kazcw May 26, 2023
@farmaazon farmaazon added this to the Design Partners milestone May 29, 2023
@farmaazon farmaazon moved this from ⚙️ Design to 📤 Backlog in Issues Board May 29, 2023
@farmaazon farmaazon moved this from 📤 Backlog to ❓New in Issues Board May 29, 2023
@farmaazon farmaazon added the p-high Should be completed in the next sprint label May 29, 2023
@farmaazon farmaazon moved this from ❓New to 📤 Backlog in Issues Board May 29, 2023
@Frizi Frizi assigned Frizi and unassigned farmaazon May 31, 2023
@Frizi Frizi moved this from 📤 Backlog to 🔧 Implementation in Issues Board May 31, 2023
@enso-bot
Copy link

enso-bot bot commented May 31, 2023

Paweł Grabarz reports a new 🔴 DELAY for yesterday (2023-05-30):

Summary: There is 11 days delay in implementation of the Malfunctioning edge after detaching source end (#6772) task.
It will cause 0 days delay for the delivery of this weekly plan.

Delay Cause: Task resumed after planning span-tree changes.

@enso-bot
Copy link

enso-bot bot commented May 31, 2023

Paweł Grabarz reports a new STANDUP for yesterday (2023-05-30):

Progress: Investigated possible alternative representations for edge endpoint, having in mind that we don't want to rely on span tree or old AST for too long. Turns out AST ID is not used for that at the controller level, so previous plans to use it at view layer as well will not be as straightforward to do as I initially thought. It should be finished by 2023-06-02.

@enso-bot
Copy link

enso-bot bot commented Jun 1, 2023

Paweł Grabarz reports a new STANDUP for yesterday (2023-05-31):

Progress: Started replacing the connection endpoint with AST ID, but this is not enough to properly handle disconnected edges. Proposed changing the behavior for edges with disconnected source to not apply target node modification until new source is set or edge is removed. It should be finished by 2023-06-02.

@enso-bot
Copy link

enso-bot bot commented Jun 2, 2023

Paweł Grabarz reports a new STANDUP for yesterday (2023-06-01):

Progress: The proposed behavior change was accepted. I'm continuing refactoring endpoints and edge handling FRP network in graph editor. It should be finished by 2023-06-02.

@enso-bot
Copy link

enso-bot bot commented Jun 6, 2023

Paweł Grabarz reports a new 🔴 DELAY for yesterday (2023-06-05):

Summary: There is 5 days delay in implementation of the Malfunctioning edge after detaching source end (#6772) task.
It will cause 5 days delay for the delivery of this weekly plan.

Delay Cause: Required logic changes and FRP refactoring turned out to be a lot more complicated than estimated.

@enso-bot
Copy link

enso-bot bot commented Jun 6, 2023

Paweł Grabarz reports a new STANDUP for yesterday (2023-06-05):

Progress: Continuing refactoring of edge handling. Completely replaced old logic related to handling detached edges. It should be finished by 2023-06-07.

@enso-bot
Copy link

enso-bot bot commented Jun 6, 2023

Paweł Grabarz reports a new STANDUP for today (2023-06-06):

Progress: Continuing refactoring of edge handling. Cleaned up replaced detached edge logic. Now propagating new edge endpoint data into the node components. It should be finished by 2023-06-07.

@enso-bot
Copy link

enso-bot bot commented Jun 13, 2023

Paweł Grabarz reports a new 🔴 DELAY for today (2023-06-13):

Summary: There is 7 days delay in implementation of the Malfunctioning edge after detaching source end (#6772) task.
It will cause 7 days delay for the delivery of this weekly plan.

Delay Cause: More graph editor refactoring left to do after days off and weekend. I was also maybe a bit too ambitious with the scope of those changes, but it's too late to change course now.

@enso-bot
Copy link

enso-bot bot commented Jun 13, 2023

Paweł Grabarz reports a new STANDUP for yesterday (2023-06-12):

Progress: Finished the bulk of refactoring in the controller layer and inside node views. The application appears to mostly work, but there are still some small edge interaction bugs to flesh out. I also want to split up the FRP network initialization inside graph editor, as it is a single absurdly huge method right now. It should be finished by 2023-06-14.

@enso-bot
Copy link

enso-bot bot commented Jun 13, 2023

Paweł Grabarz reports a new STANDUP for today (2023-06-13):

Progress: Split up most of graph editor edge-related FRP network into smaller initialization methods. Debating whether or not to refactor changes unrelated to edge editing, or if it's better to keep them in the old form for now. It should be finished by 2023-06-14.

@enso-bot
Copy link

enso-bot bot commented Jun 15, 2023

Paweł Grabarz reports a new STANDUP for yesterday (2023-06-14):

Progress: Finished split of graph editor FRP. Decided to keep it mostly scoped to edge-related part, since I don't want to spend any more time on this right now. Updated the branch to latest develop and still need to fix some issues caused by edge rendering optimizations. It should be finished by 2023-06-14.

@Frizi Frizi moved this from 🔧 Implementation to 👁️ Code review in Issues Board Jun 20, 2023
@Frizi Frizi moved this from 👁️ Code review to 🌟 Q/A review in Issues Board Jun 20, 2023
@enso-bot
Copy link

enso-bot bot commented Jun 20, 2023

Paweł Grabarz reports a new 🔴 DELAY for yesterday (2023-06-19):

Summary: There is 6 days delay in implementation of the Malfunctioning edge after detaching source end (#6772) task.
It will cause 6 days delay for the delivery of this weekly plan.

Delay Cause: Non-trivial merges, issues with broken tests and lacking testing utilities.

@enso-bot
Copy link

enso-bot bot commented Jun 20, 2023

Paweł Grabarz reports a new STANDUP for yesterday (2023-06-19):

Progress: Added improved mouse event dispatching and frame simulation to test utilities. Fixed all remaining failing tests. PR marked for review. It should be finished by 2023-06-20.

@enso-bot
Copy link

enso-bot bot commented Jun 20, 2023

Paweł Grabarz reports a new STANDUP for today (2023-06-20):

Progress: Applied review comments and fixed a few additional issues found during testing. Requested QA. It should be finished by 2023-06-20.

@mergify mergify bot closed this as completed in #7041 Jun 20, 2023
mergify bot pushed a commit that referenced this issue Jun 20, 2023
…side. (#7041)

Fixes #6772

When detaching an existing edge by grabbing by a source port, the node's code is no longer immediately modified. It is only changed once the edge has been either connected or destroyed. When grabbing on the source side, the existing behavior is preserved. That way, we always have guaranteed place to keep the edge connected to.

https://github.com/enso-org/enso/assets/919491/49e560cb-0a29-4c6a-97ec-4370185b8c89

In general, the detached edges are now more stable, resilient to all kinds of expression modifications during the drag.

https://github.com/enso-org/enso/assets/919491/e62450ff-46b2-466f-ac33-f4f19e66ee1d


In case there is a situation where the currently dragged edge's port is destroyed (e.g. by Undo/Redo), instead of showing glitched port position it is simply dropped.

https://github.com/enso-org/enso/assets/919491/8fb089aa-a4a5-4a8c-92eb-23aeff9867b8

# Important Notes

The whole edge connection and view handling at the graph-editor view level has been completely rewritten. The edge endpoints are now identified using new `PortId` structure, that is not dependant on the span-tree. This prepares us for eventual removal of the span-tree in favour of manipulating AST directly. Right now those `PortId`s are still stored within the span-tree nodes, but it will be easy to eventually generate them on the fly from the AST itself. The widget tree has also already been switched to that representation where appropriate.

Additionally, I have started splitting the graph editor FRP network into smaller methods. Due to its absolutely enormous size and complexity of it, I haven't finished the split completely, and mostly edge-related part is refactored. I don't want to block this PR on this any longer though, as the merge conflicts are getting a bit unwieldy to deal with.
@github-project-automation github-project-automation bot moved this from 🌟 Q/A review to 🟢 Accepted in Issues Board Jun 20, 2023
@farmaazon farmaazon moved this from 🟢 Accepted to 🗄️ Archived in Issues Board Jun 27, 2023
@enso-bot
Copy link

enso-bot bot commented Jul 13, 2023

Paweł Grabarz reports a new STANDUP for the provided date (2023-06-07):

Progress: Continuing edge data refactoring in controller. It should be finished by 2023-06-20.

@enso-bot
Copy link

enso-bot bot commented Jul 13, 2023

Paweł Grabarz reports a new STANDUP for the provided date (2023-06-15):

Progress: Cleaned up edge rendering after rebase. Started preparing PR for final review, but still have some tests to fix. It should be finished by 2023-06-20.

@enso-bot
Copy link

enso-bot bot commented Jul 13, 2023

Paweł Grabarz reports a new STANDUP for the provided date (2023-06-16):

Progress: Continuing with fixing span-tree generation and edge connect/disconnect tests. Fixed issues found during testing, where connecting to nodes without assignment expression was not handled properly. It should be finished by 2023-06-20.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug p-high Should be completed in the next sprint
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants