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

Editor: Focus value editor on type change in Dictionary and Array editors #88322

Conversation

EmrysMyrddin
Copy link
Contributor

@EmrysMyrddin EmrysMyrddin commented Feb 14, 2024

In the inspector, when editing an exported Dictionary property, the focus is lost once a type is chosen for the new key, the new value or an existing value.

Enregistrement.de.l.ecran.2024-02-14.a.14.10.45.mov

This PR is fixing this by focusing the editor property after changing the type:

Enregistrement.de.l.ecran.2024-02-14.a.14.13.18.mov

@EmrysMyrddin EmrysMyrddin force-pushed the fix/dict_editor_focus_on_type_change branch from 4d10334 to cb761ee Compare February 14, 2024 13:14
@AThousandShips AThousandShips added this to the 4.3 milestone Feb 14, 2024
@AThousandShips AThousandShips requested review from a team and removed request for AThousandShips February 14, 2024 21:03
@KoBeWi
Copy link
Member

KoBeWi commented Feb 29, 2024

The focus is still lost when pressing the Add Key/Value Pair button. Any chance this could also be improved?

EDIT:
Seems like it only happens right after changing type 🤔 If you edit a value and then add it, the focus is preserved.

@ajreckof
Copy link
Member

ajreckof commented Mar 29, 2024

This PR was superseded by #88231 as it fix the focus loss. But I feel like the idea of switching the focus from the change_type button to the EditorProperty is a good idea and could be a great improvement.

@EmrysMyrddin
Copy link
Contributor Author

@ajreckof Thank you for the update, should I continue to work on this then ?

@ajreckof
Copy link
Member

I think the switch of focus would be a great improvement and I would 100% be for it. And I don't see any reasons it wouldn't be accepted. If you need any help with it feel free to send me a message on rocket chat.

@EmrysMyrddin EmrysMyrddin force-pushed the fix/dict_editor_focus_on_type_change branch from 77df718 to d20ef93 Compare May 27, 2024 20:36
@EmrysMyrddin
Copy link
Contributor Author

Wow, @ajreckof great work on this refactor! The code is muuuch clearer now, it's even cleaner and more understandable to fix the focus now!

I've rebased and fixed issue pointed out by @KoBeWi, the problem was that the focus was taken only on property update, which doesn't happens if you open the change type popup but chose the same type already selected.

@EmrysMyrddin
Copy link
Contributor Author

After looking at @ajreckof refactoring, I understood that the Array Editor works pretty much the same, so I also made the change for it, so that both Dictionary and Array editors have the same behaviour.

@EmrysMyrddin EmrysMyrddin changed the title Editor: Fix losing focus after changing type in dict editor Editor: Focus value editor on type change in Dictionary and Array editors May 28, 2024
@AThousandShips
Copy link
Member

Please squash your commits into one, see here

@EmrysMyrddin EmrysMyrddin force-pushed the fix/dict_editor_focus_on_type_change branch from 0b4a000 to c845e53 Compare June 4, 2024 13:19
@EmrysMyrddin
Copy link
Contributor Author

Sure, done ✅

@EmrysMyrddin EmrysMyrddin force-pushed the fix/dict_editor_focus_on_type_change branch from c845e53 to c31111f Compare June 4, 2024 14:51
@EmrysMyrddin
Copy link
Contributor Author

Should keep the branch as much up to date as possible by rebasing on master on a regular bases ?

@KoBeWi
Copy link
Member

KoBeWi commented Jun 5, 2024

Only when there are conflicts or a long time has passed (i.e months). Usually it's not necessary.

@akien-mga akien-mga merged commit 0c9531c into godotengine:master Jun 7, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected. This is a nice usability boost for keyboard usage 🙂

(Sorry, forgot to refresh before posting the review.)

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

Successfully merging this pull request may close these issues.

6 participants