Rockbox mail archiveSubject: RE: Latest Bookmarking Patch
RE: Latest Bookmarking Patch
From: Benjamin <mailinglists_at_samuraipanda.com>
Date: Sat, 1 Feb 2003 07:50:33 -0800
1. I gotta find a better dos2unix tool :-). Anyway, I'll look into
getting rid of the CRLFs. Are they all in bookmark.h and bookmark.c or
are they in some of the other modified files?
2. Autobookmark is basically an automatically generated bookmark. As a
result, it uses a lot of the same functions as a manually created and
loaded bookmark. The main differences is how automated it is and where
it is stored (at the beginning of the file). By passing a true-false
value, I can use a lot of common code. That and this has been an
evolutionary process so there is probably some places I could have done
it a little more efficiently.
3. I'm not attached to how I get the buttons. I just found a sample
elsewhere in the code and used that. I believe I use button_get()
4. I'd use DEBUGF() if I could, but I haven't been able to compile the
simulator on my Windows boxes and I don't have a serial out on my
archos. I'll probably end up removing bookmark_debug_print() by the
final release. I only use it when I'm debugging a problem.
Can you see any place else I might be doing a buffer overrun?
From: owner-rockbox_at_cool.haxx.se [mailto:owner-rockbox_at_cool.haxx.se] On
Behalf Of Daniel Stenberg
Sent: Saturday, February 01, 2003 7:22 AM
Subject: RE: Latest Bookmarking Patch
On Sat, 1 Feb 2003, Benjamin wrote:
Thanks for all your work on this feature. I do however have some remarks
on your recent patch (dated feb 1 2003):
* It still contains CRLF newlines all over.
* On several places in the code you have changed a section like:
with your new code that looks like this:
Evidently, you alter the flow and I'm not convinced that you should do
that. Is it on purpose?
* I suggest you use button_get(true) instead of button_get_w_tmo() if
not gonna do anything special after the timeout has elapsed.
* The bookmark_debug_print() function should probably be replaced with
DEBUGF() in case you really want it to remain.
Otherwise I think it looks mighty fine!
-- Daniel Stenberg -- http://rockbox.haxx.se/ -- http://daniel.haxx.se/Received on 2003-02-01