Skip to content

playerctl: apply button options selectively#2325

Open
lasers wants to merge 1 commit into
ultrabug:masterfrom
lasers:playerctl-add-buttons
Open

playerctl: apply button options selectively#2325
lasers wants to merge 1 commit into
ultrabug:masterfrom
lasers:playerctl-add-buttons

Conversation

@lasers

@lasers lasers commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

This clears out the default buttons.

I initially don't know whenever to leave button_play_pause alone... or to let users configure that themselves. If we leave button_play_pause at default, then an user can run into same problem with a different button. It is more clean not to assign any default buttons.

And this checks for duplicate buttons too.

/cc notifying @jdholtz author on buttons

Closes #2324

@lasers
lasers force-pushed the playerctl-add-buttons branch 3 times, most recently from 2f41327 to e1e9caf Compare April 20, 2026 18:29
@lasers lasers changed the title playerctl: add buttons playerctl: drop default buttons, avoid duplicate buttons Apr 20, 2026
@lasers
lasers force-pushed the playerctl-add-buttons branch 2 times, most recently from 0f8014a to 811bd30 Compare April 21, 2026 03:09
@lasers lasers changed the title playerctl: drop default buttons, avoid duplicate buttons playerctl: drop default buttons, validate buttons Apr 21, 2026
@lasers
lasers force-pushed the playerctl-add-buttons branch from 811bd30 to c6ce4a3 Compare April 21, 2026 11:54
@ultrabug

Copy link
Copy Markdown
Owner

Thanks @lasers ; that PR will break users using the module with default settings for ages.

Could we make it to actually respect the user's setting instead of resetting everything to None like this? I'm confused.

@lasers

lasers commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

User requires button_stop = None in his/her config first in order to get a working button_next = 3.

User gave it a random number as a workaround/solution.

This can happen with any modules with default buttons. This one just has the largest button choices.

Do we suggest users to assign default buttons with something else first?

My approach for this module with that many button choices was that there are no default buttons anymore and users just assign what they want to use.

Happy to hear your suggestions/solutions.

@ultrabug

Copy link
Copy Markdown
Owner

I understand but I'm also wary of breaking something being used seamlessly by possibly a large number of users vs one having a specific case even if it's indeed not smooth for her/him.

In this case my favored approach would be :

  • on the validation function step, check if user tried to use one of the default (1 or 3) button number by setting them on another button
  • if so, set the previously set button to None
  • else, keep the default

Does it make sense mate?

@lasers
lasers force-pushed the playerctl-add-buttons branch from c6ce4a3 to 60817ec Compare May 1, 2026 07:24
@lasers

lasers commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

OK. I added a commit. I like the first one. It's more neutral in the long run. I think it also could lead to users having button_play = 1 or button_play_pause = 1 in their config and not needing button_stop = 3.

The second one contains your favored approach. Just more post_config_hook code. Everything seems to work fine on my side. Please review and if you like this, I'll squash em and update PR title+commit. 👍

P.S. This kind of button code validation could be applied to all modules with buttons in the eventual future, just not in modules themselves. It'll do for now.

@lasers
lasers force-pushed the playerctl-add-buttons branch from 60817ec to 0a3f0c6 Compare May 1, 2026 07:42
@ultrabug

Copy link
Copy Markdown
Owner

I think the second and current one is my favored yes

@lasers
lasers force-pushed the playerctl-add-buttons branch from 0a3f0c6 to 914322d Compare July 17, 2026 15:55
@lasers lasers changed the title playerctl: drop default buttons, validate buttons playerctl: apply button options selectively Jul 17, 2026
@lasers
lasers force-pushed the playerctl-add-buttons branch from 914322d to 3843ecf Compare July 20, 2026 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

playerctl module incorrectly handles defaults

2 participants