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

Use InputMap actions consistently across all LineEdit's that filter an underlying Tree or ItemList. #96400

Conversation

Maran23
Copy link
Contributor

@Maran23 Maran23 commented Aug 31, 2024

Fixes: #96212 (review)
Similiar to #43663

  • Instead of checking for Key::UP, Key::DOWN, Key::PAGEUP, Key::PAGEDOWN etc., we rather check for the action like ui_up or ui_down etc..
  • Also use AcceptDialog's register_text_enter functionality to consistently close a dialog when ENTER is pressed while the LineEdit has focus (instead of redirecting ENTER keys to e.g. the underlying Tree).
  • Unify the LineEdit filter behavior for the SceneTreeDialog and corresponding usages.
  • Improve OK Button disablement (something should be selected).

While at the editor/plugins/visual_shader_editor_plugin.cpp, found some related things I fixed while there:

  • grab focus for corresponding control when the varying dialogs are shown
  • shader code preview dialog always used the old size as minimum size, making the dialog bigger and bigger without the possibility to make the dialog smaller
  • shader code preview dialog is an AcceptDialog now, which enables the same navigation as for other dialogs (press ENTER to confirm, ESCAPE to close, was the only dialog where this does not work)

Affected dialogs and how to test them (Navigate while the LineEdit has focus, press ENTER to confirm selection):

  • CreateDialog (Create Node or Resource)
  • EditorCommandPalette (CTRL+Shift+P)
  • EditorHelpSearch (F1)
  • EditorQuickOpen (CTRL+Shift+O)
  • GridMapEditor (GridMap with MeshLibrary)
  • PropertySelector (MultiplayerSynchronizer -> add property)
  • SceneTreeDialog (@export var abc: NodePath -> Assign a Node, used in AnimationTrackEditor, ReplicationEditor)
  • ScriptEditorQuickOpen (edit Script -> CTRL+ALT+F)
  • ThemeTypeDialog (edit Theme -> add type)
  • VisualShaderEditor, including varying dialog and shader preview dialog (Shader Editor)

…n underlying Tree or ItemList.

- Instead of checking for Key::UP, Key::DOWN, Key::PAGEUP, Key::PAGEDOWN etc., we rather check for the action like 'ui_up' or 'ui_down'.
- Also use AcceptDialog's 'register_text_enter' functionality to consistently close a dialog when ENTER is pressed while the LineEdit has focus (instead of redirecting ENTER keys to e.g. the underlying Tree).
- Unify the LineEdit filter behavior for the SceneTreeDialog and corresponding usages
- Improve OK Button disablement (something should be selected)
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

VisualShader editor changes might use another check, as they are bigger and I'm not familiar with this area, but otherwise looks good.

Seeing all this duplicated code for handling navigation, I wonder if we should have a custom DialogLineEdit class that does it automatically for assigned Control.

@akien-mga akien-mga requested a review from Chaosus September 13, 2024 08:54
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Sep 13, 2024
@akien-mga akien-mga merged commit 4d35402 into godotengine:master Sep 16, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

4 participants