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: Latest Bookmarking Patch
From: Benjamin (mailinglists_at_samuraipanda.com)
Date: 2003-02-01


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()
occasionally.

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?

Ben

-----Original Message-----
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
To: Rockbox
Subject: RE: Latest Bookmarking Patch

On Sat, 1 Feb 2003, Benjamin wrote:

Hi Benjamin!

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:

  if(global_settings.resume)
    bla-bla;

  with your new code that looks like this:

  if(bookmark_autoload)
    your stuff;
  else
    bla-bla;

  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
you're
  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/



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