|
Rockbox mail archiveSubject: Re: BookmarksRe: Bookmarks
From: Benjamin <mailinglists_at_samuraipanda.com>
Date: Wed, 29 Jan 2003 09:02:55 -0800 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) Received on 2003-01-29 Page template was last modified "Tue Sep 7 00:00:02 2021" The Rockbox Crew -- Privacy Policy |