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.

Sign up for GitHub

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

Jump to bottom

FP-2468 - Study performance decrease in tree view flow editor #166

Closed
wants to merge 1 commit into from
Closed

FP-2468 - Study performance decrease in tree view flow editor #166

wants to merge 1 commit into from

Conversation

quirinpa
Copy link
Contributor

@quirinpa quirinpa requested a review from a team as a code owner July 10, 2023 12:21
@quirinpa quirinpa self-assigned this Jul 10, 2023
@quirinpa quirinpa changed the title Fp 2473 ide flows appear as running but nodes dont in tree view 6 FP-2468 - Study performance decrease in tree view flow editor Jul 10, 2023
@PedroCristovao-Movai PedroCristovao-Movai changed the base branch from FP-2473-ide-flows-appear-as-running-but-nodes-dont-in-tree-view-5 to dev July 10, 2023 13:22
@quirinpa quirinpa force-pushed the FP-2473-ide-flows-appear-as-running-but-nodes-dont-in-tree-view-6 branch 4 times, most recently from a95e770 to db6808e Compare July 11, 2023 08:38
manuelnogueiramov
manuelnogueiramov requested changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@manuelnogueiramov manuelnogueiramov left a comment

Choose a reason for hiding this comment

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

To be fair I saw a couple of improvements, ie: when selecting node and then selecting another node, there's no longer a 300ms delay (which is very good, even though it wasn't noticeable by most humans). This, just to say that the idea of the PR seems good and the direction seems interesting.
The issue is, I made a couple of comments, to be honest most of them aren't that big of a deal. But upon testing the PR thoroughly I found a lot of regressions that need to be addressed before merging this.

List of regressions that I've found (which doesn't mean more might have slipped):

  • No warnings being shown.
    no_warnings_shown

  • After dragging a selection of nodes, or a selected node it gets de-selected in the end.
    shift_drag_unselects

  • Double clicking a subflow, doesn't open said subflow (infinite loading).
    open_subflow_explodes

  • Double clicking the start node in tree view, causes Add Node Bookmark to forever disappear.double_click_start_node

  • Selecting a node, or multiple nodes, causes it/them to be dragged along side with another node that you want to drag.
    drag_weird_selection

  • Update document is no longer working.
    not_updating_flow

As I said, there might be a lot more regression issues I didn't catch.

src/decorators/withBookmarks.jsx Outdated Show resolved Hide resolved
src/editors/Flow/view/Components/Explorer/Preview.jsx Outdated Show resolved Hide resolved
src/editors/Flow/view/Components/interface/MainInterface.js Outdated Show resolved Hide resolved
src/editors/Flow/view/Components/interface/MainInterface.js Outdated Show resolved Hide resolved
src/editors/Flow/view/Components/interface/MainInterface.js Outdated Show resolved Hide resolved
src/editors/Flow/view/Components/interface/MainInterface.js Outdated Show resolved Hide resolved
src/editors/Flow/view/Components/interface/MainInterface.js Outdated Show resolved Hide resolved
src/editors/Flow/view/Components/interface/canvas.js Show resolved Hide resolved
src/editors/Flow/view/Flow.jsx Show resolved Hide resolved
src/plugins/DocManager/DocManager.js Show resolved Hide resolved
quirinpa added a commit that referenced this pull request Jul 24, 2023
@quirinpa
Fix nodes getting unselected after drag
84cecbc 
- [Part of comment](#166 (review))
@quirinpa quirinpa force-pushed the FP-2473-ide-flows-appear-as-running-but-nodes-dont-in-tree-view-6 branch 2 times, most recently from b88f73e to 47a05b4 Compare July 24, 2023 14:00
@quirinpa
Copy link
Contributor Author 

* Update document is no longer working.
  ![not_updating_flow](https://user-images.githubusercontent.com/94365419/254418956-4d452f2a-043e-44df-aada-95552bcc0634.gif)

Just to clarify, it is not the "saving" that is in question here. The document is saved.
The following video shows how in version 2.4.0-61 of project tugbot there was already a problem with "Update document".
Suggesting that this issue is not fully introduced by this PR:
untitled

@quirinpa
Copy link
Contributor Author

quirinpa commented Jul 24, 2023 •
edited
Loading 

* Double clicking a subflow, doesn't open said subflow (infinite loading).
  ![open_subflow_explodes](https://user-images.githubusercontent.com/94365419/254418857-f8b41bad-e664-43b5-911a-2377150fa492.gif)

This is not happening to me. It may be a (older) problem that happens sometimes in which the flow fails to load due to a timeout. Can you please test again and see if it still happens?

@quirinpa quirinpa changed the base branch from dev to FP-2541-invalid-links-warning-message-is-missing-in-removing-port July 24, 2023 14:28
@quirinpa
Copy link
Contributor Author

quirinpa commented Jul 24, 2023 •
edited
Loading

  • After dragging a selection of nodes, or a selected node it gets de-selected in the end.

This is fixed.

@quirinpa
Copy link
Contributor Author

quirinpa commented Jul 24, 2023 •
edited
Loading

  • No warnings being shown.

This problem was already in dev. This PR fixes it:
#173

@quirinpa
Copy link
Contributor Author

  • Double clicking the start node in tree view, causes Add Node Bookmark to forever disappear.

Not anymore ;)

@quirinpa
Copy link
Contributor Author

  • Selecting a node, or multiple nodes, causes it/them to be dragged along side with another node that you want to drag.

Fixed

@quirinpa quirinpa force-pushed the FP-2473-ide-flows-appear-as-running-but-nodes-dont-in-tree-view-6 branch 2 times, most recently from f81ed83 to a946b18 Compare July 26, 2023 13:38
@quirinpa quirinpa changed the base branch from FP-2541-invalid-links-warning-message-is-missing-in-removing-port to dev July 26, 2023 13:39
@quirinpa quirinpa force-pushed the FP-2473-ide-flows-appear-as-running-but-nodes-dont-in-tree-view-6 branch from a946b18 to d5ae392 Compare July 26, 2023 13:43
@manuelnogueiramov
Copy link
Contributor 

* Update document is no longer working.
  ![not_updating_flow](https://user-images.githubusercontent.com/94365419/254418956-4d452f2a-043e-44df-aada-95552bcc0634.gif)

Just to clarify, it is not the "saving" that is in question here. The document is saved. The following video shows how in version 2.4.0-61 of project tugbot there was already a problem with "Update document". Suggesting that this issue is not fully introduced by this PR: untitled

You're right. I can confirm that, and already have a fix here.

@manuelnogueiramov
Copy link
Contributor 

* Double clicking a subflow, doesn't open said subflow (infinite loading).
  ![open_subflow_explodes](https://user-images.githubusercontent.com/94365419/254418857-f8b41bad-e664-43b5-911a-2377150fa492.gif)

This is not happening to me. It may be a (older) problem that happens sometimes in which the flow fails to load due to a timeout. Can you please test again and see if it still happens?

I could replicate this 100%. As in, if I open the flow from the Explorer, it opens fine, but when I try to open it from inside the flow it will happen 100%. Also, tested in 2.4.0-4 and it worked fine (to be fair didn't test in dev yet to check if it isn't caused by this PR).

@quirinpa
Copy link
Contributor Author

quirinpa commented Jul 26, 2023 •
edited
Loading 

* Double clicking a subflow, doesn't open said subflow (infinite loading).
  ![open_subflow_explodes](https://user-images.githubusercontent.com/94365419/254418857-f8b41bad-e664-43b5-911a-2377150fa492.gif)

This is not happening to me. It may be a (older) problem that happens sometimes in which the flow fails to load due to a timeout. Can you please test again and see if it still happens?

I could replicate this 100%. As in, if I open the flow from the Explorer, it opens fine, but when I try to open it from inside the flow it will happen 100%. Also, tested in 2.4.0-4 and it worked fine (to be fair didn't test in dev yet to check if it isn't caused by this PR).

Ok, but:
noInfinite
Certainly not 100% of the time, for me.

For me, that bug happens 0% of the time.
Is there something different between our environments?

@quirinpa
Copy link
Contributor Author

quirinpa commented Jul 26, 2023 •
edited
Loading

You're right. I can confirm that, and already have a fix here.

Ok. Thank you. I didn't realize you were trying to fix that in that PR. Can you rebase or merge with dev? It has been updated.

@quirinpa quirinpa force-pushed the FP-2473-ide-flows-appear-as-running-but-nodes-dont-in-tree-view-6 branch from d5ae392 to c1fde12 Compare July 31, 2023 15:55
@sonarcloud
Copy link

sonarcloud bot commented Jul 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@manuelnogueiramov manuelnogueiramov marked this pull request as draft September 24, 2023 16:18
@quirinpa
Copy link
Contributor Author

quirinpa commented Oct 2, 2024

Wouldn't happen this way

@quirinpa quirinpa closed this Oct 2, 2024
@diasdm diasdm deleted the FP-2473-ide-flows-appear-as-running-but-nodes-dont-in-tree-view-6 branch October 14, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manuelnogueiramov manuelnogueiramov Awaiting requested review from manuelnogueiramov

Assignees

@quirinpa quirinpa

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@quirinpa @manuelnogueiramov