Rockbox mail archive

• Next message: Eric Linenberg: "Re[2]: Latest Bookmarking Patch"
• Previous message: Daniel Stenberg: "RE: Latest Bookmarking Patch"
• In reply to: Daniel Stenberg: "RE: Latest Bookmarking Patch"
• Next in thread: Eric Linenberg: "Re[2]: Latest Bookmarking Patch"
• Reply: Eric Linenberg: "Re[2]: Latest Bookmarking Patch"
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ] [ attachment ]

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/

• Next message: Eric Linenberg: "Re[2]: Latest Bookmarking Patch"
• Previous message: Daniel Stenberg: "RE: Latest Bookmarking Patch"
• In reply to: Daniel Stenberg: "RE: Latest Bookmarking Patch"
• Next in thread: Eric Linenberg: "Re[2]: Latest Bookmarking Patch"
• Reply: Eric Linenberg: "Re[2]: Latest Bookmarking Patch"
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ] [ attachment ]