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: play_card function with second_swipe support #2330

Closed

Conversation

pabera
Copy link
Collaborator

@pabera pabera commented Apr 10, 2024

Until now, 3 functions existed to be registered to a card: play_single, play_album and play_folder. None of these functions supported "Second Swipe". Instead, another function existed, called play_card which had Second Swipe support, but it ended up to call play_folder, ingoring the other 2 functions all together

This PR aims at solving this problem and to prepare for hoffie@c4805ce

  • Refactor play_card in PlayerMPD
  • Adapting UI to assign only play_card command instead of play_single, play_album or play_folder
  • Handle cards.yaml and rewrite commands to use play_card

@pabera pabera self-assigned this Apr 10, 2024
@pabera pabera added the future3 Relates to future3 development label Apr 10, 2024
@pabera pabera added this to the v3.6 milestone Apr 10, 2024
pabera referenced this pull request in hoffie/RPi-Jukebox-RFID Apr 10, 2024
@AlvinSchiller
Copy link
Collaborator

Regarding "second swipe":
I found it strange that it's implemented in the player, and that it also is triggered for the UI playback (apart its name).

That's definitely another behaviour known from V2.

Nevertheless would it not make more sense that it is an option on the folder/card, then an overall state? So different cards could have different settings?

@pabera
Copy link
Collaborator Author

pabera commented Apr 11, 2024

I found it strange that it's implemented in the player

I agree

.. and that it also is triggered for the UI playback (apart its name).

I don't think that's the case. See below explanation.

That's definitely another behaviour known from V2.

I haven't deep dived into the v2 implementation much.

Nevertheless would it not make more sense that it is an option on the folder/card, then an overall state? So different cards could have different settings?

I agree


Intuitively, we opted for the commands play_single, play_album, and play_folder in the UI and for card assignments, rather than using the existing play_card function. The latter served as a wrapper for a combination of a Second Swipe event and play_folder.

Initially, I questioned the necessity of play_card. It seemed feasible to integrate the second swipe functionality directly into play_single, play_album, and play_folder. However, this approach doesn't align with our intentions. If the non-play_card commands are executed from the UI, we aim to avoid triggering a Second Swipe event (as Alvin Schiller pointed out, if I understand correctly). Conversely, if the command is activated via the RFID reader, it should trigger a Second Swipe, depending on certain configurations. For the moment, let's set aside the discussion on configurations (though I concur that a default Second Swipe event should exist, with the possibility of being customized within individual card commands).

Nonetheless, combining the play_album (Player event) with a second_swipe (RFID event) could result in a next (Player event), as illustrated below:

if(second_swipe): next() # Avoid triggering play_album()

This is likely the rationale behind the original implementation within the Player, as it was the only entity capable of managing this logic.

I am dissatisfied with the current implementation of the second swipe functionality and propose its removal, with the intention of reintroducing it in a separate PR.

@pabera pabera changed the title Fix play_card function with second_swipe support fix: play_card function with second_swipe support Apr 12, 2024
@s-martin s-martin linked an issue Apr 15, 2024 that may be closed by this pull request
@s-martin
Copy link
Collaborator

#1698 is related and probably needs to be considered.

@damaev
Copy link
Contributor

damaev commented Jun 27, 2024

as my current setup (v3/devlop, place_not_swipe=true) does not resume after replacing, but playing the track again, i finally found this thread. However, it looks like a lot of people already dug into this topic in enhancement calls etc.

from my point of view (not sure if its correct) the second swipe action is dealt with in the mpd_player. however, why isn't this put into the cards itself like:
defining a general secondwipe behaviour for all cards .
allowing a second wipe action for each card.

How do you think about it?

@damaev
Copy link
Contributor

damaev commented Jul 6, 2024

see d9d40bf for a card-wrapper and the updated mpdplayer-script

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11652970324

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 38.14%

Totals Coverage Status
Change from base Build 8668006920: -0.1%
Covered Lines: 492
Relevant Lines: 1290

💛 - Coveralls

@pabera
Copy link
Collaborator Author

pabera commented Nov 3, 2024

@AlvinSchiller

Regarding "second swipe": I found it strange that it's implemented in the player, and that it also is triggered for the UI playback (apart its name).

That's definitely another behaviour known from V2.

Nevertheless would it not make more sense that it is an option on the folder/card, then an overall state? So different cards could have different settings?

Here is an explanation why Second Swipe is part of playermpd and not somewhere else. I agree with these thoughts at the moment, but maybe this could still be done differently?

""" # noqa: E501
# Warum ist "Second Swipe" im Player und nicht im RFID Reader?
# Second swipe ist abhängig vom Player State - nicht vom RFID state.
# Beispiel: RFID triggered Folder1, Web App triggered Folder2, RFID Folder1:
# Dann muss das 2. Mal Folder1 auch als "first swipe" gewertet werden.
# Wenn der RFID das basierend auf IDs macht, kann der nicht unterscheiden und glaubt es ist 2. Swipe.
# Beispiel 2: Jemand hat RFID Reader (oder 1x RFID und 1x Barcode Scanner oder so) angeschlossen. Liest zuerst Karte mit
# Reader 1 und dann mit Reader 2: Reader 2 weiß nicht, was bei Reader 1 passiert ist und denkt es ist 1. swipe.
# Beispiel 3: RFID trigered Folder1, Playlist läuft durch und hat schon gestoppt, dann wird die Karte wieder vorgehalten.
# Dann muss das als 1. Swipe gewertet werden
# Beispiel 4: RFID triggered "Folder1", dann wird Karte "Volume Up" aufgelegt, dann wieder Karte "Folder1": Auch das ist
# aus Sicht ders Playbacks 2nd Swipe
# 2nd Swipe ist keine im Reader festgelegte Funktion extra fur den Player.
#
# In der aktuellen Implementierung weiß der Player (der second "swipe" dekodiert) überhaupt nichts vom RFID.
# Im Prinzip gibt es zwei "Play" Funktionen: (1) play always from start und (2) play with toggle action.
# Die Web App ruft immer (1) auf und die RFID immer (2). Jetzt kann man sogar für einige Karten sagen
# immer (1) - also kein Second Swipe und für andere (2).
# Sollte der Reader das Swcond swipe dekodieren, muss aber der Reader den Status des Player kennen.
# Das ist allerdings ein Problem. In Version 2 ist das nicht aufgefallen,
# weil alles uber File I/Os lief - Thread safe ist das nicht!
#
# Beispiel: Second swipe bei anderen Funktionen, hier: WiFi on/off.
# Was die Karte Action tut ist ein Toggle. Der Toggle hängt vom Wifi State ab, den der RFID Kartenleser nicht kennt.
# Den kann der Leser auch nicht tracken. Der State kann ja auch über die Web App oder Kommandozeile geändert werden.
# Toggle (und 2nd Swipe generell) ist immer vom Status des Zielsystems abhängig und kann damit nur vom Zielsystem geändert
# werden. Bei Wifi also braucht man 3 Funktionen: on / off / toggle. Toggle ist dann first swipe / second swipe

@pabera
Copy link
Collaborator Author

pabera commented Nov 3, 2024

Abandoning in favor of #2452

@pabera pabera closed this Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future3 Relates to future3 development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Second swipe defect after playlist runs out
5 participants