Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#6272 - Autocreate bookmark if bookmark file exists

Attached to Project: Rockbox
Opened by Stephane Doyon (sdoyon) - Wednesday, 01 November 2006, 06:28 GMT+2
Last edited by Torne Wuff (torne) - Monday, 05 July 2010, 18:43 GMT+2
Task Type Patches
Category Settings
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This patch adds a new option to the "Bookmark on Stop" setting, which
will automatically add a new bookmark on stop, but only if a bookmark
file already exists.

I want bookmarks for my audiobooks, but not generally when browsing my
music. I find it annoying to have all these .bmark files popping up all
over the place. With this option, I can indicate exactly which directory
/ playlist should be bookmarked: if I want bookmarking, I create the
first bookmark explicitly. Once that's done, future bookmarks are added
automatically. And if I did not create a bookmark by hand, none is
automatically saved.

How does that sound?

On a related subject: the dependency between "Bookmark on Stop" and
"Maintain a List of Recent Bookmarks?" is rather confusing. Wouldn't it
be clearer if:
-"Bookmark on Stop" did not have any options relating to "most recent
bookmarks",
-"Maintain a List of Recent Bookmarks?" had no / yes / ask,
-and possibly we could add an additional option to determine whether the
"most recent bookmarks" should be "unique only" or not...
   bmark-if_exists.diff (4.5 KiB) 
 rockbox-devel/apps/bookmark.c        |   29 ++++++++++++++++++++---------
 rockbox-devel/apps/lang/english.lang |   14 ++++++++++++++
 rockbox-devel/apps/settings.c        |    2 +-
 rockbox-devel/apps/settings.h        |    5 +++--
 rockbox-devel/apps/settings_menu.c   |    3 ++-
 5 files changed, 40 insertions(+), 13 deletions(-)

This task depends upon

Closed by  Torne Wuff (torne)
Monday, 05 July 2010, 18:43 GMT+2
Reason for closing:  Fixed
Additional comments about closing:  A simpler version of this has been committed in r27294 - it just adds an auto-update-bookmarks option, which will cause existing bookmark files to always be updated without prompting. If the file doesn't exist, the behaviour will be determined by bookmark on stop. This should cover all the combinations of desired behaviour here, and is simpler to read :)
Comment by Stephane Doyon (sdoyon) - Friday, 03 November 2006, 01:45 GMT+2
New version that works as advertised. My previous patch was broken and
would disallow manual/explicit bookmark creation...
   bmark-if_exists2.diff (6.5 KiB) 
 rockbox-devel/apps/bookmark.c        |   44 ++++++++++++++++++++++-------------
 rockbox-devel/apps/lang/english.lang |   14 +++++++++++
 rockbox-devel/apps/settings.c        |    2 -
 rockbox-devel/apps/settings.h        |    5 ++-
 rockbox-devel/apps/settings_menu.c   |    3 +-
 5 files changed, 48 insertions(+), 20 deletions(-)

Comment by James Teh (jteh) - Monday, 20 November 2006, 18:10 GMT+2
This appears to be broken against current cvs (the last hunk of bookmark.c fails). I'm not sure why, as the file doesn't seem to have been modified in that section. I think I've corrected it in my copy, but nevertheless, would you be able to upload a new patch? Thanks.
Comment by Stephane Doyon (sdoyon) - Monday, 20 November 2006, 20:39 GMT+2
Sure. I thought I had eliminated all traces of my other patches but
some were still there. Here's another patch, against CVS of a few minutes
ago and with nothing else applied (really this time). Sorry for the
fuss, and thanks for the feedback.
   bmark-if_exists3.diff (6.4 KiB) 
 rockbox-devel/apps/bookmark.c        |   40 ++++++++++++++++++++++-------------
 rockbox-devel/apps/lang/english.lang |   14 ++++++++++++
 rockbox-devel/apps/settings.c        |    2 -
 rockbox-devel/apps/settings.h        |    5 ++--
 rockbox-devel/apps/settings_menu.c   |    3 +-
 5 files changed, 46 insertions(+), 18 deletions(-)

Comment by blue_lizard (blue_lizard) - Friday, 24 November 2006, 15:55 GMT+2
I find this function very usefull. Thank you for writing this patch.
Comment by James Teh (jteh) - Tuesday, 28 November 2006, 15:15 GMT+2
Thanks for the new patch. I have applied it successfully and it works like a charm. I have wanted something like this for quite a while now (very tired of losing my spot in a book when I forget to manually bookmark) and this does the trick very nicely. Thanks once again!
Comment by James Teh (jteh) - Monday, 04 December 2006, 05:06 GMT+2
If "Maintain a list of recent bookmarks" is enabled with the current patch, a recent bookmark gets added when one presses stop even if "Bookmark on stop" is set to "If bookmark file already exists" and a bookmark file does not exist. This updated patch fixes that by moving the recent bookmark code below the bookmark file code. This way, if the file does not exist and the function returns, a recent bookmark is not generated either. This should not adversely affect any other functionality. I've also updated the patch for english.lang so it will apply against current CVS.
(application/octet-stream)    bmark-if_exists4.diff (6.7 KiB) 
 rockbox-devel/apps/bookmark.c        |   45 ++++++++++++++++++++++-------------
 rockbox-devel/apps/lang/english.lang |   14 ++++++++++
 rockbox-devel/apps/settings.c        |    2 -
 rockbox-devel/apps/settings.h        |    5 ++-
 rockbox-devel/apps/settings_menu.c   |    3 +-
 5 files changed, 49 insertions(+), 20 deletions(-)

Comment by James Teh (jteh) - Monday, 04 December 2006, 05:11 GMT+2
Ok, the patch again without corrupted line endings. Stupid Windows.
(application/octet-stream)    bmark-if_exists4a.diff (6.5 KiB) 
 rockbox-devel/apps/bookmark.c        |   45 ++++++++++++++++++++++-------------
 rockbox-devel/apps/lang/english.lang |   14 ++++++++++
 rockbox-devel/apps/settings.c        |    2 -
 rockbox-devel/apps/settings.h        |    5 ++-
 rockbox-devel/apps/settings_menu.c   |    3 +-
 5 files changed, 49 insertions(+), 20 deletions(-)

Comment by Stephane Doyon (sdoyon) - Monday, 04 December 2006, 15:11 GMT+2
jteh wrote:
> If "Maintain a list of recent bookmarks" is enabled with the current patch,
> a recent bookmark gets added when one presses stop even if "Bookmark on
> stop" is set to "If bookmark file already exists" and a bookmark file does
> not exist.

Yes, I realize I forgot to discuss that, but that's the way I would like
it to be. That way I can still go back to some of the last music I
browsed recently. I use .bmark files for books and podcasts, and the "most
recent bookmarks" seems most useful to me when it does record all the last
things you've listened to.

I suppose some people might like it differently. As I said at the top of
this ticket, seems to me the autobookmark preference could be structured
better.
Comment by James Teh (jteh) - Monday, 04 December 2006, 23:46 GMT+2
I am really uncertain as to how the bookmarking options need to be structured. I agree that they definitely need to be restructured, but your suggestion seems counter-intuitive to me personally. To my understanding, 'recent bookmarks' refers to just that: recently added bookmarks. If you press stop and no bookmark gets added, then it shouldn't be added to recent bookmarks because it was not added as a bookmark. Maybe we need to have a list called 'recently played' as well?

The problem with separating recent bookmarks from bookmark on stop is that if you want to only add a recent bookmark when you press stop, you can't. Your version of the patch does allow for this, but as I note above, this is ocunter-intuitive for some. Unfortunately, I don't presently have any better ideas...
Comment by Robert Jung (IpaqMan) - Sunday, 24 December 2006, 03:20 GMT+2
Thanks for the bookmarking patch. I also prefer not to have my DAP filled with "useless" bookmark files.

It would seem useful to have one file that it written on power off that contains a list of STOP/tracks. Only recent STOPs would be logged. Old STOPs would fall off the list. That would take care of "missing" STOP/tracks.

However, in any case, I prefer the auto bookmark feature to the bookmark on every STOP.
Comment by Mike Felix (MisterScoundrel) - Tuesday, 02 January 2007, 23:56 GMT+2
I like this patch. I modified it a bit to work better with the way I use bookmarks: rather than looking for a pre-existing bookmark file when it decides whether to write the bookmark, it looks for a flag per-directory (currently implemented by a file called "can_bookmark" in the directory where the bookmark would be stored). If not found, no bookmark is written.

I prefer it this way because I can put such a file in my /Podcasts directory and all my podcasts (which are stored in a subdirectory per feed) will be subject to bookmarking without having to create a new bookmark file every time I add a podcast. Also, I can delete old bookmark files at will to reduce clutter. All the while, my files in /Music will not create bookmarks because I don't have a "can_bookmark" file there.

I see both Stephane's and James' points about the Recent Bookmark menu. Ultimately, I think the bookmark options would be well-structured if they went something like this:

When to bookmark
- On stop
- - Yes
- - No
- On shutdown
- - Yes
- - No
Bookmark creation behavior
- Create bookmark file
- - Always
- - If directory/playlist is flagged as bookmarkable (something like what I explained above)
- - Always ask
- - Ask if directory is bookmarkable
- - Never
- Add to recent bookmarks menu
- - Always
- - If directory is bookmarkable
- - Always ask
- - Ask if directory is bookmarkable
- - Never
Bookmark file loading behavior
- Automatically
- Ask
- Only when manually selected
Recent bookmarks menu behavior
- Maximum length
- Permit multiple per playlist/directory?
- - Yes
- - No
Comment by Mike Felix (MisterScoundrel) - Wednesday, 24 January 2007, 21:14 GMT+2
This patch is now failing on settings.c.
Comment by Stephane Doyon (sdoyon) - Tuesday, 18 September 2007, 04:46 GMT+2
I do think the flag file is also a good idea, and I do agree the
bookmark options could use some restructuring... But in the meantime,
here's the updated patch which I use.
   bmark-if_exists.diff (5.9 KiB) 
 trunk/apps/bookmark.c        |   40 +++++++++++++++++++++++++++-------------
 trunk/apps/settings_list.c   |    5 +++--
 trunk/apps/settings.h        |    5 +++--
 trunk/apps/lang/english.lang |   14 ++++++++++++++
 4 files changed, 47 insertions(+), 17 deletions(-)

Comment by Stephane Doyon (sdoyon) - Wednesday, 26 September 2007, 03:20 GMT+2
The previous uploaded version depended on another patch.
This one should be much more useful :-).
   bmark-if_exists.diff (5.8 KiB) 
 trunk/apps/bookmark.c        |   38 +++++++++++++++++++++++++-------------
 trunk/apps/settings_list.c   |    5 +++--
 trunk/apps/settings.h        |    5 +++--
 trunk/apps/lang/english.lang |   14 ++++++++++++++
 4 files changed, 45 insertions(+), 17 deletions(-)

Comment by Costas Calamvokis (cc) - Friday, 19 October 2007, 18:06 GMT+2
Hey Stephane - thanks for creating and updating this patch! I was looking for a way to auto bookmark my radio shows
and books but not my music... and this does exactly what I want.

I guess the other things discussed above might be more flexible, but I think your patch is nice because it is very simple but still makes life much easier.
Comment by Costas Calamvokis (cc) - Saturday, 02 February 2008, 21:33 GMT+2
The patch was failing to apply - just due to some changes to the lines after the patched lines in settings.h. I have attached a new version that does apply cleanly.
   bmark-if_exists020208.diff (5.4 KiB) 
 apps/lang/english.lang |   14 ++++++++++++++
 apps/settings.h        |    5 +++--
 apps/settings_list.c   |    5 +++--
 apps/bookmark.c        |   34 +++++++++++++++++++++++-----------
 4 files changed, 43 insertions(+), 15 deletions(-)

Comment by Stephane Doyon (sdoyon) - Tuesday, 15 July 2008, 22:08 GMT+2
sync with svn
   bmark-if_exists.diff (5.8 KiB) 
 trunk/apps/lang/english.lang |   14 ++++++++++++++
 trunk/apps/bookmark.c        |   36 +++++++++++++++++++++++++-----------
 trunk/apps/settings_list.c   |    5 +++--
 trunk/apps/settings.h        |    5 +++--
 4 files changed, 45 insertions(+), 15 deletions(-)

Comment by Torne Wuff (torne) - Monday, 29 December 2008, 23:04 GMT+2
Synced with svn r19611

One minor functional change: make write_bookmark return false if the bookmark is not going to be written because no file exists. This more accurately reflects the other cases where false is returned if autobookmarking is disabled. Nothing actually checks this return code at present, but it seems like the right thing to do.
   bmark-if_exists-r19611.diff (5.5 KiB) 
 apps/bookmark.c        |   36 +++++++++++++++++++++++++-----------
 apps/lang/english.lang |   16 +++++++++++++++-
 apps/settings.h        |    5 +++--
 apps/settings_list.c   |    5 +++--
 4 files changed, 46 insertions(+), 16 deletions(-)

Comment by Torne Wuff (torne) - Monday, 29 December 2008, 23:08 GMT+2
Hm, flyspray's diff summary thing doesn't seem to like the '\ No newline at end of file' marker in lang/english.lang there. The patch applies correctly, though.
Comment by Marcus Lowe (skyfish) - Wednesday, 10 February 2010, 02:43 GMT+2
Could someone please be so kind to sync with svn?
Comment by Torne Wuff (torne) - Wednesday, 10 February 2010, 12:41 GMT+2
Done, but the only file that has a problem is english.lang and fixing language file sync is easy: just move the added language entries to the bottom of the file after the ones that have been added in svn.
   bmark-if_exists-r24579.diff (5.4 KiB) 
 apps/bookmark.c        |   36 +++++++++++++++++++++++++-----------
 apps/lang/english.lang |   14 ++++++++++++++
 apps/settings.h        |    1 +
 apps/settings_list.c   |    5 +++--
 4 files changed, 43 insertions(+), 13 deletions(-)

Comment by Marcus Lowe (skyfish) - Thursday, 11 February 2010, 07:58 GMT+2
Thank you, Torne.
I really wonder why this one isn't on svn -- it's not closed, so there are probably no immediate objections. As to its usefulness, it's imho a must-have for those with audiobooks, podcasts etc. Does not bloat the code or ui either. It is natural to bookmark large books and not to bookmark 12p leaflets. Plus, it's an option.
Comment by Torne Wuff (torne) - Thursday, 11 February 2010, 11:46 GMT+2
The settings for bookmarking are not structured brilliantly and this patch makes it more complicated. There was also discussion of some way to mark directories as needing automatic bookmarking, which hasn't happened (there has been discussion of a way to auto-load settings files for particular directories, which would remove the need for this patch entirely).

This is on my list of things to revisit (I am a committer) but I have not yet spent the time on revising the settings UI to a point where I consider it acceptable. A better way to solve this problem might come along before then :)
Comment by Marcus Lowe (skyfish) - Thursday, 11 February 2010, 13:24 GMT+2
Looked at the bookmark.c more carefully, I see your point.

I also was checking how easy / elegantly it would be to implement a conditional auto bookmarking based on the genre tag, with a user tags list («Audiobook», «Speech», «Podcast» etc). Or a custom one («Bookmarkable»), or smth. Or maybe both -- if a custom tag is not against the RB mojo. And I cannot see a nicer way than the Sdoyon's -- same thing, just with tags.

Marking directories seems smarter -- radically smarter if that would allow recursion, so you would need to mark only the root folders (2-4 ../ checks won't slow it down much). Just check for the '.bookmarkable' file recursively up, this seems simple... hm.

Torne, what would be your advice? Write the tag patch, write the rec dir marking patch, wait for you, or wait for bookmarking to be redone by unknown forces, using the Sdoyon's patch until then? First two options won't deliver much of a beauty, as I'm a less brighter coder than my cat supposes.
Comment by Torne Wuff (torne) - Thursday, 11 February 2010, 14:40 GMT+2
There have been various past discussions of how to differentiate podcasts/audiobooks/etc because there are way more settings than just this which people might find relevant: you don't want to crossfade audiobook chapters, and so on. A generic mechanism to handle settings being different for different files, whether that's directory-tree-based or tag-based, would probably be much appreciated if it were elegant and reasonably lightweight. I suspect there are forum discussions or even FS# entries for such things, but you can search yourself ;)

If you want something done in the short term that will be reasonably usable, then propose a nice way to structure the bookmark settings so that "if already exists" fits in neatly with everything else - I am not sure what one would be :) If it seems nice then someone (you, me, someone else) might implement it and then that can probably be committed as a short term measure until a much nicer way to handle this globally comes up.
Comment by Alex Parker (BigBambi) - Saturday, 05 June 2010, 23:37 GMT+2
Discussion between Torne and me at devcon
New setting "Auto update", yes or no - If yes, it updates any bookmarks that already exist, and if no it doesn't, regardless of how they were created. This means that whether bookmarks are created automatically, are asked for on stop, or are only created manually they will be updated next time you stop playback, if they already exist.

Loading...