Merge lp://qastaging/~ted/libindicator/loader into lp://qastaging/libindicator/0.4

Implements a basic loader that is stolen from indicator-applet. This should put it all in one place, for everyone.

107 +An interface for indicators to link to for creation.
108 +

This is brief to the point of being opaque.

135 +/**
136 + IndicatorObjectPrivate:
137 + @label: The label representing this indicator or #NULL if none.
138 + @icon: The icon representing this indicator or #NULL if none.
139 + @menu: The menu representing this indicator or #NULL if none.
140 +
141 + Private data for the object.
142 +*/

Is this private for libindicator or private for subclasses - I assume private for libindicator, but a little more clarity would help.

175 +/* Inititalize an instance */

Typo

 +static void
215 +indicator_object_finalize (GObject *object)
216 +{
217 +
218 + G_OBJECT_CLASS (indicator_object_parent_class)->finalize (object);
219 + return;
220 +}

return here is noise.

271 + /* A this point we allocate the object, any code beyond
272 + here needs to deallocate it if we're returning in an
273 + error'd state. */
274 + GObject * object = g_object_new(INDICATOR_OBJECT_TYPE, NULL);
275 + IndicatorObjectPrivate * priv = INDICATOR_OBJECT_GET_PRIVATE(object);
276 +
277 + /* The function for grabbing a label from the module
278 + execute it, and make sure everything is a-okay */
279 + get_label_t lget_label = NULL;
280 + if (!g_module_symbol(module, INDICATOR_GET_LABEL_S, (gpointer *)(&lget_label))) {
281 + g_warning("Unable to get '" INDICATOR_GET_LABEL_S "' symbol from module: %s", file);
282 + goto unrefandout;
283 + }
284 + if (lget_label == NULL) {

I prefer to put constants on the left in if statements, it makes typos like 'lget_label = NULL' more visually distinctive. Up to you though.

341 +/**
342 + indicator_object_get_label:
343 + @io: An #IndicatorObject.
344 +
345 + A function to get the label for a particular object. This
346 + function does not increase the refcount. That's your job
347 + if you want to do it.

Thats an uncommon idiom isn't it, in gtk land? Perhaps making this clear in the name would help (append _unreffed)? If I'm wrong then its fine as-is, of course.

377 +/**
378 + indicator_object_get_menu:
379 + @io: An #IndicatorObject.
380 +
381 + A function to get the menu for a particular object. This
382 + function does not increase the refcount. That's your job
383 + if you want to do it.

Ditto.

395 === added file 'libindicator/indicator-object.h'
396 --- libindicator/indicator-object.h 1970-01-01 00:00:00 +0000
397 +++ libindicator/indicator-object.h 2009-10-28 21:50:22 +0000
398 @@ -0,0 +1,60 @@
399 +/*
400 +An interface for indicators to link to for creation.

Same comment about clarity ;)

Tests look good.

The tools/* directory seems unused, is it needed?

744 === added directory 'tools'
745 === added file 'tools/Makefile.am'
746 --- tools/Makefile.am 1970-01-01 00:00:00 +0000
747 +++ tools/Makefile.am 2009-10-28 21:50:22 +0000
748 @@ -0,0 +1,1 @@
749 +#Something

Overall good, some things to tweak/improve then +1

On Tue, 2009-11-03 at 02:19 +0000, Robert Collins wrote:
> Review: Needs Fixing
> 107 +An interface for indicators to link to for creation.
> 108 +
>
> This is brief to the point of being opaque.

Fixed r368.

> 135 +/**
> 136 + IndicatorObjectPrivate:
> 137 + @label: The label representing this indicator or #NULL if none.
> 138 + @icon: The icon representing this indicator or #NULL if none.
> 139 + @menu: The menu representing this indicator or #NULL if none.
> 140 +
> 141 + Private data for the object.
> 142 +*/
>
> Is this private for libindicator or private for subclasses - I assume
> private for libindicator, but a little more clarity would help.

It's private for the object. Almost all GObject objects have this
private data area that is created on a per-instance area. It's a way to
have private variables in C based objects :)

> 175 +/* Inititalize an instance */
>
> Typo

Fixed r369

> +static void
> 215 +indicator_object_finalize (GObject *object)
> 216 +{
> 217 +
> 218 + G_OBJECT_CLASS (indicator_object_parent_class)->finalize (object);
> 219 + return;
> 220 +}
>
> return here is noise.

I'd actually argue the opposite. I'd argue that functions without
returns mean that you didn't think about how the function ends. In
reality, that whole function is noise, but it's part of the GTK
boilerplate because it's a pain to create when you do need it.

> 271 + /* A this point we allocate the object, any code beyond
> 272 + here needs to deallocate it if we're returning in an
> 273 + error'd state. */
> 274 + GObject * object = g_object_new(INDICATOR_OBJECT_TYPE, NULL);
> 275 + IndicatorObjectPrivate * priv = INDICATOR_OBJECT_GET_PRIVATE(object);
> 276 +
> 277 + /* The function for grabbing a label from the module
> 278 + execute it, and make sure everything is a-okay */
> 279 + get_label_t lget_label = NULL;
> 280 + if (!g_module_symbol(module, INDICATOR_GET_LABEL_S, (gpointer *)(&lget_label))) {
> 281 + g_warning("Unable to get '" INDICATOR_GET_LABEL_S "' symbol from module: %s", file);
> 282 + goto unrefandout;
> 283 + }
> 284 + if (lget_label == NULL) {
>
> I prefer to put constants on the left in if statements, it makes typos
> like 'lget_label = NULL' more visually distinctive. Up to you though.

I like them being on the right because I think it makes the code easier
to read. If I was explaining it to you in English I would say "If the
label is NULL" not "If NULL was the value of the label." In general, if
people have to think to much about the code, I think they're likely to
make more mistakes rather than less.

In the end, this is why I've been advocating use working with the
Coverity folks. I really want LP integration there. Sadly, I think I'm
the only one that's really excited about it :(

> 341 +/**
> 342 + indicator_object_get_label:
> 343 + @io: An #IndicatorObject.
> 344 +
> 345 + A function to get the label for a particular object. This
> 346 + function does not increase the refcount. That's your job
> 347 + if you want to do it.
>
> Thats an uncommon idiom isn't it, in gtk land? Perhaps making this
> clear in the name would help (append _unreffed)? If I'm wrong then
> its fine as-is, of course.

Unfortunate...

> 135 +/**
> 136 + IndicatorObjectPrivate:
> 137 + @label: The label representing this indicator or #NULL if none.
> 138 + @icon: The icon representing this indicator or #NULL if none.
> 139 + @menu: The menu representing this indicator or #NULL if none.
> 140 +
> 141 + Private data for the object.
> 142 +*/
>
> Is this private for libindicator or private for subclasses - I assume private
> for libindicator, but a little more clarity would help.

This is GObject-fu. It's the private data for an object, in order to keep it out of public structs. However, Ted, I think that we're all using this in a slightly sub-optimal way and perhaps it would be good to start switching the style that we use this.

Right now we've been defining FooPrivate in the .c file, and whenever it's needed we use FOO_GET_PRIVATE() in a function. Now that we're starting to move sealed items into private data in gtk+ the way we're doing it is to typedef this struct in the .h file and then have a pointer to it in FooObject...

typedef struct _FooObject FooObject;
typedef struct _FooObjectPrivate FooObjectPrivate;
struct _FooObject
{
   GObject parent_object;
   FooObjectPrivate *priv;
};

Then in the .c file, foo_object_init() sets obj->priv = FOO_OBJECT_GET_PRIVATE(obj);

The reason is that FOO_OBJECT_GET_PRIVATE() is much slower than just doing foo->priv. So I'm starting to switch over to using this style.

> 218 + G_OBJECT_CLASS (indicator_object_parent_class)->finalize (object);
> 219 + return;
> 220 +}
>
> return here is noise.

woot! :)

> 284 + if (lget_label == NULL) {
>
> I prefer to put constants on the left in if statements, it makes typos like
> 'lget_label = NULL' more visually distinctive. Up to you though.

Putting constants on the right is pretty much the style that's used everywhere in the desktop modules though. I understand the rationale here, but I'd rather Ted keep this the way he has it for consistency.

> 345 + A function to get the label for a particular object. This
> 346 + function does not increase the refcount. That's your job
> 347 + if you want to do it.
>
> Thats an uncommon idiom isn't it, in gtk land? Perhaps making this clear in
> the name would help (append _unreffed)? If I'm wrong then its fine as-is, of
> course.

I think it kind of depends on what's being returned and what it's likely to be returned for. If he's returning a widget, it's usually to either operate on it (e.g., set the label's text) or to insert it into a container or something. In the first case you don't want to retrieve a widget, do some operation, and then unref it. In the second case your container will ref it.

On Tue, 2009-11-03 at 16:18 +0000, Ted Gould wrote:
>
> > Is this private for libindicator or private for subclasses - I
> assume
> > private for libindicator, but a little more clarity would help.
>
> It's private for the object. Almost all GObject objects have this
> private data area that is created on a per-instance area. It's a way
> to
> have private variables in C based objects :)

Oh, yes I *know* that; what I mean is that the comment was too brief:
even for common idioms, unless there is too high a cost to do so, we
should aim for clarity.

It was the 'private data for this object' that was a little unclear -
not hugely so but I had to read it twice to be sure.

> > +static void
> > 215 +indicator_object_finalize (GObject *object)
> > 216 +{
> > 217 +
> > 218 + G_OBJECT_CLASS (indicator_object_parent_class)->finalize
> (object);
> > 219 + return;
> > 220 +}
> >
> > return here is noise.
>
> I'd actually argue the opposite. I'd argue that functions without
> returns mean that you didn't think about how the function ends. In
> reality, that whole function is noise, but it's part of the GTK
> boilerplate because it's a pain to create when you do need it.

I see Cody agree's with me :). A function that returns void, returns
void - it *is* noise to add a return there, because you /cannot/ return
anything. I'll accept your point in a function that returns non-void
(and so will compilers :)).

> In the end, this is why I've been advocating use working with the
> Coverity folks. I really want LP integration there. Sadly, I think
> I'm
> the only one that's really excited about it :(

That would rock! foo == NULL is fine, it was just an observation.

> > 341 +/**
> > 342 + indicator_object_get_label:
> > 343 + @io: An #IndicatorObject.
> > 344 +
> > 345 + A function to get the label for a particular object. This
> > 346 + function does not increase the refcount. That's your job
> > 347 + if you want to do it.
> >
> > Thats an uncommon idiom isn't it, in gtk land? Perhaps making this
> > clear in the name would help (append _unreffed)? If I'm wrong then
> > its fine as-is, of course.
>
> Unfortunately, it's rather inconsistent. But, since the common usage
> of
> this should be grabbing it and putting it into a container widget. So
> it'd only force an unref in the calling application and increase the
> chance of a runaway count.

Would it be horrible to *signal* that this does not take a ref:
indicator_object_get_label_(steal|unref|noref|...) ?

> > 377 +/**
> > 378 + indicator_object_get_menu:
> > 379 + @io: An #IndicatorObject.
> > 380 +
> > 381 + A function to get the menu for a particular object. This
> > 382 + function does not increase the refcount. That's your job
> > 383 + if you want to do it.
> >
> > Ditto.
>
> Ditto.

Ditto. :)

> > The tools/* directory seems unused, is it needed?

> Not currently. I had expected to do more with this branch, but since
> priorities got rescheduled it had to be cut back :( It should be used
> in the future hopefully as all the other indicators can't really be
> tested effectively until there is a loader tool that allows for
> test...

On Tue, 2009-11-03 at 15:54 +0000, Cody Russell wrote:
> > 135 +/**
> > 136 + IndicatorObjectPrivate:
> > 137 + @label: The label representing this indicator or #NULL if none.
> > 138 + @icon: The icon representing this indicator or #NULL if none.
> > 139 + @menu: The menu representing this indicator or #NULL if none.
> > 140 +
> > 141 + Private data for the object.
> > 142 +*/
> >
> > Is this private for libindicator or private for subclasses - I assume private
> > for libindicator, but a little more clarity would help.
>
> This is GObject-fu. It's the private data for an object, in order to keep it out of public structs. However, Ted, I think that we're all using this in a slightly sub-optimal way and perhaps it would be good to start switching the style that we use this.
>
> Right now we've been defining FooPrivate in the .c file, and whenever it's needed we use FOO_GET_PRIVATE() in a function. Now that we're starting to move sealed items into private data in gtk+ the way we're doing it is to typedef this struct in the .h file and then have a pointer to it in FooObject...
>
> typedef struct _FooObject FooObject;
> typedef struct _FooObjectPrivate FooObjectPrivate;
> struct _FooObject
> {
> GObject parent_object;
> FooObjectPrivate *priv;
> };
>
> Then in the .c file, foo_object_init() sets obj->priv = FOO_OBJECT_GET_PRIVATE(obj);
>
>
> The reason is that FOO_OBJECT_GET_PRIVATE() is much slower than just doing foo->priv. So I'm starting to switch over to using this style.

This works fine for me.

-Rob

On Tue, 2009-11-03 at 22:45 +0000, Robert Collins wrote:
> On Tue, 2009-11-03 at 15:54 +0000, Cody Russell wrote:
> > > 135 +/**
> > > 136 + IndicatorObjectPrivate:
> > > 137 + @label: The label representing this indicator or #NULL if none.
> > > 138 + @icon: The icon representing this indicator or #NULL if none.
> > > 139 + @menu: The menu representing this indicator or #NULL if none.
> > > 140 +
> > > 141 + Private data for the object.
> > > 142 +*/
> > >
> > > Is this private for libindicator or private for subclasses - I assume private
> > > for libindicator, but a little more clarity would help.
> >
> > This is GObject-fu. It's the private data for an object, in order to keep it out of public structs. However, Ted, I think that we're all using this in a slightly sub-optimal way and perhaps it would be good to start switching the style that we use this.
> >
> > Right now we've been defining FooPrivate in the .c file, and whenever it's needed we use FOO_GET_PRIVATE() in a function. Now that we're starting to move sealed items into private data in gtk+ the way we're doing it is to typedef this struct in the .h file and then have a pointer to it in FooObject...
> >
> > typedef struct _FooObject FooObject;
> > typedef struct _FooObjectPrivate FooObjectPrivate;
> > struct _FooObject
> > {
> > GObject parent_object;
> > FooObjectPrivate *priv;
> > };
> >
> > Then in the .c file, foo_object_init() sets obj->priv = FOO_OBJECT_GET_PRIVATE(obj);
> >
> >
> > The reason is that FOO_OBJECT_GET_PRIVATE() is much slower than just doing foo->priv. So I'm starting to switch over to using this style.
>
> This works fine for me.

Let's do this for new objects, I don't see a reason to modify older ones
to do it.

  --Ted

On Tue, 2009-11-03 at 22:42 +0000, Robert Collins wrote:
> On Tue, 2009-11-03 at 16:18 +0000, Ted Gould wrote:
> >
> > > Is this private for libindicator or private for subclasses - I
> > assume
> > > private for libindicator, but a little more clarity would help.
> >
> > It's private for the object. Almost all GObject objects have this
> > private data area that is created on a per-instance area. It's a way
> > to
> > have private variables in C based objects :)
>
> Oh, yes I *know* that; what I mean is that the comment was too brief:
> even for common idioms, unless there is too high a cost to do so, we
> should aim for clarity.
>
> It was the 'private data for this object' that was a little unclear -
> not hugely so but I had to read it twice to be sure.

r370

> > > +static void
> > > 215 +indicator_object_finalize (GObject *object)
> > > 216 +{
> > > 217 +
> > > 218 + G_OBJECT_CLASS (indicator_object_parent_class)->finalize
> > (object);
> > > 219 + return;
> > > 220 +}
> > >
> > > return here is noise.
> >
> > I'd actually argue the opposite. I'd argue that functions without
> > returns mean that you didn't think about how the function ends. In
> > reality, that whole function is noise, but it's part of the GTK
> > boilerplate because it's a pain to create when you do need it.
>
> I see Cody agree's with me :). A function that returns void, returns
> void - it *is* noise to add a return there, because you /cannot/ return
> anything. I'll accept your point in a function that returns non-void
> (and so will compilers :)).

No, it's explicitly stating something that is otherwise implicit. For
instance the return has value in a situation like this, even though no
value is returned.

void
myfunc (void * myvar) {
  if (myvar == NULL)
    return;

  /* do stuff */

  return;
}

Both of them have value because they're both signaling points where the
function would end. Likewise something like this would provide value:

void
myfunc (void * myvar) {
  if (myvar == NULL)
    goto end;

  /* do stuff */

  return;
end:
  g_warning("NULL input");
  return;
}

You're just counting on the '}' at the end of the function putting an
implied return there. I'm saying we should be explicit about the ending
of a function because it's a very important thing in the function.

> > > 341 +/**
> > > 342 + indicator_object_get_label:
> > > 343 + @io: An #IndicatorObject.
> > > 344 +
> > > 345 + A function to get the label for a particular object. This
> > > 346 + function does not increase the refcount. That's your job
> > > 347 + if you want to do it.
> > >
> > > Thats an uncommon idiom isn't it, in gtk land? Perhaps making this
> > > clear in the name would help (append _unreffed)? If I'm wrong then
> > > its fine as-is, of course.
> >
> > Unfortunately, it's rather inconsistent. But, since the common usage
> > of
> > this should be grabbing it and putting it into a container widget. So
> > it'd only force an unref in the calling application and increase the
> > chance of a runaway count.
>
> Would it be horrible to *signal* that this does not take a ref:
> indicator_object_get_label_(steal|unref|noref|...) ...

On Tue, 2009-11-03 at 23:15 +0000, Ted Gould wrote:

> > I see Cody agree's with me :). A function that returns void, returns
> > void - it *is* noise to add a return there, because you /cannot/ return
> > anything. I'll accept your point in a function that returns non-void
> > (and so will compilers :)).
>
> No, it's explicitly stating something that is otherwise implicit. For
> instance the return has value in a situation like this, even though no
> value is returned.
...
> You're just counting on the '}' at the end of the function putting an
> implied return there. I'm saying we should be explicit about the ending
> of a function because it's a very important thing in the function.

I can see you feel strongly about this; I'm sure it will turn up in
reviews again. So far you haven't convinced Cody or myself. Its a very
idiosyncratic coding style.

I have no objection to you landing this patch with it like that, but I
think this bears more discussion: having other folk cringe every time
they see a return at the end of a function isn't great :)

> > Would it be horrible to *signal* that this does not take a ref:
> > indicator_object_get_label_(steal|unref|noref|...) ?
>
> I'd agree, but I think with out guidance from GTK/Glib I'm not sure that
> it'd be that useful. Personally, I'd rather go with _ref on functions
> that do create refs.

Lets land this branch. Perhaps you could blog (you are on planet gnome,
yes?) about this and see if there is any major feelings from the
community?

Adding _ref works fine for me too - I don't care about the specific
representation of the signal, just the presence.

-Rob