Rockbox.org home
release
dev builds
extras
themes manual
wiki
device status forums
mailing lists
IRC bugs
patches
dev guide



Rockbox mail archive

Subject: Re: Bookmarks
From: Benjamin (mailinglists_at_samuraipanda.com)
Date: 2003-01-29


Below are my comments...Ben

>- Auto Bookmark is one option. It should occupy one bit, not eight. If
>you feel the triplet yes/ask/no is a must-have instead of a simple
boolan
>yes/no, that is still just two bits.
I can reduce it to 4 bits (00 00), but I think the yes/no/ask is
important and there should be two settings (see next comment1/29/2003
9:37AM).

>- The Auto Load settings should be removed. Use a single Auto Bookmark
setting.
I think there needs to be both and here is my reasoning. I prefer
to autobookmark and ask to autoload. One user prefers ask to both.
I can't see why someone would have no to one and yes to the other,
but users are weird creatures :-).

>- Don't use the resume settings as temporary variables when you
load bookmarks
The code currently leveraging the existing resume capability, so
it over-writes the existing resume infomration and then calls bookmark_play(),
which is just a copy of start_resume() with some modifications.
I have elinated a couple of places where the global_settings were
used and replaced them with local variables.

>- Put bookmarks in a separate sub menu, below recording settings
Agreed. I assume you meant have the Bookmark menu item located off
of the main menu and not as a sub menu of the recording settings.

>- Default auto bookmark setting should be "off"
Agreed.

>- Name the attribute constant in the same form as the already existing
>ones, i.e TREE_ATTR_BMARK
Agreed. BMF was my original extension and I never changed it.

>- Restore the deleted "restore=true; break;" before "case TREE_ATTR_BMF:
"
Whoops. I'll fix it.

>- Change bookmark format to be as simple as possible. (why are you
storing
>queue resume index?)
Queue information has been removed from the latest version(which
I'll post in the next couple of days along with a script to convert
existing bookmarks)

>- Use standard comma-separated syntax in the file. If you need a
filename,
>put it last on the line. Then it can contain anything.
My preference is to keep the * as the separator because it's one
of the few characters not usable on the Windows OS for a file or
directory name. Also, there are now two fields that are text based,
first the path/name and the second is the MP3 title (used for the
meaning system to make it easier to know which MP3 that you are listening
to). I could, theoretically, read the MP3 title from the MP3, but
that would add an additional read. By keeping everything needed
in a single file it reduces overall disk access.

>- Don't assume the first bookmark in the file is the auto bookmark.
Mark
>it specifically.
I didn't want to have to read through a file just to find the single
auto-bookmark located 2/3rd of the way down. Keeping it in a fixed,
guaranteed location reduces the number of reads.

>- Parse the bookmark file with strtok_r(), not manually byte-by-byte
There may be an issue with strtok_r() (or it may be my implementation).
When I use it instead of my version, the parse function works from
some function calls, but not others, even it the parameters stay
the same. I have yet to look into it further to understand what
is happening.

>- Simplify the code more. bookmark_create() should be max two snprintfs

>and two writes. Not 14.
Agreed and already done.

>- Create a bookmark struct instead of passing 14 parameters to many
functions.
The latest version only has one function with multiple parameters
(bookmark_parse()). It seems creating a structure for this function
alone seems to add more code then necessary.

>- Don't copy read_line() from settings. Export the one in settings
instead.
Agreed and already done.

>- Hardcode the bookmark extension and separator to ".bmark" and ",". I
>appreciate your current flexibility, but it clutters the code.
Easy enough change.

>- Move all strings to the english.lang file
I'll need help with this one as I'm not very familiar with the lang
file structure and concept.

>- Fix the code to work on players too (max 2 lines of text, different
keys)
I've avoid doing this because I don't have access to a player. If
someone wants to volunteer to code this up...

>
>Additionally, I want to separate between file and playlist bookmarks.
I believe these should be the same for simplicity. The way I understand
it, the Rockbox code treats a directory as a playlist, playing each
file in the directory, even if the user wants to play one file (unless
they set Repeat One). Is this incorrect?

>
>Also we need to solve the location problem. I suggest a single setting
>choosing between creating the file in /.rockbox or the same dir as the
>.m3u/.mp3. The .bmark file contains the path+name of the file, but
only
>uses it if there is not an .mp3/.m3u with the same name in the same
dir.
>Then we get both models in a single solution.
The code is easily modified to handle the bookmark being anywhere.
There is a single function, bookmark_create_name(), that controls
how the file is named and where the file is located). My main concern
with a centralized is the duplication of the file name (\music\sting\tracks.
m3u and \music\cure, the\tracks.m3u could potentially create the
same bookmark file name, tracks.m3u.bmark.
The only solutions I've thought of to prevent this risk is to base
the bookmark name off of full path, but I'd rather not have to create
a bookmark file name, that has the potential to get very large. (i.
e. \music\sting\fields of dreams\foo.mp3 becomes \.rockbox\bookmarks\music_sting_field
of dreams_foo_mp3.bmark)



Page was last modified "Jan 10 2012" The Rockbox Crew
aaa