Awn

Code review comment for lp:~albyrock87/awn/awn-lucido

Revision history for this message
Alberto Aldegheri (albyrock87) wrote :

> Ok, there'll be many points here, but here we go:
>
> 1) Please add curly brackets to all ifs (& elses) per code guidelines (the
> one-liners).
> 2) awn_background_lucido_redraw - please add variables for
> AwnBackgroundLucidoPrivate and AwnBackground, everything caps looks just
> strange...
Ok..

> 3) _init_positions - remove 239-240 - it's init, so the assumption that you
> need to free something is wrong (otherwise it's not really init)
> 4) 315: starting a timer in init method is suspicious, ideally it should be
> started only once you know that you need to animate.
You're right..

> 5) 1082: shouldn't the value be 10?
Not in that case: if the first applet is a separator, that separator acts like a "background changer". If you put a separator in first position, the curve starts from bottom, otherwise from top.
> 6) 1224: What's the 10000?
It can be changed to : if (expand) w = -w; it's just for give "expand" a great importance in the "w" calculation.

> 7) I still don't like the "transparent" property, I suggest connecting to
> expose event instead of setting this property, since you mentioned you don't
> want to add panel-style switch in Separator's expose.
I mentioned I do want to add style switches to Separator :)
So.. what to do now?

> 8) Revert 1436 & 1442.
Ok

« Back to merge proposal