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

Mute button #3165

Merged
merged 15 commits into from Mar 8, 2020
Merged

Mute button #3165

merged 15 commits into from Mar 8, 2020

Conversation

ghost
Copy link

@ghost ghost commented Mar 1, 2020

Closes #2876.

Added "Mute button" for MainPlayer, BackgroundPlayer and PopupPlayer.

mute_button-debug.zip

@TobiGr TobiGr added the player Issues related to any player (main, popup and background) label Mar 1, 2020
@ghost ghost changed the title Mute button, issue #2876 Mute button Mar 1, 2020
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

There are a few issues here and there, but code looks good. Thank you!

@Stypox
Copy link
Member

Stypox commented Mar 2, 2020

I am not sure if the mute button should be in the place you put it into. It is not symmetric and looks out of place imho. I think a better place would be the top bar, with a mute menu item that changes name and icon color based on whether mute is active or not. I would also show the menu item with the option "ifRoom", so that if there is no place it is shown in the three-dot menu.
Also, for the icon, make sure the color follows the theme (i.e. the "active" color with the white theme should be black).
IMG_20200302_160734

I tested the current apk and it works correctly, thanks ;-)

@Stypox Stypox self-assigned this Mar 3, 2020
@ghost
Copy link
Author

ghost commented Mar 4, 2020

Done! I think this functionality may require translation for other languages now ;)
mute_button-acitonbar.zip

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Code looks good, just a question, thank you :-D
I also tested the apk and it works correctly

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Sorry, I forgot to bring further the discussion about themes ;-)

@@ -394,6 +400,11 @@ protected void setShuffleButton(final ImageButton shuffleButton, final boolean s
shuffleButton.setImageAlpha(shuffleAlpha);
}

protected void setMuteButton(final ImageButton muteButton, final boolean isMuted) {
muteButton.setColorFilter(ContextCompat.getColor(getApplicationContext(), isMuted ? R.color.white : R.color.gray));
Copy link
Member

Choose a reason for hiding this comment

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

What if the theme is white theme? The active button color should be black in that case. I'd suggest adding two attributes in attrs.xml named something along the lines of button_active and button_inactive and giving them different color values in the themes.xml file, so that they are themed.

Copy link
Author

@ghost ghost Mar 5, 2020

Choose a reason for hiding this comment

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

White theme does not affect background color for Main video player. Background is always black for this player. So if icon would turn black in WhiteTheme, then it would become invisible ;)

Copy link
Member

Choose a reason for hiding this comment

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

Oh you are right, nevermind 🤦‍♂️
Then there should be different code for every player, at this point

Copy link
Author

Choose a reason for hiding this comment

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

For MainVideoPlayer icon change happens in setMuteButton (place you've mentioned above).
For PopUp and Background, icon change happens for queue in ServicePlayerActivity in onMaybeMuteChanged. I did it similarly to other buttons, (e.g onPlayModeChanged also in ServicePlayerActivity)

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Ok, sorry for the misunderstading, this is ready :-D

@Stypox Stypox merged commit 8fa29ff into TeamNewPipe:dev Mar 8, 2020
@B0pol B0pol mentioned this pull request Mar 13, 2020
1 task
@opusforlife2
Copy link
Collaborator

The grey color of the icon breaks consistency with all the others.

Is it not possible to have 2 white icons that switch to one another when toggled? One, a speaker with sound waves, and the other with no sound waves?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mute button
5 participants