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 menus and dropdowns requiring two clicks #101246

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

wlsnmrk
Copy link
Contributor

@wlsnmrk wlsnmrk commented Jan 7, 2025

Fixes #101214. Makes handling of all pressed status elements consistent on toggle buttons with ACTION_MODE_BUTTON_PRESSED.

menufix_crop.mp4

scene/gui/base_button.cpp Outdated Show resolved Hide resolved
@Hilderin
Copy link
Contributor

Hilderin commented Jan 8, 2025

I'm wondering, since it's easy to create side effects in this code, if conditions like that should be use instead.

if (p_event->is_pressed() && (mouse_button.is_null() || status.hovering)) {
	status.press_attempt = true;
	status.pressing_inside = true;
	if (!status.pressed_down_with_focus) {
		status.pressed_down_with_focus = true;
		emit_signal(SNAME("button_down"));
	}
}
if (!p_event->is_pressed()) {
	status.press_attempt = false;
	status.pressing_inside = false;
	if (status.pressed_down_with_focus) {
		status.pressed_down_with_focus = false;
		emit_signal(SNAME("button_up"));
	}
}

Worst case scenario, the button_down or button_up are not emitted but they are a lot less used.

@badsectoracula
Copy link
Contributor

I tried and works, however i'll agree with Hilderin that button_up should be emitted after any press event. I modified the code a bit to use an emit_button_up boolean which is checked at the end of the function above queue_draw (using the existing code) and it seemed to work fine, so i'd recommend that.

Fixes some editor menus and option buttons requiring two clicks to open
by checking status.pressed_down_with_focus separately from other press
status flags. Makes all pressed statuses consistent on toggle buttons
with ACTION_MODE_BUTTON_PRESSED.
@wlsnmrk
Copy link
Contributor Author

wlsnmrk commented Jan 8, 2025

I'm wondering, since it's easy to create side effects in this code, if conditions like that should be use instead.

Great suggestion, thanks. I've done exactly this. I've removed the special-case button-up signal for toggle buttons with ACTION_MODE_BUTTON_PRESSED, so with respect to the above discussion about signal order, this is currently preserving the somewhat inconsistent 4.3 behavior. If a fix/change is desired there, I can take a look at it.

@Hilderin
Copy link
Contributor

Hilderin commented Jan 8, 2025

I've removed the special-case button-up signal for toggle buttons with ACTION_MODE_BUTTON_PRESSED

Great!

so with respect to the above discussion about signal order, this is currently preserving the somewhat inconsistent 4.3 behavior. If a fix/change is desired there, I can take a look at it.

I'm not sure it causes any issue right now. I suggest we wait for an actual bug report on this.

I'll test a bit more your last commit but it looks good.

Copy link
Contributor

@Hilderin Hilderin left a comment

Choose a reason for hiding this comment

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

Test on a local build, everything in the editor and in the test project works fine.
Great job, thank you!

@akien-mga akien-mga merged commit bfa351c into godotengine:master Jan 8, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@wlsnmrk wlsnmrk deleted the dropdown-fix branch January 8, 2025 17:51
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.

Open an in-editor menu requires an extra click
6 participants