Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Added feature to resume last played track and position. Closes #484, closes #565 #576

Closed
wants to merge 6 commits into from

Conversation

edisonchee
Copy link

No description provided.

@Pitros
Copy link
Contributor

Pitros commented Jan 30, 2016

Really nice feature, but the thing is that it doesn't work like it's supposed to for me. It does restore last played track, but it doesn't restore last position.

More details:

TypeError: Failed to set the 'currentTime' property on 'HTMLMediaElement': The provided double value is non-finite.

@edisonchee
Copy link
Author

Bummer! That happens when the track progress isn't saved into local storage on exit. I probably need to write a more robust handler for exiting the node process. Thanks for the feedback!

@Pitros
Copy link
Contributor

Pitros commented Jan 30, 2016

@edisonchee probably you should use close event on nw.js window (as well there might be difference in different OS behavior).

@edisonchee
Copy link
Author

@Pitros let me know if this works. I removed the handler on process.exit and used the handler in guiConfig.js instead. I only tested it on OSX, as I don't yet have access to Linux and Windows.

@Pitros
Copy link
Contributor

Pitros commented Jan 31, 2016

@edisonchee it still doesn't work, and it just can't work. Because in guiConfig.js you can't access player.elPlayer, so nothing will be saved. I don't know why it works for you, are you sure that you're commiting all the changes?

Uncaught TypeError: Cannot read property 'currentTime' of undefined

That's really weird, @weblancaster @jakejarrett you're both running OSX (at least you were) can you check if on OSX this somehow works?

@edisonchee
Copy link
Author

@Pitros Sorry about that; I'm not too sure why it worked on OSX before. I ran my build on a Windows 10 machine and got the same error as you. I've since done a fix and tested on both Windows and OSX.

Details:

  • I've opted to store and update track duration in a data attribute on the player-progress element, using the 'timeupdate' event listener in playerService.js. Storing directly into localStorage as the progress bar updates might slow things down later, as that is a synchronous file I/O operation.
  • In order to save the track duration into localStorage, I had to do it in two places; using both node process exit handler and guiConfig.close to do it.

Let me know if it works for you; I'll squash all the commits into one.

@weblancaster
Copy link
Member

I will try on Mac and then get back to you guys

@Pitros
Copy link
Contributor

Pitros commented Feb 1, 2016

@edisonchee now it seems to be working both track with queue and track time.

@weblancaster weblancaster changed the title Added feature to resume last played track and position. Closes #484 and #565 Added feature to resume last played track and position. Closes #484, closes #565 Feb 1, 2016
@weblancaster
Copy link
Member

@edisonchee would you fix the merge conflict please.. worked fine here.

@weblancaster
Copy link
Member

Also.. can you merge your commits into 1?

here's how to do it.

git log to see your commits (from top bottom) you can see that at the moment you have 4 so to merge them.. git reset --soft HEAD~4 then git add . git commit -m "your commit message"

@Pitros
Copy link
Contributor

Pitros commented Feb 1, 2016

@weblancaster most conflicts causes app.css.map, shoudn't we add it to .gitignore?

@weblancaster
Copy link
Member

I did it @Pitros I also removed app.css.map from the repo that way avoiding future conflicts..sorry for not have done this before :/

@edisonchee
Copy link
Author

Tried to deal with the merge conflict and ended up with some weirdness. How do I resolve any merge conflicts with the main repo?

@weblancaster
Copy link
Member

I will do it don't worry about it @edisonchee

@edisonchee
Copy link
Author

@weblancaster thanks! Great job with the app.

@weblancaster weblancaster self-assigned this Feb 3, 2016
weblancaster added a commit that referenced this pull request Feb 3, 2016
merge conflict, resume last played track and position, Closes #484, closes #565, closes #576
@weblancaster weblancaster mentioned this pull request Feb 19, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants