Awn

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

awn-rewrite-ua-support
Merge into trunk-rewrite

Revision history for this message

moonbeam (rcryderman) wrote on 2009-07-16:

> 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.

> 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.

« Back to merge proposal