Code review comment for lp:~ted/indicator-appmenu/hud

Revision history for this message
Tony Espy (awe) wrote :

Ted, here are my comments. Nothing major, mostly style comments.
/t

indicator-tracker.h
-------------------

 * copyright -> 2012

 * why do use G_BEGIN_DECLS? Will this file ever be compiled as C++?

 * In general, I'm used to the style where when declaring a pointer variable, parameter, or struct member,
   the "*" prefixes the name without any whitespace. I'm also used to the same usage for return values,
   but it seems pretty common in Gtk/Glib code to use whitespace in this case...

eg.

struct _IndicatorTrackerIndicator {
        gchar * name;
 gchar * dbus_name;
        gchar * dbus_name_wellknown;
        gchar * dbus_object;
        gchar * prefix;
        gchar * icon;
};

OR

GList * indicator_tracker_get_indicators (IndicatorTracker * tracker);

---

indicator-tracker.c
-------------------

 * copyright -> 2012

 * Do we have in internal style-guide within PS or Systems? Just wondering about things like
   line length ( you have lots of very long lines > 80 chars ), or indentation rules ( tabs vs.
   spaces; you use tabs for instance, whereas recommended Gtk guidelines recommend two spaces ).

   GTK+ style guide dictates two spaces for indentation, and line length <= 80
   ( pretty standard C style ).

 * Same comment as header file re: your usage of whitespace and "*"s for variables, struct members,
   and param names. Your usage is inconsistent across the file.

 * Indentation is messed up: indicator_tracker_class_init().

 * Do you really need to add decls for all of your "static functions"?

 * Any feeling on using gtk-doc style comments for properties, signals, and
   public methods?

 * why the "return" statements at the end of void methods?

 * indicator_tracker_init() -- why do you bother initializing the priv members
   app_indicators, app_proxy_cancel, and app_indicators to NULL, when you initialize
   them to real objects a few lines later?

 * again, a style comment... you declare indicator_cnt in the middle of the block.
   Are C99 style declarations OK?

 * it took me awhile to discover that using ":" in struct initialization was a GNU
   C extension.

 * system_watch_vanished() -- not a big fan of your decrementing 'i' at the end
   of the loop, just to have it incremented in the for loop again. Isn't there
   a better way you can code this ( ie. with maybe a more complex iteration
   for clause then a simple i++ )?

  * l528: could you explain the comment "let's go nuclear"? I *think* what your
    saying is that forcing a name change causes the entire list to be retrieved?

  * l535: why do you initialize the ptrs but not the guint?

  * l593: why do g_warning() like in the following method?

review: Approve (indicatortracker)

« Back to merge proposal