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

Add scope to blocks #79

Merged
merged 3 commits into from
Jun 26, 2024
Merged

Add scope to blocks #79

merged 3 commits into from
Jun 26, 2024

Conversation

wnbaum
Copy link
Contributor

@wnbaum wnbaum commented Jun 24, 2024

Made it so that parameter blocks copy-dragged from an EntryBlock can only be placed below entry blocks with the same method signature.

Also darken the block trees that cannot be snapped to by the current dragged block.

https://phabricator.endlessm.com/T35521

@wnbaum wnbaum requested review from manuq, wjt and dylanmccall June 24, 2024 22:43
Copy link
Contributor

@manuq manuq left a comment

Choose a reason for hiding this comment

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

Looks good! Can you transfer the information in the PR description to each commit message body?

@wnbaum
Copy link
Contributor Author

wnbaum commented Jun 25, 2024

Looks good! Can you transfer the information in the PR description to each commit message body?

Yes! I'll be better at doing that in the future ;)

@wnbaum
Copy link
Contributor Author

wnbaum commented Jun 25, 2024

Ok, should be better now @manuq

I just realized however that we shouldn't just check the scope of the dragged block, but also the scope of the dragged block's child blocks. That way you couldn't drag a child block out of it's scope. Should I add that to this PR?

Comment on lines 120 to 126
func set_scope(scope: String):
for block in _window.get_children():
if block is EntryBlock:
var entry_block = block as EntryBlock

if scope != entry_block.get_entry_statement():
entry_block.modulate = Color(0.5, 0.5, 0.5, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

As a general advice, at least in languages that have semantic indentation (Python, GDScript) it is better to return early to avoid nesting and thus very indented code. In a for loop you can do that with break/continue. In a function you can do that with return. Example:

func set_scope(scope: String):
	for block in _window.get_children():
		if block is not EntryBlock:
			continue
		var entry_block = block as EntryBlock
		(...)

@manuq
Copy link
Contributor

manuq commented Jun 25, 2024

Ok, should be better now @manuq

I just realized however that we shouldn't just check the scope of the dragged block, but also the scope of the dragged block's child blocks. That way you couldn't drag a child block out of it's scope. Should I add that to this PR?

I think it's fine to merge now after fixing conflicts and my request above. But if you think it's better to do the children blocks scoping now, that's fine with me too.

wnbaum added 2 commits June 25, 2024 21:52
Made it so that parameter blocks copy-dragged from an EntryBlock can only be placed below entry blocks with the same method signature.
Darken the block trees that cannot be snapped to by the currently dragged block.
@wnbaum
Copy link
Contributor Author

wnbaum commented Jun 26, 2024

Alright, I actually decided to just implement checking the child block scope as well. There should be no case where block trees will have blocks from different scopes.

Definitely gained some solid merging experience from rebasing with all of Dylan's new dragging/snapping code 💪

@wnbaum wnbaum requested a review from manuq June 26, 2024 02:16
Copy link
Contributor

@manuq manuq left a comment

Choose a reason for hiding this comment

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

Almost there!

Comment on lines 261 to 263
if copy_block:
if copy_block.drag_started.get_connections().size() == 0:
copy_block.drag_started.connect(func(b: Block): drag_copy_parameter(b, block))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if copy_block:
if copy_block.drag_started.get_connections().size() == 0:
copy_block.drag_started.connect(func(b: Block): drag_copy_parameter(b, block))
if copy_block == null:
continue
if copy_block.drag_started.get_connections().size() == 0:
copy_block.drag_started.connect(func(b: Block): drag_copy_parameter(b, block))

if tree_scope == "" or scope == tree_scope:
valid = true

if !valid:
Copy link
Contributor

Choose a reason for hiding this comment

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

The ! negation is deprecated in GDScript. Use not:

Suggested change
if !valid:
if not valid:

@wnbaum wnbaum requested a review from manuq June 26, 2024 16:59
Copy link
Contributor

@manuq manuq left a comment

Choose a reason for hiding this comment

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

@wnbaum looks good now! Feel free to squash the last commit and merge.

When dragging a block, we need to see if its child blocks are out of scope as well. Also check if child blocks of orphaned blocks are in a different scope.

Small code cleanup
@wnbaum wnbaum merged commit c5ae7e6 into main Jun 26, 2024
2 checks passed
@wnbaum wnbaum deleted the block-scope branch June 26, 2024 18:38
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