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 permanently selected audio bus effects #85879

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

berrybus
Copy link
Contributor

@berrybus berrybus commented Dec 7, 2023

This allows the audio bus effects to be deselected if the focus changes to fix this bug. Here is a recording of the new behavior:

Screen.Recording.2023-12-06.at.11.28.41.PM.mov

This is my first PR, and I wasn't sure if this was the best way to do it. I'm open to any feedback. Thanks for taking a look!

Bugsquad edit:

@@ -500,6 +500,10 @@ void EditorAudioBus::_effect_selected() {
updating_bus = false;
}

void EditorAudioBus::_effects_deselected() {
effects->deselect_all();
Copy link
Contributor Author

@berrybus berrybus Dec 7, 2023

Choose a reason for hiding this comment

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

This works for some cases when focus is lost, but it's kind of annoying when your mouse is just hovering over another node like the scene editor. Is there a way to check "is left mouse button down" at this part? That way, it's clear that the user is intentionally clicking away.

Copy link
Member

Choose a reason for hiding this comment

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

No. Other docks don't deselect on focus lost (e.g. file dock), rather they do it when you click away. Though then there is inspector, which doesn't allow deselecting properties 🤷‍♂️

You could use "empty_clicked" signal for deselecting. This means you'd need to click the empty space under effects, but they'd be deselectable in a predictable way at least. The empty space in bus layout (between buses) could be used too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay, the empty click event makes more sense. I'll try that, thanks! I couldn't find a generic one for "clicked somewhere else /outside/ of the current panel," so I guess that doesn't exist?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, clicking another control at most can steal focus.

editor/editor_audio_buses.h Outdated Show resolved Hide resolved
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.

The solution could use some tweaks, as discussed in the comments, but it's already acceptable I think.

@akien-mga
Copy link
Member

Is this equivalent to #42035?

@KoBeWi
Copy link
Member

KoBeWi commented Dec 13, 2023

Yes, although the solution here is better.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Dec 13, 2023
@berrybus
Copy link
Contributor Author

berrybus commented Dec 13, 2023

I was planning to use empty_clicked on the effects Tree and some "click empty space" method on the audio buses pane, but I could not find an easy way to check an "click empty space" event on the BoxContainer or ScrollContainer of the audio buses , so I've just gone ahead and renamed the focus_exited function.

And ah, yes it looks like someone else already made this change 3 years ago haha. I suppose y'all can decide which PR to merge!

EDIT: I did a force push to clean the commit log, hopefully that's okay

@berrybus berrybus force-pushed the deselect-audio-effects branch from e53a383 to cf39cc9 Compare December 13, 2023 17:37
@KoBeWi
Copy link
Member

KoBeWi commented Dec 13, 2023

I could not find an easy way to check an "click empty space" event on the BoxContainer or ScrollContainer of the audio buses

For stuff like generic containers you could use gui_input signal and check for mouse click, but it's not worth it.

@YuriSizov YuriSizov merged commit 9d280f8 into godotengine:master Dec 14, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks, and congrats on your first merged Godot contribution!

@YuriSizov YuriSizov added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 14, 2023
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

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.

Unable to deselect Audio bus effects (including selecting nodes or files, switching tabs, etc.)
5 participants