Improve ControlBlock background drawing #177
Conversation
1265925 to
38ab701
Compare
|
|
||
| var snap_gutter := Control.new() | ||
| snap_gutter.name = "Background" | ||
| snap_gutter.set_script(preload("res://addons/block_code/ui/blocks/utilities/background/gutter.gd")) |
There was a problem hiding this comment.
I've seen this in a couple places in here. Why not just instantiate the class directly?
There was a problem hiding this comment.
I'm not sure why we don't do that, indeed. I'll fix that :)
There was a problem hiding this comment.
Yep, set_script() only makes sense in the BlockCode node when it replaces the parent script. All others should be fixed.
|
|
||
| var snap_gutter := Control.new() | ||
| snap_gutter.name = "Background" | ||
| snap_gutter.set_script(preload("res://addons/block_code/ui/blocks/utilities/background/gutter.gd")) |
There was a problem hiding this comment.
Yep, set_script() only makes sense in the BlockCode node when it replaces the parent script. All others should be fixed.
Instead of reusing the background.gd script to draw a gutter in ControlBlock, define a new script (gutter.gd) specifically for that purpose. In the existing background.gd script, draw a left border which is expected to flow into the gutter if shift_top or shift_bottom is set. https://phabricator.endlessm.com/T35538
38ab701 to
b20005e
Compare
|
In the newest set of commits, I removed the unnecessary change to |
| @@ -144,9 +143,8 @@ func format(): | |||
| snap_container.custom_minimum_size.y = 30 | |||
| snap_container.size_flags_horizontal = Control.SIZE_SHRINK_BEGIN | |||
|
|
|||
| var snap_gutter := Control.new() | |||
| var snap_gutter := BlockGutter.new() | |||
There was a problem hiding this comment.
I think we've been trying to move away from global classes unless their really needed since it reduces the chance for startup issues with the global class cache. Instead, I think you should just preload them at the top:
const BlockBackground = preload("res://addons/block_code/ui/blocks/utilities/background/background.gd")
const BlockGutter = preload("res://addons/block_code/ui/blocks/utilities/background/gutter.gd")
There was a problem hiding this comment.
Yes, exactly. Can you make this last change @dylanmccall ?
There was a problem hiding this comment.
Ah, I missed that happening! Makes sense :) That's fixed in the newest commit.
There was a problem hiding this comment.
I think Manuel started it near the beginning of GUADEC in the context of import errors and I happened to notice it since I was trying to wrap up some of my own work that Thursday and Friday.
| @@ -144,9 +143,8 @@ func format(): | |||
| snap_container.custom_minimum_size.y = 30 | |||
| snap_container.size_flags_horizontal = Control.SIZE_SHRINK_BEGIN | |||
|
|
|||
| var snap_gutter := Control.new() | |||
| var snap_gutter := BlockGutter.new() | |||
There was a problem hiding this comment.
Yes, exactly. Can you make this last change @dylanmccall ?
| @@ -1,4 +1,5 @@ | |||
| @tool | |||
| class_name BlockBackground | |||
There was a problem hiding this comment.
As Dan pointed, please remove class_name and use preload instead.
b20005e to
d2495b0
Compare
Instead of reusing the background.gd script to draw a gutter in ControlBlock, define a new script (gutter.gd) specifically for that purpose. In the existing background.gd script, draw a left border which is expected to flow into the gutter if shift_top or shift_bottom is set.
https://phabricator.endlessm.com/T35538