WIP: Godot 4 migration pass #1192
Conversation
This should fix the mess I've made previously
Refactor ThemeManager to improve type safety and clarity. res://addons/gdscript-course-builder/ui/InsertContentBlockDialog.tscn now plays properly, will check the rest
|
Thanks again for the work you're doing. One small note, if you use LLMs as part of your toolset, please make sure to preserve comments as to how the file is supposed to function and things like that. The comments about why certain things were done were already sparse, and they're important to keep to maintain a good understanding of caveats or workarounds, etc. |
|
Thanks for the note — totally fair point. This isn’t the final form yet. While doing the mechanical Godot 4 port and tracking down runtime issues, some explanatory comments may have been removed or simplified. I’m currently focusing on getting the project to run cleanly first, and I’ll be restoring missing comments (and adding Godot 4–specific notes where behavior changed) as I continue iterating over the files in the next few days. |
NathanLovato
left a comment
NathanLovato
left a comment
There was a problem hiding this comment.
I'm just leaving you a few comments here as you're working through this to point out things I'd like you to change in the approach as you're working on this so the work you do does not have to be undone later on.
I'll check things out as you progress and leave you a few pointers like these for guidelines. If you have any questions, let me know.
| onready var _theme = preload("res://ui/theme/gdscript_app_theme.tres") | ||
|
|
||
| var _font_defaults := {} | ||
| @onready var _theme: Theme = preload("res://ui/theme/gdscript_app_theme.tres") |
There was a problem hiding this comment.
| @onready var _theme: Theme = preload("res://ui/theme/gdscript_app_theme.tres") | |
| var _theme: Theme = preload("res://ui/theme/gdscript_app_theme.tres") |
There was a problem hiding this comment.
No need to delay the variable execution here. the resource will be preloaded at compile/load time.
|
|
||
| func _cache_font_defaults() -> void: | ||
| _font_defaults.clear() | ||
| _cache_theme_defaults() |
There was a problem hiding this comment.
Here I recommend inlining the function's code and not calling a separate function. You can include a comment about why you use this code: to store the default size of things.
Splitting small pieces of code like that to separate functions, on the one hand, labels the functions, but when they're small single use pieces like this, it also creates indirection. In a small script like that, it's fine. It's just at scale, it makes the code harder to trace.
| func _cache_theme_defaults() -> void: | ||
| _theme_defaults.clear() | ||
| for type: String in THEME_TYPES: | ||
| _theme_defaults[type] = { | ||
| "size": _theme.get_font_size("font_size", type), | ||
| "font": _theme.get_font("font", type) | ||
| } |
There was a problem hiding this comment.
Why clear the theme defaults here? They start empty already and this is called only once. Also, as mentioned above, I'd prefer this code to be inlined in the _ready() function.
| var current_profile = UserProfiles.get_profile() | ||
|
|
||
| error = fs.list_dir_begin(true, true) | ||
| if error != OK: | ||
| printerr("Failed to read theme fonts directory at '%s': Error code %d" % [THEME_FONTS_ROOT, error]) | ||
| return | ||
| # Fix for lines 20-22: Cast the Variant to the target type before passing to functions | ||
| var scale_val: float = current_profile.font_size_scale as float | ||
| var contrast_val: bool = current_profile.lower_contrast as bool | ||
| var dyslexia_val: bool = current_profile.dyslexia_font as bool | ||
|
|
||
| var current_file := fs.get_next() as String | ||
| while not current_file.empty(): | ||
| if current_file.get_extension() != "tres": | ||
| current_file = fs.get_next() | ||
| continue | ||
|
|
||
| var font_resource = ResourceLoader.load(THEME_FONTS_ROOT.plus_file(current_file)) as DynamicFont | ||
| if not font_resource: | ||
| current_file = fs.get_next() | ||
| continue | ||
|
|
||
| _font_defaults[font_resource] = {"size": font_resource.size, "font": font_resource.font_data.font_path} | ||
| current_file = fs.get_next() | ||
|
|
||
| scale_all_font_sizes(roundi(scale_val), false) | ||
| set_lower_contrast(contrast_val, false) | ||
| set_dyslexia_font(dyslexia_val, false) |
There was a problem hiding this comment.
GitHub is buggy and won't let me suggest code here, but for code like this:
- The comment is not helpful here and should be removed. Looks like an LLM insertion (the code also does look like that)
- The variables look like proxies around the current profile properties and add unnecessary lines of code.
- The type information is only missing on line 16 on the current profile. Adding just a colon for type inference would give you type information.
Instead here the code should look more like this:
var current_profile := UserProfiles.get_profile()
# ...
scale_all_font_sizes(current_profile.font_size_scale, false)
# ...It looks like you use an LLM, so I'd say it's really important that you check after the tool and think about what the code actually needs to do and if anything is unnecessary. No matter what you do, and even with the best LLM, it will likely generate and include unnecessary and poor code that does things that are not needed. It may also break the code in subtle ways.
2 participants
Please check if the PR fulfills these requirements:
The commit message follows our guidelines.
For bug fixes and features:
Related issue (if applicable): N/A
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
(The project behavior remains the same; changes are required for Godot 4 compatibility.)
New feature or change
What is the current behavior?
What is the new behavior?
project.godotwere updated for Godot 4 compatibility.Known issues / follow-ups
WindowDialog,ScrollContainer) that require layout or design decisions.Other information
.gdproject.godot.tscnand.tresfiles.import,.godot/) were intentionally excluded and are regenerated by the engine.