-
-
Notifications
You must be signed in to change notification settings - Fork 261
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
playerctl module: hide non-can-play players and use chrome and chromium as default value. #2256
Open
valdur55
wants to merge
1
commit into
ultrabug:master
Choose a base branch
from
valdur55:playerctl-hide_non_canplay
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Without digging in, Is
not player.props.can_play
alone enough withoutplayer_hide_non_canplay
config.... This seems like an mpris issue on chrome/chromium side.Do chrome/chromium change this flag when it's playing/not playing?
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.
Actually there are other chromium based browsers like brave which has same bug. Chromium issue tracker has already bug in their system: https://issues.chromium.org/issues/40703847
Even the issue is from older raport states same issue: JasonLG1979/gnome-shell-extension-mpris-indicator-button#10
Added some pictures about different "can" properties:
State when chromium is playing media
State when chromium media is paused:
State when chromium mpris is idle state:
VLC is opened but no media opened (just opened vlc, no media selected):
VLC is opened, media loaded and stopped playing media.
I don't wan't to blindly hide all players with can_play == false state because I prefer to see VLC media player on statusbar just after I opened it as empty player.
It littlebit seems like hide_non_can_play is too technical so even we can simplify it to level: hide media players stopped state and user provides list on media players which he want to hide.
I searched some chromium source code and found out where this all "can" properties false comes. I suspect that line is this: https://source.chromium.org/chromium/chromium/src/+/main:components/system_media_controls/linux/system_media_controls_linux.cc;l=288
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 have been out of scene with py3status. Just sharing thoughts.
I think it doesn't benefit users to display empty players. If anything, this is maybe something that should be added in first place to ensure better quality control/behavior.
I do think
hide_non_can_play
config is too specific that partially addresses chromium-based players and not that general... I would like an approach that that users can allow/deny (all)can_xxx_xxx
configs (either by a dict config or new placeholders). New placeholders that exposes boolean like this... is maybe hard for users to come up with a newformat_player
that excludes those players... Dict may be be easier.It should be easier to just ignore players that doesn't have
can_play
flag true because of things like... You saying you don't like this empty-player thing on chrome/chromium #2255 but are okay with this on vlc... I think it would make more sense to avoid all empty-players in first place... to keep it simple.This approach also may omit the need for adding
|{player}
at end offormat_player
config too.Try it for a while. See if this small but simple tweak works quite well.