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

Introduce new PlayContentHandler to abstract Second Swipe #2452

Open
wants to merge 14 commits into
base: future3/develop
Choose a base branch
from

Conversation

pabera
Copy link
Collaborator

@pabera pabera commented Nov 3, 2024

Replaces abandoned #2330


Until now, 3 functions existed to be registered to content: 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, igoring the other 2 functions all together.

  • Refactor play_single, play_album or play_folder in PlayerMPD to become play_content
  • Adapting UI to assign only play_content command
  • Handle cards.yaml and rewrite commands to use play_with_reader (Backwards compatibility)

Maybe to be considered from old PR

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

@pabera pabera added the future3 Relates to future3 development label Nov 3, 2024
@pabera pabera added this to the v3.6 milestone Nov 3, 2024
@pabera pabera self-assigned this Nov 3, 2024
@pabera
Copy link
Collaborator Author

pabera commented Nov 3, 2024

@AlvinSchiller @s-martin @damaev

I think this code gets closer to "Second Swipe". But I have a doubt. See the following Flow Chart.

It makes me think.. "Second Swipe" should only be considered if the action comes from RFID (or other reader), nothing else.

E.g. swiping a card triggers this command: player.ctrl.play_album "Bibi & Tina" "Jetzt in Echt - Soundtrack zum Kinofilm"

But when I navigate in the WebApp to the same album of the same artist, the command that would trigger is the same.

That would be considered a "Second Swipe", at least for now. And I don't seem to be able to find a distinguisher.

Thoughts?

flowchart TD
    Start(["External Call<br>(e.g., Web UI, RFID)"]) --> playContent["player.play_content()<br>content, type, recursive"]
    
    subgraph PlayerMPD
        playContent --> validateContent["Validate content and create<br>PlayContent object"]
        validateContent --> handlerPlay["play_content_handler.play_content()"]
    end

    subgraph PlayContentHandler
        handlerPlay --> checkLastContent{"Is last_played_content<br>stored?"}
        checkLastContent -- "No" --> FirstSwipe["First Swipe:<br>- Run firstSwipe callbacks<br>- Play content<br>- Store as last_played_content"]
        
        checkLastContent -- "Yes" --> compareContent{"Is current content<br>same as last_played_content?"}
        compareContent -- "No" --> FirstSwipe
        compareContent -- "Yes" --> checkAction{"Is second_swipe_action<br>configured?"}
        
        checkAction -- "No" --> FirstSwipe
        checkAction -- "Yes" --> SecondSwipe["Second Swipe:<br>- Run secondSwipe callbacks<br>- Execute second_swipe_action"]
    end

    FirstSwipe --> End(["Content Playing"])
    SecondSwipe --> End
Loading

@s-martin
Copy link
Collaborator

s-martin commented Nov 4, 2024

I agree with your observations. Right now I don’t see a use case for second swipe from the web UI (or maybe MQTT?).

Looks like the simple (but maybe not so pretty) approach could be to pass an argument, if the call comes from an rfid reader.

@AlvinSchiller
Copy link
Collaborator

I like the idea so far. But i also think, triggering the "second swipe" from the ui, is not really a usecase, and could cause more confusion. So there needs to be a parameter, or a different function, to opt-in second swipe.

Also there are other features, that are mainly build for card swipes, like the syncing (triggered through play_card_callbacks).
These should not trigger from the WebUI.

@coveralls
Copy link

coveralls commented Nov 16, 2024

Pull Request Test Coverage Report for Build 11991199591

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 remained the same at 38.14%

Totals Coverage Status
Change from base Build 11768279903: 0.0%
Covered Lines: 492
Relevant Lines: 1290

💛 - Coveralls

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have improved the rpc script we could think about removing the (kind of redundant) rpc client written in C.

See also #2162

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be better now :-)

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.

4 participants