Fix DynamicFont breaking mouse grab in inspector spinners #40701
Conversation
opl-
force-pushed
the
fix/25046
branch
2 times, most recently
from
7b32780 to
79b59b6
Compare
scene/resources/dynamic_font.cpp
Outdated
| #define SET_SPACING_VALUE(NAME) \ | ||
| spacing_##NAME = p_value; \ | ||
| emit_changed(); \ | ||
| _change_notify("extra_spacing_" #NAME); | ||
|
|
There was a problem hiding this comment.
In this trivial case, the define is probably not needed. It only saves a few lines of code and the code would be more easily readable without it.
There was a problem hiding this comment.
I was a bit on the fence about this myself. The main reason for this was that seemingly _change_notify was always called before emit_changed, but looking at other resources it's not a requirement.
I'll push the change after testing it to make sure. It seems current master broke GDScript's tool which I'm relying on for testing.
There was a problem hiding this comment.
Even it emit_changed() should be kept first, it's ok to have it duplicated in the four branches.
There was a problem hiding this comment.
Changed it and rebased the branch to an older commit from master as #40598 doesn't yet have support for tool which is rather useful for testing here.
Edit: Apparently in my confusion I forgot to actually stage the relevant change. I also noticed that according to the PR tool works, but has been replaced with an @tool annotation. Will correctly fix this in a bit.
|
This fix would work, although we might also want to properly fix the more general unwanted behavior with spinners (probably outside of this PR's scope). |
| @@ -677,7 +677,7 @@ DynamicFontAtSize::~DynamicFontAtSize() { | |||
|
|
|||
| ///////////////////////// | |||
|
|
|||
| void DynamicFont::_reload_cache() { | |||
| void DynamicFont::_reload_cache(const char *p_triggering_property) { | |||
There was a problem hiding this comment.
Any specific reason for const char * over const String &?
There was a problem hiding this comment.
Lack of my knowledge about project conventions. Also, _change_notify takes in const char *, so I reused the type.
This was caused by DynamicFont not specifying which property was edited, resulting in the whole inspector property tree being recreated. Fixes godotengine#25046
|
@aaronfranke It seems that this is still an issue on master, but since #45879 the property change detection system works differently, so I'd have to look into it again and create an entirely new PR. Meanwhile, this fix is still relevant to the 3.2 branch so we could just change the target branch for this PR. |
|
This pull request should be reopened against the |
|
The Edit button next to the title shows a dropdown that allows changing the target branch. We could try to see if it works. If it doesn't, at least we learned something and I'll open a new PR. |
|
@opl- It looks like it's not working, so I had to revert it. Please open a new pull request against the |
This was caused by DynamicFont not specifying which property was edited, resulting in the whole inspector being recreated
Fixes #25046
Edit: Valuable lessons about code formatting and the tools associated have been learned.