MiczFlor / RPi-Jukebox-RFID

A Raspberry Pi jukebox, playing local music, podcasts, web radio and streams triggered by RFID cards, web app or home automation. All plug and play via USB. GPIO scripts available.
http://phoniebox.de
MIT License
1.3k stars 394 forks source link

Fix button SecondFunc trigger #2367

Closed AlvinSchiller closed 1 month ago

AlvinSchiller commented 2 months ago

I had this change laying around for some time, but with the gathered reports in https://github.com/MiczFlor/RPi-Jukebox-RFID/issues/1666 it may be worth to bring it in.


Introduce changes to the button function calling behavior, to not trigger both function if the button is held. Though this behavior is documented, it is still counter intuitive and likely the uncommon case.

For the "SecondFunc" hold_mode the new behavior is:

Other hold_modes have not been affected and behave the same:

closes https://github.com/MiczFlor/RPi-Jukebox-RFID/issues/1666

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 9009259380

Details


Totals Coverage Status
Change from base Build 8928871385: 0.04%
Covered Lines: 454
Relevant Lines: 578

💛 - Coveralls
AlvinSchiller commented 2 months ago

@s-martin not sure if we should just change the behavior and explain the changes in the release notes (and needed updates for existing configurations). Or make this new behavior activatable via a new parameter for the button (or activate the old behavior as the new should be the more common case).

I personally dont see a usecase where its usefull to trigger both functions, but they maybe exist.

s-martin commented 1 month ago

For me the new behavior you implemented is more intuitive and consistent.

I don't have a comprehensive upgrade path for that behavior change. My suggestion would be to clearly state this as a breaking change in the release notes. Additionally we should make sure that we update all documentation (also in the wiki and maybe even make a quick search in the (closed) issues to add a comment, if necessary).

AlvinSchiller commented 1 month ago

I don't have a comprehensive upgrade path for that behavior change. My suggestion would be to clearly state this as a breaking change in the release notes. Additionally we should make sure that we update all documentation (also in the wiki and maybe even make a quick search in the (closed) issues to add a comment, if necessary).

I don't see an update path aswell. I added an alert to the component docs. The wiki didn't have any references that were this specific. All relevant tickets i found are already linked in the referenced issue. I will add a note to them if this is merged.