From: Benjamin <mailinglists_at_samuraipanda.com>
Date: Sat, 1 Feb 2003 07:01:55 -0800

Okay, looks like something bad happened during the upload, so the
compiled download version was corrupted. Anyway, it's fixed now and I
apologize for the problems. You can get it at
http://www.samuraipanda.com/bookmarks-20030131.zip.

Ben

-----Original Message-----
From: owner-rockbox_at_cool.haxx.se [mailto:owner-rockbox_at_cool.haxx.se] On
Behalf Of Benjamin
Sent: Friday, January 31, 2003 5:32 PM
To: rockbox_at_cool.haxx.se
Subject: Latest Bookmarking Patch


Hi All,
The latest version of my bookmarking patch (diffed off of rockbox-
daily-20030131) can be found at http://sourceforge.net/tracker/index.
php?func=detail&aid=669440&group_id=44306&atid=439120. A compiled
version can be found at: http://www.samuraipanda.com/bookmarks-20030131.
zip. Below are the changes in this version. Below that are changes
that were requested and I didn't implement and my reasoning why.
Below that is a list of TBDs/known issues.

Please note that this version will not work with previous bookmark
files. There is also a perl script included with the compiled version
that will update a 11-field bookmark to the updated format. If you
have problems updating bookmarks with it, send me a copy of the bookmark

file and I'll look into it.

Ben


Changes:
-----------------------------------------------------------------
---------
01. Autobookmarks are now stored in the first 4 bits of memory AF. You
may need to reset them the first time you load this patch. 02. Bookmark
functions(create/load) are now in their own menu off
of the main menu.
03. Default Auto-Bookmark and Auto-Load settings are OFF.
04. Name the attribute constant in the same form as the already existing

ones, i.e TREE_ATTR_BMARK
06. Bookmark format has been changed. Queue information has been
removed and the resume file has been moved to the end of the bookmark.
This breaks previous bookmark compatability, but I have written
a perl script that should update existing bookmarks (included with
the compiled version of this patch).
07. Bookmark uses a comma-separated syntax.
08. bookmark_create() was broken into two funcions. bookmark_write()
writes the bookmark; bookmark_create() creates the formated string.

09. Now using a common read_line()
10. .bmark is now the official extension for bookmark files (subject
to change).
11. Moved all strings to the english.lang file
12. strcpy() calls have been replace with calls to strncpy() calls
and some code was modified to check for overruns. I probably missed
others.
13. Indentation has been updated to 4 spaces and I tried to be more
C89 compliant, just for Daniel :-).
14. I thought I had done it before and some may be lingering, but
I ran bookmark.h and bookmark.c through a dos2unix utility.
15. On the select menu, bookmarks are now deleted using the ON+Play
key combination (I kept hitting the off key to exit).
16. The MP3 title is no longer displayed on the bookmark select menu.
This is because the new bookmark format does not store this information
anymore. This may be fixed in the future.
17. Creating a bookmark now indicates if a bookmark was created or not.
18. A bunch of code changes I'm sure I forgot about. 19. Updated to the
rockbox-daily-20030131 daily release.

Changes not done, but requested, along with my reasons why they are
not done.
-----------------------------------------------------------------
---------
01. (Björn Stenberg Stenberg) The Auto Load settings should be removed.
Use a single Auto Bookmark setting.
    I believe these should be two settings. The both occupy the
first four bits of memory AF

02. (Björn Stenberg) Don't use the resume settings as temporary
variables
when you load bookmarks
    This implimentation is based off of the resume functionality
all ready built into RockBox. Further, the global settings should
be over written once a bookmark is selected so that a user can resume
the next time they start the Rockbox up.

03. (Björn Stenberg) Don't assume the first bookmark in the file
is the auto bookmark. Mark it specifically.
    I don't want to have to search through a file just to find the
autobookmark. Further, I don't want to have to open the file and
read all the way through to determine if there is an auto-bookmark. The
design is such that if a bookmark file exists, auto-load will
ask if the user wants to load the first bookmark which is normally
reserved for the auto-bookmark.

04. (Björn Stenberg) Parse the bookmark file with strtok_r(), not
manually byte-by-byte
    Each time I try to use strtok_r(), memory seems to be corrupted. It
may be something I am doing wrong or a bug in strtok_r that
my code is revealing. I have included a strok_r version of
bookmark_parse()
called, surprisingly enough, bookmark_parse_strtok_r(). This fails
to work when loading a bookmark directly from the .bmark file, but
it will work if called as part of autoload.

05. (Björn Stenberg) Create a bookmark struct instead of passing
14 parameters to many functions.
    There is only one function that passes in 14 parameters,
bookmark_parse(). This data is then used to load a bookmark.
Consolodating his into
a structure would add more overhead to the code as this function
also validates the bookmarks or returns selected information.

06. (Björn Stenberg) Fix the code to work on players too (max 2
lines of text, different keys)
    I don't have a player to test this on and I can't get the simulator
to build on Win32 (and I don't feel like installing Linux).

07. (Björn Stenberg) Additionally, I want to separate between file
and playlist bookmarks.
    This implementation follows the basic design of Rockbox, which
is that it bookmarks playlists and directories. Further, my
understanding
of Björn's comment is that he wants the bookmark to be created for
the MP3 even if it's playing in the playlist. It's do-able, but
I don't voluteer for implementing it as that would require multiple
writes (one for the playlist, one for the MP3), which I think shouldn't
be done.

08. Centralized Bookmarking
    This can be done by modifing the
bookmark_generate_bookmark_file_name().
Doing so will change where and what name a bookmark is stored or
where the code looks to load a bookmark from. The main issue with
this is guareenteing unique names for each bookmark file as I don't
think duplicating the MP3 directories is a viable option. One possible
solution is a to generate an MD5 signature for each path+file
combination. This would almost guareentee uniqueness.

09. (Daniel Stenberg) bookmark_get_next_field() is overly exessive
and both sets and copies huge chunks of memory on each call. I can't
see any obvious error in your use of strtok_r() other than I think
you should solve the repeated invokes in a loop or similar.
    See 04.

Other Issues:
-----------------------------------------------------------------
---------
01. Develop better play capability. Current code is a duplicate
of tree.c:play_resume().
02. Bookmark/resume files are created, but they don't show up until
the next time the directory is loaded.
03. Need to come up with an icon for bookmark file.
04. Changing the shuffle mode while playing a filelist or a directory
can cause the incorrect index information to be stored in the bookmark,
which may cause the bookmark_load() function to load the wrong MP3.

05. The bookmark menu should should some identifying information
about what MP3 the bookmark is referencing. This used to happen, but it
was taken out for the new bookmark format.
Received on 2003-02-01