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

Added persistence to seektime for playing songs #923

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

newhinton
Copy link
Contributor

This pr adds the ability to store the last known time of a song in the database. Currently if the app restarts or crashes, it loads the last known timestamp for the song, and starts playing from there. Each song has its own timestamp and starts always at this timestamp.

Todo/discussion:

When should this media player start from the remembered position?

  • Mark each song as remember position type
  • Generally, but make it opt in
  • Based on id3 tags (mainly genere, podcast, audiobook etc.)

I tend to 1. but i think 3. could also be a good idea (maybe even a combination of all 3)

Also i want to implement that if you play a full album, it should play the last played song at the last remembered time (obeying the rules mentioned above)

closes #841

@adrian-bl
Copy link
Member

I'd rather implement something like bookmarks:

If i understood the code correctly, it would always seek to the last known position of a song.
So if i skip a song at 02:00 and later go back to the song, it would start playing at 02:01.

That might be nice for an audiobook player, but we are foremost a music player in which such a behavior would be super confusing.

@newhinton
Copy link
Contributor Author

That is exactly why i wanted to know when to start from the last known timestamp.

My current implementation which i have yet to commit adds the ability to mark an album so that songs from this album are getting played from the last known timestamp. Everything else starts at the beginning.

Additionally, you can set it so that the default is inverted: Everything remembers, and you set an album to not remembered, but that is not the default

@newhinton
Copy link
Contributor Author

Now you have the full ui where the default does not differ from the current master. Only if you decide to change something, the track starts at the last position.

What i want to add is a button that starts the selected album at the last track that was played

@newhinton
Copy link
Contributor Author

@adrian-bl how do i trigger the databasemigration, it seems to ignore the updated versionstring (and subsequently crashes the app if you update an old version)

@adrian-bl
Copy link
Member

You probably already executed the app once after you updated DATABASE_VERSION but before the SQL was ready (android stores the latest version it ever saw).

Check the logcat - you can also try to increment the DB version again.

@newhinton
Copy link
Contributor Author

Yeah, i guess that was the error, my non emulator device worked flawlessly

@newhinton newhinton changed the title Added persistence to seektime for playing songs (WIP) Added persistence to seektime for playing songs Feb 28, 2019
@newhinton
Copy link
Contributor Author

This should be done now

You can mark an album to remember the last track and position, or you can set this app to remember it always (except you "unmark" an album)

If you set it to remember this in the default, it starts playing the clicked album in the libraryview, except you marked the album to not remember the position

@@ -419,6 +432,20 @@ private void pickSongs(Intent intent, final int action)
query.mode = modeForAction[effectiveAction];
PlaybackService.get(this).addSongs(query);

//Delay this a short time, because sometimes the playlist is not ready and the queue is empty so it cannot set the track properly
Copy link
Member

Choose a reason for hiding this comment

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

No: if anything, that should happen in a callback.

Sleeping a hardcoded amount of time will always be a source of bugs (and a horrible hack at best).

if (type == MediaUtils.TYPE_PLAYLIST) {
fm.add(CTX_MENU_RENAME_PLAYLIST, 0, R.drawable.menu_edit, R.string.rename).setIntent(rowData);
} else if (rowData.getBooleanExtra(LibraryAdapter.DATA_EXPANDABLE, false)) {
fm.add(CTX_MENU_EXPAND, 2, R.drawable.menu_expand, R.string.expand).setIntent(rowData);
}

int timestampstate = MediaLibrary.getAlbumUseSongTimestamp(this, rowData.getLongExtra("id", LibraryAdapter.INVALID_ID));
Copy link
Member

Choose a reason for hiding this comment

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

This would block the UI thread: you shouldn't run SQL queries in the main thread.

delay=5000;
//Log.d(TAG, "Long Delay: "+delay);
}
mHandler.postDelayed(this, delay);
Copy link
Member

Choose a reason for hiding this comment

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

Wait: so this code performs an SQL upadte ever 500 / 5000ms?

That's not a cheap operation and will have significant effects on the battery consumption of the app.
This must be event driven: polling the play state is just completely wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can this be event driven? As far as i know the mediaplayer does not provide a callback, so that polling is the only way (The documentation also suggests to use the getCurrentPosition() call to keep track of the playback progress)

Albeit, i have adjusted the code to stop updating when the playback is stopped, and increased the normal delay to 2 seconds, which at least reduces the load by 75%

What we can do additionally is to store the value not directly in the database, but in a shared preference every two seconds, and only update the database value every minute (and on application restart if it crashed for whatever reason, and always before we start a new track) but i am not sure if that is computationally more easy than just writing to the database

Copy link
Member

Choose a reason for hiding this comment

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

As far as i know the mediaplayer does not provide a callback

You want to save the position if the song was paused or changed: vanilla has hooks for that.

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.

[Feature Request] Save place of play location for books.
2 participants