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

(succeeded by #30 ) Added "resume playlist" functionality #24

Closed
wants to merge 6 commits into from
Closed

(succeeded by #30 ) Added "resume playlist" functionality #24

wants to merge 6 commits into from

Conversation

Bockiii
Copy link

@Bockiii Bockiii commented Mar 21, 2018

Major overhaul of the trigger file. Open for discussion :)

Giving the rc interface a "play" command while paused will actually give you the feedback to enter "pause" again. Additionally, the code went for the wrong key. Since pause does pause/unpause, there is no need for "play"
@Bockiii Bockiii changed the title removed "play" as pause also unpauses Added "resume playlist" functionality Mar 25, 2018
@princemaxwell
Copy link
Contributor

Hey, great idea.
I tested it, but i had some issues.
One problem is, that some Files are having a ")" in it's filename and then the filename in *.track doesn't match, because "grep" is using ) as delimiter.
Can you fix this?

Added -F to grep to allow special characters in files.
Changed grep to sed to make files with special characters work better.
@Bockiii
Copy link
Author

Bockiii commented Mar 25, 2018

@princemaxwell Good catch, fixed that.

@MiczFlor
Copy link
Owner

@princemaxwell @Bockiii thanks guys. Lack of time requires me to delegate the authority for the decision to accept the pull requests to you :) the author and one peer reviewer. That should be good enough: can you both give me the thumbs up that this works?

One question regarding the new script: scripts/track_playstate.sh
https://github.com/MiczFlor/RPi-Jukebox-RFID/pull/24/files

It states: "Do this every 5 seconds. Can be tuned but will lower accuracy"
Does this mean it needs to be set up with the cronjob? If so, can you add a paragraph here or as a pull request in the appropriate documentation file how to do this?

If it goes into the documentation, it should go into this chapter, I think?
https://github.com/MiczFlor/RPi-Jukebox-RFID/blob/master/docs/CONFIGURE.md#auto-start-the-jukebox

@Bockiii
Copy link
Author

Bockiii commented Mar 26, 2018

Hi @MiczFlor , thats okay for me.

It doesn't need to run in the cronjob as it's started by every card (and killed with every card). it's also only running during vlc is active (as to check if people kill vlc by web).

This is my first time using github so I will do this: open new branch in my fork, commit all changes there and redo the PR as this one is kinda messy now. I will also add a note on how to use it in your documentation files (as a single trigger will restart last progress and double trigger will reset the playlist, that should be clarified).

@Bockiii
Copy link
Author

Bockiii commented Mar 28, 2018

I was preparing a clean commit and found an issue: This will probably cause issues with .txt files that have urls in them. I will debug that in the coming days and do the pr.

@Bockiii Bockiii changed the title Added "resume playlist" functionality (succeeded by #30 ) Added "resume playlist" functionality Apr 8, 2018
@Bockiii
Copy link
Author

Bockiii commented Apr 8, 2018

I posted a new PR for the clean commit. I tested this with .txt files and it worked.

This pull request was closed.
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.

3 participants