Code review comment for lp://qastaging/~charlesk/indicator-sync/initial-impl

Revision history for this message
Lars Karlitski (larsu) wrote :

Some minor comments:

* app-menu-item:
  - app_menu_item_new: having initialization in _new makes it very hard for
    bindings (which typically only call g_object_new); a better place to
    initialize would be _init or _constructed
  - no need for g_return_if_fail in non-public gobject methods (there's enough
    type checking in gobject and the type casts)

* indicator-sync:
  - the hard-coded red error color can probably be solved by setting a class on
    the label which is then colored in the css (talk to Cimi)
  - afaict, calculate_prog_menuitem_preferred_width() does the same as
    gtk_label_set_width_chars()

* sync-client
  - there's no need to keep the GBindings around, they will be deleted
    automatically when one of the objects is freed
  - usually, the setter functions are called in set_property (this is the other
    way around)
  - sync client is meant to be used by applications? It probably shouldn't
    block the main loop then (for example: g_bus_get_sync in sync_client_init)
  - header file has private stuff in it (menuitem types)

* sync-enum:
  - interesting docstring on SyncState ;)

* sync-service:
  - why emit_exists when clients can just listen for a unique name to appear

* python bindings: why don't you use gobject-introspection? pygtk is deprecated

« Back to merge proposal