From: Jonathan Gordon <jdgordy_at_gmail.com>
Date: Thu, 8 Feb 2007 19:42:38 +1100

On 08/02/07, Daniel Stenberg <daniel_at_rockbox.org> wrote:
> On Thu, 8 Feb 2007, mailer_at_svn.rockbox.org wrote:
>
> To me, this move is a wrong move. This introduces a complicated system that
> fewer people will understand/change and that will be bad for us in the long
> run.
>
> Some nits in the actual code follows:
>
> > /* -----------------------------------------------------------------
> > * vim: et sw=4 ts=8 sts=4 tw=78
> > */
>
> I realize you didn't add this chunk, but I personally think we should not have
> stuff like this in source files.
>
> > + if (m->flags&MENU_HAS_DESC)
> > + menu_callback= m->callback_and_desc->menu_callback;
> > + else menu_callback = m->menu_callback;
>
> This latest line is normally written on two separate lines in other Rockbox
> code, and I would appriciate if we stayed with that source code style. You use
> this same style in multiple places.
>
> > + DEBUGF("%x\n",setting->flags);
> > + if (setting->flags&F_INT_SETTING)
> > + {DEBUGF("boo");
>
> These DEBUGF() calls seem like leftovers that should be removed.
>
forgot about them... I've already seen them and removed them.
> > +#ifdef CONFIG_TUNER
> > +MENUITEM_FUNCTION(load_radio_screen, ID2P(LANG_FM_RADIO),
> > + (menu_function)radio_screen, dynamicitem_callback);
> > +#endif
> > +
> > +#include "settings_menu.h"
>
> ... here you #include a header in the "middle" of the C file. Why?
>
Isnt there a comment with them saying its temporary?
> > +struct choice_setting {
> > + void (*option_callback)(int);
> > + int count;
> > + unsigned char **desc;
> > +};
>
> And this is suddenly no longer following the 4-space indent tradition.
>
Not sure what happened there... ill fix it
> --
> Daniel Stenberg -- http://www.rockbox.org/ -- http://daniel.haxx.se/
>
Received on 2007-02-08