-
Notifications
You must be signed in to change notification settings - Fork 862
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
Toggle active tab's audio mute with ctrl + m
#16226
Conversation
ctrl + m
db8ddc4
to
e915dc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
e915dc9
to
11806b0
Compare
Awesome! Thanks @simonhong. |
@@ -259,4 +261,14 @@ void ToggleVerticalTabStripFloatingMode(Browser* browser) { | |||
!prefs->GetBoolean(brave_tabs::kVerticalTabsFloatingEnabled)); | |||
} | |||
|
|||
void ToggleActiveTabAudioMute(Browser* browser) { | |||
WebContents* contents = browser->tab_strip_model()->GetActiveWebContents(); | |||
if (!contents || !contents->IsCurrentlyAudible()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only thought is that being able to mute a tab even if it's not actively playing sound would be nice - the other browsers work this way, and I'm sure people will get tripped up if they hit CTRL+M
when there's no audio playing and it doesn't mute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we can't assume that only one tab is playing audio. It would be rare but user could do that.
We are thinking to add another shortcut to mute all from active browser window. Maybe that's what you want?
cc @rebron
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that it would toggle mute on the currently-active tab, whether or not it, or any other tab, is currently playing any sound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm tempting to change current ctrl + m
to toggle all audible tabs(but maybe mostly only one) instead of toggling active tab only.
Although user could have multiple audible tabs but it's rare and that user would not use this ctrl + m
if it toggles all that tabs.
I think it's more usable when ctrl + m
toggles any tab as @GregJArnold suggested.
@rebron WDYT?
fix brave/brave-browser#26994
Resolves
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
ctrl + m
toggles audio mute