Code review comment for lp:~awn-testing/awn/awn-rewrite-ua-support

About the dbus stuff, changing the interface sound possible, the
necessary stuff to do on screenlets's side shoudl not be too hard.

I need to take another look for the AppletManager code.

Julien Lavergne

Le 16 juil. 2009 à 16:27, moonbeam <email address hidden> a écrit :

>> Here are my comments (all line numbers are from merge diff at LP):
>>
>> 1) Please remove applets/uawrapper/*
> Done
>
>> 2) Do we really need both ua_list and ua_active_list to be saved in
>> config
>> client? There's for example a comment that ua_active_list must be
>> empty
>> AWN starts, so does it really have to be saved to permanent config?
> Yes, unfortunately we do. At least for the moment. Admittedly
> ua_active_list is ephemeral in nature. It exists so awn-settings
> can be used to manipulate the active screenlets.
>
>> 3) Please remove the g_debugs (for example 876), don't just comment
>> them out
>> (if not necessary for future development) - ok, most seem to be
>> gone now.'s
> There are still a few kicking around at the moment. I'd like to
> keep those specific ones in a bit longer.
>
>> 4) Why is touch_quark set on UaAlignment (line 599)?
> Leftover from duplication of code in create_applet(). I'll remove it.
>
>> 5) The UA has its special DBus object AND interface, I think it
>> should be part
>> of the instanced Panel object (though a separate interface).
>> Currently it is
>> multipanel-unfriendly. This would also remove the need to add dbus-
>> specifics
>> to AppletManager (*-glue.h and changes to Makefile.am)
> Beyond my time and abilities to do. Though I know we would like to
> keep the 0.4 mult-panel friendly I feel this can be dealt with in
> the 0.6 cycle.
>
>> 6) get_all_server_flags method looks suspicious - hash table is not
>> allocated,
>> but TRUE is returned. (line 639)
> Changed to return FALSE.
>
>> 7) There are mixed tabs/spaces in awn-applet-manager.h
>
>> 8) Please revert the whitespace changes to awn-applet-proxy.c
> Missed those.
>
>> 9) Wouldn't it be better if instead of each UaAlignment watching
>> the ua-
>> active-list, the AppletManager would do it? The logic behind is
>> very similar
>> to applet watching, so it does make sense to put it into
>> AppletManager.
>> (moreover it'd be a little faster)
> Maybe. I gave consideration to leaving it ther, it could have went
> either way. I chose to move as much into UAAlignment as possible
> for the moment. My inclination is to leave it as it is for now but
> possibly move it back into AppletManager at a later date.
>
>> 10) Why are the x/y-scale in gtk_alignment_set sometimes set to 0.5
>> (line
>> 1142)?
> Probably because they worked fine that way:-) I'l fix that before I
> push the changes.
>
> --
> https://code.launchpad.net/~awn-testing/awn/awn-rewrite-ua-support/+merge/8851
> You proposed lp:~awn-testing/awn/awn-rewrite-ua-support for merging.