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

Add up/down keys to inc/dec val in editor spin slider [3.x] #53090

Merged
merged 1 commit into from
Oct 5, 2021

Conversation

Razoric480
Copy link
Contributor

@Razoric480 Razoric480 commented Sep 26, 2021

3.x backport for #41855 , since signal connections work differently between versions and this needed a classDB binding addition.

@coppolaemilio
Copy link
Member

Any chance this can be merged since the 4.0 version already did?

@akien-mga
Copy link
Member

It needs an update to be in sync with what was merged in master (see #41855 (comment)).

@Razoric480
Copy link
Contributor Author

Wasn't sure if it was the case in 3.x since APPLE_STYLE_KEYS is used a lot more often than in 4.0. In any case, done and done.

@akien-mga
Copy link
Member

Wasn't sure if it was the case in 3.x since APPLE_STYLE_KEYS is used a lot more often than in 4.0. In any case, done and done.

This might be worth testing, I'm actually not 100% sure that @groud was correct in #41855 (CC @godotengine/macos).

@Razoric480
Copy link
Contributor Author

goes digging up macos virtual machine

@Razoric480
Copy link
Contributor Author

Razoric480 commented Sep 29, 2021

After testing, I've found the following:

  • get_alt() is not being read at all, and needs to be replaced with get_metakey()
  • get_control() is not being read, and needs to be replaced with get_command()

I'll test on 4.0 next, since the functions may have changed, but at least in 3.x we need the apple_style_keys define, so I'll remake the change.

Though if someone who is better versed in macos can let me know if I got those backwards, by all means.

EDIT: I can't actually test on 4.0 with my current setup due to vulkan incompatibility.

Comment on lines +212 to +216
#ifdef APPLE_STYLE_KEYS
} else if (k->get_metakey()) {
#else
} else if (k->get_alt()) {
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised by your findings on get_alt() vs get_metakey(), that doesn't seem to match other APPLE_STYLE_KEYS checks in the codebase.

But let's merge and see if any macOS user reports unexpected behavior.

@akien-mga akien-mga merged commit 86ee82c into godotengine:3.x Oct 5, 2021
@akien-mga
Copy link
Member

Thanks!

@Razoric480 Razoric480 deleted the key_up_down_spin_slider_32 branch October 5, 2021 14:31
@Mickeon
Copy link
Contributor

Mickeon commented Oct 26, 2021

Hey, I don't know if this should be its own issue. I don't think it should for now, since it's about this specific feature, but forgive me for doing a mistake.

When changing a value with the up/down keys, the value being displayed in the inspector doesn't update properly, or at least it's not consistent.

For example, considering this simple script:

extends Node
tool

export var rectangle := Rect2() setget _set_rectangle

func _set_rectangle(new: Rect2):
	rectangle = new
	rectangle.size.x = 256
	rectangle.size.y = clamp(rectangle.size.y, 0, 1)

I lock the rectangle's width to 256, and clamp its height between 0 and 1.
I used a Rect2 in the example, as it lacks any special way to export its variables, so features like editor limit ranges have to be written manually in the setter.

Editing the size of the rectangle works as expected, by sliding with the mouse. The value is properly modified, and what ends up displaying in the inspector is the result of the given operations.
0bd7ef42ffe066a47f11f989a0947afe

However, this is not the case when using the up/down keys. The value is actually being modified, but doesn't display as such until I leave the text box.

9ff4c7196ae455a495d7e22abb453aef

I acknowledge this is as a quirk of using a text input for numerical values, where having your input change as it's being written would lead to a horrible user experience. I do not believe it should behave the same way for this input method, however.

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.

5 participants