[MPlayer-cvslog] r33013 - in trunk/gui: app.c app.h skin/skin.c

[Alexander Strasser]

Hi,

ib wrote:
> Author: ib
> Date: Thu Mar  3 14:13:20 2011
> New Revision: 33013
> 
> Log:
> Remove parameter from appInitStruct() function.
> 
> Since there is only one listItems structure variable (the global appMPlayer),
> there is no need to pass it to the function.

  I don't think this change is desirable.  

I will just throw in a few things to consider:

  - code is marginally simplified on two lines,
    but it is much harder to read the init function (alone)
  - the init function now is less flexible, reusable and testable
    (because it now explicitly depends on global state)
  - future modifications might get harder/bigger
    (like allocating the structure e.g. on the heap or renaming the global)

  As for what I am concerned, I did not check other code surrounding
this and I don't have as much familiarity with the GUI as you have.
IOW I leave further decision to you.

> Modified:
>    trunk/gui/app.c
>    trunk/gui/app.h
>    trunk/gui/skin/skin.c
> 
> Modified: trunk/gui/app.c
> ==============================================================================
> --- trunk/gui/app.c	Thu Mar  3 13:30:50 2011	(r33012)
> +++ trunk/gui/app.c	Thu Mar  3 14:13:20 2011	(r33013)
> @@ -106,43 +106,43 @@ static void appClearItem(wItem *item)
>      item->tmp       = 0;
>  }
>  
> -void appInitStruct(listItems *item)
> +void appInitStruct(void)
>  {
>      int i;
>  
> -    for (i = 0; i < item->NumberOfMainItems; i++)
> -        appClearItem(&item->mainItems[i]);
> -    for (i = 0; i < item->NumberOfBarItems; i++)
> -        appClearItem(&item->barItems[i]);
> -    for (i = 0; i < item->NumberOfMenuItems; i++)
> -        appClearItem(&item->menuItems[i]);
> +    for (i = 0; i < appMPlayer.NumberOfMainItems; i++)
> +        appClearItem(&appMPlayer.mainItems[i]);
> +    for (i = 0; i < appMPlayer.NumberOfBarItems; i++)
> +        appClearItem(&appMPlayer.barItems[i]);
> +    for (i = 0; i < appMPlayer.NumberOfMenuItems; i++)
> +        appClearItem(&appMPlayer.menuItems[i]);
>  
> -    item->NumberOfMainItems = -1;
> -    memset(item->mainItems, 0, 256 * sizeof(wItem));
> +    appMPlayer.NumberOfMainItems = -1;
> +    memset(appMPlayer.mainItems, 0, 256 * sizeof(wItem));
>  
> -    item->NumberOfMenuItems = -1;
> -    memset(item->menuItems, 0, 64 * sizeof(wItem));
> +    appMPlayer.NumberOfMenuItems = -1;
> +    memset(appMPlayer.menuItems, 0, 64 * sizeof(wItem));
>  
> -    item->NumberOfBarItems = -1;
> -    memset(item->barItems, 0, 256 * sizeof(wItem));
> +    appMPlayer.NumberOfBarItems = -1;
> +    memset(appMPlayer.barItems, 0, 256 * sizeof(wItem));
>  
> -    appClearItem(&item->main);
> -    item->mainDecoration = 0;
> +    appClearItem(&appMPlayer.main);
> +    appMPlayer.mainDecoration = 0;
>  
> -    appClearItem(&item->sub);
> -    item->sub.width  = 0;
> -    item->sub.height = 0;
> -    item->sub.x      = -1;
> -    item->sub.y      = -1;
> +    appClearItem(&appMPlayer.sub);
> +    appMPlayer.sub.width  = 0;
> +    appMPlayer.sub.height = 0;
> +    appMPlayer.sub.x      = -1;
> +    appMPlayer.sub.y      = -1;
>  
> -    appClearItem(&item->menuBase);
> -    appClearItem(&item->menuSelected);
> +    appClearItem(&appMPlayer.menuBase);
> +    appClearItem(&appMPlayer.menuSelected);
>  
> -    item->sub.R         = item->sub.G = item->sub.B = 0;
> -    item->bar.R         = item->bar.G = item->bar.B = 0;
> -    item->main.R        = item->main.G = item->main.B = 0;
> -    item->barIsPresent  = 0;
> -    item->menuIsPresent = 0;
> +    appMPlayer.sub.R         = appMPlayer.sub.G = appMPlayer.sub.B = 0;
> +    appMPlayer.bar.R         = appMPlayer.bar.G = appMPlayer.bar.B = 0;
> +    appMPlayer.main.R        = appMPlayer.main.G = appMPlayer.main.B = 0;
> +    appMPlayer.barIsPresent  = 0;
> +    appMPlayer.menuIsPresent = 0;
>  }
>  
>  int appFindMessage(unsigned char *str)
> 
> Modified: trunk/gui/app.h
> ==============================================================================
> --- trunk/gui/app.h	Thu Mar  3 13:30:50 2011	(r33012)
> +++ trunk/gui/app.h	Thu Mar  3 14:13:20 2011	(r33013)
> @@ -189,7 +189,7 @@ typedef struct {
>  extern listItems appMPlayer;
>  
>  int appFindMessage(unsigned char *);
> -void appInitStruct(listItems *);
> +void appInitStruct(void);
>  void btnModify(int, float);
>  void btnSet(int, int);
>  
> 
> Modified: trunk/gui/skin/skin.c
> ==============================================================================
> --- trunk/gui/skin/skin.c	Thu Mar  3 13:30:50 2011	(r33012)
> +++ trunk/gui/skin/skin.c	Thu Mar  3 14:13:20 2011	(r33013)
> @@ -885,7 +885,7 @@ int skinRead(char *dname)
>  
>      mp_dbg(MSGT_GPLAYER, MSGL_DBG2, "[skin] file: %s\n", fn);
>  
> -    appInitStruct(&appMPlayer);
> +    appInitStruct();
>      fntFreeFont();
>  
>      linenumber = 0;

  I intentionally didn't snip the code, so people don't have to dig
their archives to verify for themselves my statements.

Greetings,
  Alexander