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

[Alexander Strasser]

Ingo Brückl wrote:
> Alexander Strasser wrote on Thu, 3 Mar 2011 20:09:22 +0100:
> 
> >> 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.
> > IOW I leave further decision to you.
> 
> The main reason I did this was because I wanted to add freeing the fonts
> there.
> 
> Actually, the fonts would belong into the listItems structure as all the
> other skin items, too, but unfortunately their storage is currently realized
> differently. If the function would have kept its parameter, the whole would
> have looked that way:
> 
> void appInitStruct(listItems *item)
> {
>     frobnicate(&item->member);
>     ...
>     fntFreeFont();
> }
> 
> i.e. it would have been possible that there are different listItems but only
> one font array. As you might already see looking at the code (fntFreeFont()
> independent from item) this is wrong and could lead to mistakes because the
> fonts always belong to the skin items which means that there cannot be two
> listItems with only one font array.
> 
> So I had two alternatives. Either the fonts become part of the listItems
> structure, what would have required a lot of changes and only for the one
> purpose to have it in the only (global) variable appMplayer (i.e. all calls
> would only have been done with appMplayer anyway), or else make a independent
> treatment of listItems and fonts much less likely by design. I have decided
> in favour of the second, simpler way.

  I see... This is why I wrote the disclaimer. If I understood you right
it is the rest of the code that made you choose this way.

> > code is marginally simplified on two lines,
> 
> It was not about simplification of the code itself.

  OK, but you silently snipped the more important part after the ","!

  Which was my main point and it can't be argued with "I didn't want
to simplify at all".

> 
> > the init function now is less flexible, reusable and testable
> 
> It has only the one purpose of dealing with appMplayer (and this is exactly
> what it does now).

  I think you understood me correctly, but for the record I meant
less flexible, less reusable and less testable.

> > (because it now explicitly depends on global state)
> 
> That was the goal.
> 
> > future modifications might get harder/bigger (like allocating the structure
> > e.g. on the heap or renaming the global)
> 
> If this would become necessary, there would be a lot of other problems,
> because the global appMPlayer is *everwhere*.

  It is this way now. But maybe while improving things this explicit
dependencies would vanish. I am not against the global here but against
using it directly from everywhere in the call tree.

> It may seem that the init function has lost usefulness, but actually there
> are no other listItems (the listItems structure *is* the skin resp. the GUI
> application).

  I wanted to say even if there are no others (now or never will be) it
can still make sense to have the routine written to take an argument.

> I hope this clarifies why I did it.

  I like to think my points are still valid, but as I said I leave
further decisions to you. It just seemed as a step in the wrong
direction to me.

  Your first paragraph was enlightening and did clarify your choice.
I don't think there is need to go on arguing. I just wanted to point
these things out and understand your reasoning.

  So to be clear it is OK with me if you decided for the code to stay
as it is now. Thanks for taking the time and explaining me why you did
it that way.

  Alexander