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

Fix dictionary editor steals focus when reloading script #96940

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Dowsley
Copy link
Contributor

@Dowsley Dowsley commented Sep 13, 2024

Fix #96913

The default value on the form of a typed dictionary (e.g. 0 for an int) would count as an "unsaved change", thus triggering a update_property call, and would trigger the focus.

I am not aware of any case in which focus trigger to the "New Key" or "New Pair" inspectors is desired. Assuming this correct, the proposed change is the simplest possible fix I could find.

@AThousandShips
Copy link
Member

But can confirm that this fixes the bug

@ajreckof
Copy link
Member

This is most likely not the right fix as it will break the functionality added here : #88322 for the new key/Value. The bug cited might be a regression from the PR mentioned.

@Dowsley
Copy link
Contributor Author

Dowsley commented Sep 17, 2024

This is most likely not the right fix as it will break the functionality added here : #88322 for the new key/Value. The bug cited might be a regression from the PR mentioned.

Definitely a regression from that. All the functions I debugged through were added by this PR. Will take a look on how we can fix this without breaking the necessary focus change.

I'm thinking of rehauling the focus mechanism instead of removing it. Make it only focus when the type is being changed.

@Dowsley
Copy link
Contributor Author

Dowsley commented Sep 17, 2024

@ajreckof @AThousandShips Can you take a look at the new proposal? It's just a mock. But from my test, the logic prevents needless focus change (i.e. the editor), but causes focus change when the type is changed from the dropdown menu, thus respecting the previous PR and fixing its regression.

@AThousandShips AThousandShips changed the title Make exported type dictionary New Key Value pair not steal focus Fix dictionary editor steals focus when reloading script Sep 17, 2024
@ajreckof ajreckof self-assigned this Sep 17, 2024
@ajreckof
Copy link
Member

ajreckof commented Sep 25, 2024

So, I’ve reviewed the changes, and unfortunately, the issue persists. While the new proposal does mitigate some problems, changing the type of the new key will refocus the new value, which breaks the functionality in another way.

The section of the code you’ve been modifying is actually correct. The issue stems from the fact that changing_type_index is being assigned a value it shouldn’t have (i.e., -1, which corresponds to new_value when it should be -3 for no_change). I recommend reviewing the parts of the code that assign this value to track down the source of the problem.

Edit: If you have any questions /reach a block feel free to join the rocket chat and send me a dm there.

@Dowsley
Copy link
Contributor Author

Dowsley commented Oct 4, 2024

Will be revisiting this issue soon.

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