Rockbox

Tasklist

FS#9407 - bookmarks overhaul

Attached to Project: Rockbox
Opened by Jonathan Gordon (jdgordon) - Tuesday, 16 September 2008, 08:32 GMT
Task Type Patches
Category Playlists
Status New
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

this patch is a WIP to update the bookmark system.

the change is to save "resume points" into the current .playlist_control file and copy that as a .bmark file. Right now it works fine to load and save (And even list the bookmark resume points).

What doesn't work yet (in bmark.3.diff) is the recent bookmarks menu item.


So far, the only downside I have found is that when you open a .bmark file the current playlist has to be stopped (it gets bookmarked automatically so if you cancel the .bmark it will resume anyway).
I'm hopeing that this isnt a big enough issue to reject the patch.
This task depends upon

Comment by Jonathan Gordon (jdgordon) - Tuesday, 16 September 2008, 08:46 GMT
ok, turns out sscanf() isnt compiled for targets so this patch uses parse_list() instead
Comment by Jonathan Gordon (jdgordon) - Tuesday, 16 September 2008, 10:16 GMT
ok not much changed but getting this one out early because i changed the .bmark resume position format a bit so it can be displayed a bit better (shows the track time instead of file offset..)

also, autoload from the filebrowser should work like svn (so even if the bookmark only has one point it will still show the listing (unless we want to get rid of that?) )
Comment by Magnus Holmgren (learman) - Tuesday, 16 September 2008, 11:00 GMT
It took a little while before I realized how to test it; i.e., by creating a bookmark from the WPS context menu (which I never do, in normal usage). After having passing that little hurdle, I noticed the following things missing:

* Restore of playlist seed, shuffle on/off and repeat mode (seed and shuffle are very important, IMO).
* Display resume index (nice, but not as important).
* Display resume time in minutes:seconds (important too, if you have multiple bookmarks for a file).

And looking at the code, it seems the list is limited to 10 bookmarks.
Comment by Magnus Holmgren (learman) - Tuesday, 16 September 2008, 11:02 GMT
For the record, my comments were for version 4, since version 5 was posted while I was testing version 4. :)
Comment by Jonathan Gordon (jdgordon) - Tuesday, 16 September 2008, 11:09 GMT
hmm, I thought playlist shuffle/seed was handled automatically.. need to check that.
the other two points are either fixed in 5, or will be in 6 which I'll upload in a few hours.

and yeah, should have mentinoed that you need to create with the WPS context menu... autocreating isnt done, and recent bmarks also isnt done.

thanks for testing
Comment by Jonathan Gordon (jdgordon) - Tuesday, 16 September 2008, 12:34 GMT
ok, v6... the list now looks exactly like svn.

I've also removed almost all the old code to make it a bit easier to work with. lots of TODO's if anyone wants to help out.

I'm not sure how I want to implement the recent bookmarks without stealing either lots of ram, or hitting the disk, suggestions welcome.

autocreating bookmarks if its set to "yes"
Comment by Jonathan Gordon (jdgordon) - Monday, 22 September 2008, 15:26 GMT
.6 looks totally broken :p
this version adds talking bmark list.

suggestions on how to do the recent bookmark list???
Comment by Magnus Holmgren (learman) - Monday, 22 September 2008, 18:16 GMT
I'm not sure what the problem is, really. What would you need the ram for?
Comment by Jonathan Gordon (jdgordon) - Sunday, 28 September 2008, 08:24 GMT
ok, this has the start of th recent bmark list, also it seems that creating bookmarks from a stopped playlist never actually worked (as in autocreate when playback is stopped)... this fixes it...
Comment by Steve Bavin (pondlife) - Monday, 29 September 2008, 09:19 GMT
Hi Jonathan,

Been away for the past fortnight so not yet had time to look at any of this, but I'd suggest that (long-term) the recent bookmarks list should be a more-general "recent stuff" list, i.e. not be tied into bookmarks at all. (I personally don't use bookmarks much, but would like to be able to just see the last n files I selected...).

This might be worth considering at this point, if not entirely OT.
Comment by Magnus Holmgren (learman) - Monday, 29 September 2008, 17:26 GMT
Had a quick look at version 9 diff, and I don't understand the explanation for bmark_stopped_position. I mean, it wasn't needed before, so why now? :) Is it because you removed audio_pause() from bookmark_autobookmark() perhaps?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 01 October 2008, 09:43 GMT
When stop is pressed the screen calls autobookmark() before calling audio_stop(). Now to be able to speak the menu (if its set to ask) we need to actually stop audio, but when audio is actually stopped the id3 struct is memset to 0 so the resume point in the actual track is lost. To get around this we could either add another var to global_status to store the seconds position (which is what bmark_stopped_position stores) so make it global to the bookmark file.

The other option is creating the bookmark in the playback thread but I decided thats a bad idea because pausing there for user input is probably a Bad Thing (TM)
Comment by Mark Ganson (TheMarkster) - Monday, 03 November 2008, 18:07 GMT
I use my player (Sansa e260v1) for listening to audiobooks. I often find myself rewinding immediately after resuming a bookmark, especially if it has been a couple days or so since last listening to a book. I was thinking it would be a nice feature for bookmarks if the bookmarked position could be automatically jumped back 5 or 10 seconds. That way, when you resume listening (usually to an audiobook or podcast) you would hear again the last 5 or 10 seconds prior to the bookmark, kind of re-acquainting yourself within the context of what you were listening to when you stopped.

The default behavior could be to just resume right where you left off, with the alternative options to backstep 5 seconds or 10 seconds (or perhaps some other figure, but I think 10 seconds seems about right). Probably the way to do it is with a jumpback variable (0, 5, or 10 seconds) which would be subtracted from the current position (in seconds) prior to creation of the bookmark, checking, of course, that we don't rewind past the beginning of the file. This might be a good place to add in such a feature while bookmarking is getting re-worked.
Comment by Magnus Holmgren (learman) - Monday, 03 November 2008, 18:28 GMT
I don't think this patch is the right place for auto-rewind. The resume point is a byte offset in the file, where we know there's data ready to be decoded (allowing a codec to just start buffering at that offset and start playing that data). Therefore, any auto-rewind should to be done by the playback code, either before writing or after reading the bookmark (in other words, the bookmark code isn't affected by it).
Comment by Steve Bavin (pondlife) - Thursday, 13 November 2008, 13:58 GMT
I agree - the auto-rewind is not bookmark-related. (I don't think it should be headphone-related either, but that's another story!)
Comment by Jonathan Gordon (jdgordon) - Monday, 22 December 2008, 14:00 GMT
bump/resync/minor update... I've added code to handle deleting bookmarks but nothing actually calls it yet so its untested. I've also had a go at making the mrb broswer a bit more readable (possibly needs more work), and also I've added the track name to the resume info instead of having to load the playlist to find out the track name, the advantage of this is we dont actually have to stop playback when browsing .bmark files (or the mrb browser), the disadvantage is that we dont have the option later of displaying info from the tracks id3 data (although this might be possible if the playlist control/.bmark file is loaded into the plugin buffer...

There are a few TODO's still in the patch, but im not sure what else is missing... so test please.

Loading...