Rockbox mail archiveSubject: Bookmarks
From: Björn Stenberg <bjorn_at_haxx.se>
Date: Wed, 29 Jan 2003 10:08:09 +0100
> Trust me (since I developed the code), it's probably best that it
> not make it into the next public release. It's a patch that is less
> then perfect (I'm working on that) and can be done better.
I dove into the patch yesterday and here's a list of things I want addressed before merging:
- 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.
- The Auto Load settings should be removed. Use a single Auto Bookmark setting.
- Don't use the resume settings as temporary variables when you load bookmarks
- Put bookmarks in a separate sub menu, below recording settings
- Default auto bookmark setting should be "off"
- Name the attribute constant in the same form as the already existing ones, i.e TREE_ATTR_BMARK
- Restore the deleted "restore=true; break;" before "case TREE_ATTR_BMF:"
- Change bookmark format to be as simple as possible. (why are you storing queue resume index?)
- Use standard comma-separated syntax in the file. If you need a filename, put it last on the line. Then it can contain anything.
- Don't assume the first bookmark in the file is the auto bookmark. Mark it specifically.
- Parse the bookmark file with strtok_r(), not manually byte-by-byte
- Simplify the code more. bookmark_create() should be max two snprintfs and two writes. Not 14.
- Create a bookmark struct instead of passing 14 parameters to many functions.
- Don't copy read_line() from settings. Export the one in settings instead.
- Hardcode the bookmark extension and separator to ".bmark" and ",". I appreciate your current flexibility, but it clutters the code.
- Move all strings to the english.lang file
- Fix the code to work on players too (max 2 lines of text, different keys)
Additionally, I want to separate between file and playlist bookmarks.
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.
-- BjörnReceived on 2003-01-29