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

Can't write Label text #88176

Closed
KoBeWi opened this issue Feb 10, 2024 · 5 comments · Fixed by #88182
Closed

Can't write Label text #88176

KoBeWi opened this issue Feb 10, 2024 · 5 comments · Fixed by #88182

Comments

@KoBeWi
Copy link
Member

KoBeWi commented Feb 10, 2024

Tested versions

Regression from #68420

System information

Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1060 (NVIDIA; 30.0.15.1403) - Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz (8 Threads)

Issue description

godot.windows.editor.dev.x86_64_OGJVNqg6Ih.mp4

This is caused by Label calling update_configuration_warnings() in set_text():

update_configuration_warnings();

Which since #68420 will cause inspector update.

I somehow overlooked it while reviewing, I think there should be a separate warning signal that causes inspector update.
CC @RedMser

Steps to reproduce

  1. Add Label
  2. Change text property

Minimal reproduction project (MRP)

N/A

@KoBeWi KoBeWi added this to the 4.3 milestone Feb 10, 2024
@AThousandShips AThousandShips changed the title Can't write Labe text Can't write Label text Feb 10, 2024
@kleonc
Copy link
Member

kleonc commented Feb 10, 2024

CC @redms

cc @RedMser 🙃

@RedMser
Copy link
Contributor

RedMser commented Feb 10, 2024

So when update_configuration_warnings() happens, instead of directly queuing an inspector update, it should instead wait for a time when the user is not interacting with inspector? Can this be detected, or simply start a delayed timer so the problem is circumvented slightly?

Or do you suggest another update mechanism specifically for updating the config warning icon in the inspector, without updating the rest of the inspector (which sounds a lot harder given the current update logic, it ties itself to the whole inspector's layout)?

@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 10, 2024

I mean since your new inspector update is specifically to update configuration warnings that refer to a property, there should be a separate method that updates warnings and the inspector. That way the old method would not trigger inspector update, because most warnings don't need inspector update. If a user has a warning referring to a property, they should use the other method.

That's the simplest solution for now. I don't think partial inspector update or the other thing you mentioned is reasonable to implement

@RedMser
Copy link
Contributor

RedMser commented Feb 10, 2024

I'd prefer to have user code still just call update_configuration_warnings() like before, and inspector only updates when warnings actually refer to a specific property.

So maybe just make the inspector check if the configuration warning info that's currently showing in the UI is actually different from its previous state (e.g. by keeping track of a dirty flag, and doing string comparison from old value).

It would still be a problem if you're typing into a field that gets a new property config warning due to your input, but at least for all other cases it would be fine.

@RedMser
Copy link
Contributor

RedMser commented Feb 10, 2024

I implemented my proposed fix in #88182

Tested on both the Label example, and on a custom script that runs update_configuration_warnings() in _process just to verify (unlike scene tree, the inspector doesn't have the 0.5s update timer throttle).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants