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

Fix button SecondFunc trigger #2367

Merged

Conversation

AlvinSchiller
Copy link
Collaborator

@AlvinSchiller AlvinSchiller commented May 6, 2024

I had this change laying around for some time, but with the gathered reports in #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:

  • SecondFunc
    • only main function is executed if button is NOT held past hold_time
    • only secondary function is executed if button is held past hold_time
  • SecondFuncRepeat
    • only main function is executed if button is NOT held past hold_time
    • only secondary function is executed if button is held past hold_time and repeatedly if button is futher held past hold_time

Other hold_modes have not been affected and behave the same:

  • None (no changes)
    • main function is executed instantly
  • Repeat (no changes)
    • main function is executed instantly and repeatedly if button is held past hold_time
  • Postpone (no changes)
    • main function is executed if button is held past hold_time

closes #1666

@AlvinSchiller AlvinSchiller added enhancement legacy_v2 Issues, discussions and PRs related to Version 2.x labels May 6, 2024
@AlvinSchiller AlvinSchiller added this to the v2.8.0 milestone May 6, 2024
@AlvinSchiller AlvinSchiller self-assigned this May 6, 2024
@coveralls
Copy link

coveralls commented May 6, 2024

Pull Request Test Coverage Report for Build 9009259380

Details

  • 14 of 14 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 78.547%

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

💛 - Coveralls

@AlvinSchiller
Copy link
Collaborator Author

AlvinSchiller commented May 6, 2024

@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
Copy link
Collaborator

s-martin commented May 7, 2024

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
Copy link
Collaborator Author

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.

@AlvinSchiller AlvinSchiller merged commit f3b4b20 into MiczFlor:develop May 8, 2024
9 checks passed
@AlvinSchiller AlvinSchiller deleted the fix/button-func-trigger branch May 8, 2024 22:20
@s-martin s-martin mentioned this pull request May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement legacy_v2 Issues, discussions and PRs related to Version 2.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀One Button for Next Song and Skip Forward
3 participants