Awn

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

Revision history for this message
Michal Hruby (mhr3) wrote :

Here are my comments (all line numbers are from merge diff at LP):

1) Please remove applets/uawrapper/*
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 when AWN starts, so does it really have to be saved to permanent config?
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.
4) Why is touch_quark set on UaAlignment (line 599)?
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)
6) get_all_server_flags method looks suspicious - hash table is not allocated, but TRUE is returned. (line 639)
7) There are mixed tabs/spaces in awn-applet-manager.h
8) Please revert the whitespace changes to awn-applet-proxy.c
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)
10) Why are the x/y-scale in gtk_alignment_set sometimes set to 0.5 (line 1142)?

review: Needs Fixing (detailed)

« Back to merge proposal