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.

Sign up for GitHub

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

Jump to bottom

Button shortcuts no longer "press" the Button. #71328

Merged
merged 1 commit into from
Jan 13, 2023
Merged

Button shortcuts no longer "press" the Button. #71328

merged 1 commit into from
Jan 13, 2023
+65 −28

Conversation

reduz
Copy link
Member

@reduz reduz commented Jan 13, 2023 •
edited
Loading

  • Button shortcuts were treated as generic input events on buttons. This means that to activate a button shortcut you had to press and release.
  • This logic is removed and now shortcuts always activate on press.
  • This makes the editor feel more responsive and solves problems related to this behavior.
  • Shortcut feedback is kept, but it is now a timed event after press.

Fixes #45033 and possibly others.
Fixes #40464.

@reduz reduz requested a review from a team as a code owner January 13, 2023 13:04
@KoBeWi KoBeWi added this to the 4.0 milestone Jan 13, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Jan 13, 2023 •
edited
Loading

This basically reverts (or more like supersedes) #63335
You forgot to update the docs btw.

@reduz reduz force-pushed the button-shortcuts-no-longer-press branch from 29ce71c to 1ed6094 Compare January 13, 2023 13:13
@EricEzaM
Copy link
Contributor

Also fixes #40464

@reduz reduz force-pushed the button-shortcuts-no-longer-press branch from 1ed6094 to 72ad688 Compare January 13, 2023 13:58
@reduz reduz requested a review from a team as a code owner January 13, 2023 13:58
@reduz
Copy link
Member Author

reduz commented Jan 13, 2023

@KoBeWi I restored shortcut feedback (works a little different now).

@akien-mga
Copy link
Member

akien-mga commented Jan 13, 2023 •
edited
Loading

Seems good to me.

Shortcut feedback is kept, but it is now a timed event after press.

I foresee potential requests to make the timer configurable... but we can see first if we can get away with a reasonable default. I guess we can also add a project setting for it eventually if there's demand.

Which BTW raises the question of whether folks really want to configure this on a per-button basis, or if a global toggle for shortcut feedback and duration wouldn't be better.

@reduz reduz force-pushed the button-shortcuts-no-longer-press branch from 72ad688 to 1f1b4c3 Compare January 13, 2023 14:24
@reduz
Copy link
Member Author

reduz commented Jan 13, 2023

@akien-mga made the timer configurable as a project setting

akien-mga
akien-mga reviewed Jan 13, 2023
View reviewed changes
scene/gui/base_button.cpp Outdated
if (shortcut_feedback_timer == nullptr) {
shortcut_feedback_timer = memnew(Timer);
add_child(shortcut_feedback_timer);
shortcut_feedback_timer->set_wait_time(GLOBAL_GET("gui/button/shortcut_feedback_highlight_time"));
Copy link
Member

@akien-mga akien-mga Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm always a bit unhappy about re-fetching the value every time it's needed when it's unlikely to change during the process. But this code won't be called multiple time per frame so I guess it's OK.

I still think there would be value in a dirty flag/caching system for GLOBAL_GET as attempted in #60926 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be done the same as SNAME if needed, I guess. But of course, may be better for another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Or it could be cached in the constructor and updated when project settings change (with #62038).

@KoBeWi
Copy link
Member

KoBeWi commented Jan 13, 2023

I think the default value 0.5 is too long and doesn't look good. Something like 0.2 is better. The button is highlighted for similar time as the shortcut is normally pressed.

@reduz reduz force-pushed the button-shortcuts-no-longer-press branch from 1f1b4c3 to 68c3125 Compare January 13, 2023 15:48
@reduz
Copy link
Member Author

reduz commented Jan 13, 2023

@KoBeWi changed!

@akien-mga
Copy link
Member

CC @Spartan322 as you had interest in disabling the shortcut feedback (which this still preserves but refactors).

The last question for me is this one:

Which BTW raises the question of whether folks really want to configure this on a per-button basis, or if a global toggle for shortcut feedback and duration wouldn't be better.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 13, 2023

Ah yes, another one-element section lol
image

a global toggle for shortcut feedback and duration wouldn't be better.

It can be easily achieved by setting the highlight time to 0.

@akien-mga
Copy link
Member

Ah yes, another one-element section lol

Maybe we can move to gui/timers/.

KoBeWi
KoBeWi reviewed Jan 13, 2023
View reviewed changes
doc/classes/BaseButton.xml Outdated Show resolved Hide resolved
@reduz reduz force-pushed the button-shortcuts-no-longer-press branch from 68c3125 to 740bcee Compare January 13, 2023 17:13
@reduz
Copy link
Member Author

reduz commented Jan 13, 2023

@KoBeWi done changes!

KoBeWi
KoBeWi reviewed Jan 13, 2023
View reviewed changes
scene/gui/base_button.cpp
@@ -267,6 +261,10 @@ BaseButton::DrawMode BaseButton::get_draw_mode() const {
return DRAW_DISABLED;
}

if (in_shortcut_feedback) {
Copy link
Member

Choose a reason for hiding this comment

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

This could be replaced by

Suggested change
if (in_shortcut_feedback) {
if (shortcut_feedback_timer && !shortcut_feedback_timer->is_stopped()) {

to avoid extra bool flag, but it's not important I guess.

@reduz
Button shortcuts no longer "press" the Button.
dd3a1b0 
* Button shortcuts were treated as generic input events on buttons. This means that to activate a button shortcut you had to press and release.
* This logic is removed and now shortcuts always activate on press.
* This makes the editor feel more responsive and solves problems related to this behavior.

Fixes godotengine#45033 and possibly others.
@reduz reduz force-pushed the button-shortcuts-no-longer-press branch from 740bcee to dd3a1b0 Compare January 13, 2023 17:43
@reduz reduz mentioned this pull request Jan 13, 2023
Closed
@YuriSizov YuriSizov merged commit 682ef35 into godotengine:master Jan 13, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@maiself maiself mentioned this pull request Jan 20, 2023
Closed
reduz added a commit to reduz/godot that referenced this pull request Jan 21, 2023
@reduz
Fix constant editor redraw after shortcut
51777a2 
Introduced by me by mistake on godotengine#71328. Fixes godotengine#71652.
@reduz reduz mentioned this pull request Jan 21, 2023
Merged
Streq pushed a commit to Streq/godot that referenced this pull request Feb 9, 2023
@reduz @Streq
Fix constant editor redraw after shortcut
bcfb73f 
Introduced by me by mistake on godotengine#71328. Fixes godotengine#71652.
This was referenced Jun 27, 2023
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga left review comments

@YuriSizov YuriSizov YuriSizov approved these changes

@KoBeWi KoBeWi KoBeWi approved these changes

Assignees
No one assigned
Labels
enhancement topic:gui topic:input
Projects
None yet
Milestone
4.0
5 participants
@reduz @KoBeWi @EricEzaM @akien-mga @YuriSizov