Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#7158 - Bookmark selection as a list.

Attached to Project: Rockbox
Opened by Magnus Holmgren (learman) - Saturday, 12 May 2007, 14:19 GMT+2
Last edited by Magnus Holmgren (learman) - Saturday, 26 May 2007, 12:45 GMT+2
Task Type Patches
Category User Interface
Status Closed
Assigned To Magnus Holmgren (learman)
Player Type All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This is a first, rough implementation of having the bookmark selection screen as a list. It's not finished, but it is complete enough to be usable. A proper version would include at least the following:

* Properly hooked up delete bookmark button.
* Optimized bookmark file reading (currently the file is opened, scanned and closed twice for each displayed item).
* Proper lang strings.

And perhaps the following:

* Merge the "Load bookmark?" screen with the list (first item would be "Don't resume", with second item selected on entry).
* Context menu for Select bookmark, Delete bookmark, Exit screen.
* Different display of the bookmark items.
* Different voicing.

Originally I had though of indicating shuffle mode using the list icon, but that icon is currently not available to the list, so I added the "Shuffle" text instead.

Comments and suggestions are appriciated.

There's also a screenshot, showing what it can look like.
(application/octet-stream)    bookmark.diff (10.8 KiB) 
 apps/bookmark.c |  284 +++++++++++++++++---------------------------------------
 1 file changed, 88 insertions(+), 196 deletions(-)

   Bookmark.png (4.1 KiB)
This task depends upon

Closed by  Magnus Holmgren (learman)
Saturday, 26 May 2007, 12:45 GMT+2
Reason for closing:  Accepted
Comment by Magnus Holmgren (learman) - Saturday, 12 May 2007, 16:46 GMT+2
Second alpha version. Changes:

* Small changes to bookmark display.
* Don't scroll all lines.
* Simple bookmark file buffering implemented, using the plugin buffer.

The buffering is quite simple, so list wrapping will always cause a reload (unless the file fits in the buffer). However, the plugin buffer should be larger that most bookmark files, so this shouldn't be a big problem.
(application/octet-stream)    bookmark_v2.diff (15.3 KiB) 
 apps/bookmark.c |  406 +++++++++++++++++++++++---------------------------------
 1 file changed, 170 insertions(+), 236 deletions(-)

   Bookmark_v2.png (8 KiB)
Comment by Magnus Holmgren (learman) - Sunday, 13 May 2007, 14:26 GMT+2
First beta version. Changes:

* Context menu added (for Resume and Delete).
* "Load bookmark?" screen merged with bookmark list.
* Button mappings updated.
* English.lang updated.
* Buffering tweaks.

This is pretty much what I had in mind, but suggestions are still welcome.
(application/octet-stream)    bookmark_v3.diff (39 KiB) 
 apps/action.h                   |    2 
 apps/lang/english.lang          |  214 +++++++++++----
 apps/bookmark.c                 |  545 +++++++++++++++++++---------------------
 apps/keymaps/keymap-ipod.c      |    4 
 apps/keymaps/keymap-player.c    |    4 
 apps/keymaps/keymap-x5.c        |    4 
 apps/keymaps/keymap-h1x0_h3x0.c |   14 -
 apps/keymaps/keymap-gigabeat.c  |   12 
 apps/keymaps/keymap-ifp7xx.c    |    3 
 apps/keymaps/keymap-recorder.c  |    4 
 apps/keymaps/keymap-e200.c      |    4 
 apps/keymaps/keymap-h10.c       |    4 
 apps/keymaps/keymap-ondio.c     |    4 
 apps/keymaps/keymap-av300.c     |    4 
 14 files changed, 441 insertions(+), 381 deletions(-)

Comment by Douglas Valentine (Dwyloc) - Sunday, 13 May 2007, 20:05 GMT+2
I use my DAPS a lot to listen to audio books and as such bookmarking is one of Rockbox's killer features for me.
So I installed your patch and have been testing it this afternoon, so far I am impressed and find it to be quite a
big improvement over the old bookmark screen.

The only thing is that I find the the playlist position number a bit confusing as I feel it should really start at
one not zero.

I also think it would be clearer what the number means if it was displayed as [Playlist track number / number of tracks in play list]
like [64/100].
Comment by Magnus Holmgren (learman) - Sunday, 13 May 2007, 22:15 GMT+2
Currently it just displays the number in the bookmark file, which apparently is zero-based. :) Easily fixed.

Getting number of tracks requires reading the playlist/directory. As it only needs to be done once, that's not a problem. However, it would be repeated for each bookmark, which I don't quite like... Making it clearer that it is the index would be nice though.
Comment by Douglas Valentine (Dwyloc) - Tuesday, 15 May 2007, 11:37 GMT+2
I was under the impression that you would only have to re-read the directory structure more than
once for the resent bookmarks screen, as normal bookmarks files would have the same number of entry
in the directory for play list for all entries.

But I quite agree if its going to slow things down a lot them its most likely not a good idea.
Comment by Magnus Holmgren (learman) - Tuesday, 15 May 2007, 13:15 GMT+2
I hadn't thought of the "Recent bookmarks" screen (I don't really use that one myself); there the entry count would indeed be different (and perhaps useful). But as you say, scanning every playlist/directory could make things slow.
Comment by Magnus Holmgren (learman) - Tuesday, 15 May 2007, 21:08 GMT+2
New version:

* Synced with svn.
* Fixed display of playlist position.
* Some more buffering tweaks and fixes.

Should be ready for commit soon...
(application/octet-stream)    bookmark_v4.diff (39.2 KiB) 
 apps/action.h                   |    2 
 apps/lang/english.lang          |  214 ++++++++++-----
 apps/bookmark.c                 |  556 ++++++++++++++++++++--------------------
 apps/keymaps/keymap-ipod.c      |    4 
 apps/keymaps/keymap-player.c    |    4 
 apps/keymaps/keymap-x5.c        |    4 
 apps/keymaps/keymap-h1x0_h3x0.c |   14 -
 apps/keymaps/keymap-gigabeat.c  |   12 
 apps/keymaps/keymap-ifp7xx.c    |    3 
 apps/keymaps/keymap-recorder.c  |    4 
 apps/keymaps/keymap-e200.c      |    4 
 apps/keymaps/keymap-h10.c       |    4 
 apps/keymaps/keymap-ondio.c     |    4 
 apps/keymaps/keymap-av300.c     |    4 
 14 files changed, 452 insertions(+), 381 deletions(-)

Comment by Douglas Valentine (Dwyloc) - Saturday, 19 May 2007, 17:14 GMT+2
Looks good.

I have tested the lasted version of your patch on both my h140 and my ipod nano, so when do you plan to commit it :)

Loading...