Skip to content

Add multiple node deletion and undo auto-arrange#19

Merged
atomic-junky merged 3 commits into
mainfrom
feature/group-controls
Aug 18, 2024
Merged

Add multiple node deletion and undo auto-arrange#19
atomic-junky merged 3 commits into
mainfrom
feature/group-controls

Conversation

@RailKill

@RailKill RailKill commented Aug 18, 2024

Copy link
Copy Markdown
Collaborator

This PR does the following:

  • Allows deletion of multiple selected nodes using the Delete key. This action is undo-able.
  • Allows undo-ing and redo-ing of the auto arrange node button introduced in Godot 4.3 graph edit.

Summary by Sourcery

Introduce the ability to delete multiple selected nodes with undo support and enhance the auto-arrange feature with undo/redo capabilities. Improve node selection and position management in the graph editor.

New Features:

  • Enable deletion of multiple selected nodes using the 'Delete' key, with support for undoing this action.
  • Add undo and redo functionality for the auto-arrange nodes feature in the graph editor.

Enhancements:

  • Improve node selection handling by maintaining a list of selected nodes and updating node positions accordingly.
  • Refactor the graph node selection and deselection logic to better manage active nodes and their positions.

@sourcery-ai

sourcery-ai Bot commented Aug 18, 2024

Copy link
Copy Markdown

Reviewer's Guide by Sourcery

This pull request implements two main features: multi-node deletion and undo/redo functionality for the auto-arrange feature in the graph editor. The changes primarily affect the MonologueGraphEdit script, with minor adjustments to MonologueControl and SidePanelNodeDetails scripts. The implementation focuses on enhancing user interaction with the graph, improving selection handling, and integrating with the existing undo/redo system.

File-Level Changes

Files Changes
Scripts/MonologueGraphEdit.gd Implemented multi-node deletion functionality using the Delete key, integrated with the undo/redo system
Scripts/MonologueGraphEdit.gd Added undo/redo support for the auto-arrange feature, recording node positions before and after arrangement
Scripts/MonologueGraphEdit.gd
Scripts/SidePanelNodes/SidePanelNodeDetails.gd
Refactored node selection handling to support multiple selected nodes
Scripts/MonologueGraphEdit.gd
Scripts/MonologueControl.gd
Scripts/SidePanelNodes/SidePanelNodeDetails.gd
Improved management of active and selected nodes, including updates to the side panel behavior
Scripts/MonologueGraphEdit.gd Removed redundant variables and simplified input handling in the graph edit script
Scripts/MonologueGraphEdit.gd
Scripts/MonologueControl.gd
Updated node position tracking for undo/redo operations

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @RailKill - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider adding unit tests for the new multiple node deletion and undo/redo functionality to ensure robustness and prevent regressions.
  • The _input function in MonologueGraphEdit.gd has become quite complex. Consider breaking it down into smaller, more focused functions for better readability and maintainability.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@@ -36,11 +36,13 @@ func on_graph_node_selected(node: MonologueGraphNode, bypass: bool = false):
if not bypass:
var graph_edit = control_node.get_current_graph_edit()
await get_tree().create_timer(0.1).timeout

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (bug_risk): Consider a more deterministic approach to handling timing

Using a timer with a fixed delay could lead to race conditions. Consider finding a more deterministic way to handle this timing issue, such as using signals or callbacks.

var timer = Timer.new()
add_child(timer)
timer.set_one_shot(true)
timer.set_wait_time(0.1)
timer.timeout.connect(_on_timer_timeout)
timer.start()

func _on_timer_timeout():
	# Place the code that should run after the delay here

@RailKill RailKill requested a review from atomic-junky August 18, 2024 17:56
@atomic-junky

Copy link
Copy Markdown
Collaborator

It seems to work perfectly!

@atomic-junky atomic-junky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

@atomic-junky atomic-junky merged commit 838f38f into main Aug 18, 2024
@RailKill RailKill deleted the feature/group-controls branch August 18, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants