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.

> +#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?

> +struct choice_setting {
> + void (*option_callback)(int);
> + int count;
> + unsigned char **desc;
> +};

And this is suddenly no longer following the 4-space indent tradition.

-- 
  Daniel Stenberg -- http://www.rockbox.org/ -- http://daniel.haxx.se/