Merge lp://qastaging/~ted/libindicator/service-management into lp://qastaging/libindicator/0.4

Creating a set of objects to create a service and to manage it. For the most part this just takes care if starting up today. There isn't much "management" per se, it is just the boot strapping of this feature to get it working.

On Wed, 2009-11-04 at 02:35 +0000, Ted Gould wrote:
>
> === modified file '.bzrignore'
> --- .bzrignore 2009-10-12 03:06:46 +0000
> +++ .bzrignore 2009-11-04 02:35:19 +0000
> @@ -117,3 +117,17 @@
These two:
> +libindicator/indicator-service-client.h
> +libindicator/indicator-service-server.h

Look bogus. I realise they aren't, but it would be nice if we could make
them clearly trash in the tree - e.g. put them in a 'generated' or
'temp' or 'local' or something directory.

> +static void
> +indicator_service_manager_dispose (GObject *object)
> +{
> + IndicatorServiceManagerPrivate * priv =
> INDICATOR_SERVICE_MANAGER_GET_PRIVATE(object);
> +
> + /* If we were connected we need to make sure to
> + tell people that it's no longer the case. */
> + if (priv->connected) {

This:

> + priv->connected = FALSE;
> + g_signal_emit(object, signals[CONNECTION_CHANGE], 0,
> FALSE, TRUE);
> + }

Looks like something to factor out into a private helper for
disconnections that aren't disposals.

...

> + }
> +
> + return;
> +}

:(. :)

> +static void
> +watch_cb (DBusGProxy * proxy, gint version, GError * error, gpointer
> user_data)

perhaps s/version/service_version/ - it would be clearer

> + /* Buffer */
> + void (*indicator_service_manager_reserved1) (void);
> + void (*indicator_service_manager_reserved2) (void);
> + void (*indicator_service_manager_reserved3) (void);
> + void (*indicator_service_manager_reserved4) (void);
> +};

Perhaps here and below would be nicer as:
void (*reserved)[4] or
void (*indicator_service_manager_reserved)(void)[4]

> + } else {
> + g_warning("Name is a string bud.");

                                                      ^^^^ ???

+loader-tester: test-loader libdummy-indicator-null.la
libdummy-indicator-simple.la Makefile
> + @echo "#!/bin/sh" > loader-tester
> + @echo gtester -k --verbose -o=$(XML_REPORT) ./test-loader >>
> loader-tester
> + @chmod +x loader-tester

Whats this indirection for ? seems like a simple
check-local: test-loader libdummy-indicator-null.la \
  libdummy-indicator-simple.la
    gtester -k --verbose -o=$(XML_REPORT) ./test-loader >> loader-tester

would be a lot cleaner

Other than that, +1

 review: abstain

On Wed, 2009-11-04 at 04:12 +0000, Robert Collins wrote:
> On Wed, 2009-11-04 at 02:35 +0000, Ted Gould wrote:
> > === modified file '.bzrignore'
> > --- .bzrignore 2009-10-12 03:06:46 +0000
> > +++ .bzrignore 2009-11-04 02:35:19 +0000
> > @@ -117,3 +117,17 @@
> These two:
> > +libindicator/indicator-service-client.h
> > +libindicator/indicator-service-server.h
>
> Look bogus. I realise they aren't, but it would be nice if we could make
> them clearly trash in the tree - e.g. put them in a 'generated' or
> 'temp' or 'local' or something directory.

Honestly, I'm not sure how to make automake do that. Patches welcome :)
The problem is the names are generated off of the xml file name and I'm
not sure how to do paths with all that.

> > +static void
> > +indicator_service_manager_dispose (GObject *object)
> > +{
> > + IndicatorServiceManagerPrivate * priv =
> > INDICATOR_SERVICE_MANAGER_GET_PRIVATE(object);
> > +
> > + /* If we were connected we need to make sure to
> > + tell people that it's no longer the case. */
> > + if (priv->connected) {
>
> This:
>
> > + priv->connected = FALSE;
> > + g_signal_emit(object, signals[CONNECTION_CHANGE], 0,
> > FALSE, TRUE);
> > + }
>
> Looks like something to factor out into a private helper for
> disconnections that aren't disposals.

I think that'd be a premature optimization at this point. I'm not
entirely sure how all the DBus watcher stuff is going to work out when
it's complete, so it's probably not worth optimizing quite yet.

> > +static void
> > +watch_cb (DBusGProxy * proxy, gint version, GError * error, gpointer
> > user_data)
>
> perhaps s/version/service_version/ - it would be clearer

Fixed r370.

> > + /* Buffer */
> > + void (*indicator_service_manager_reserved1) (void);
> > + void (*indicator_service_manager_reserved2) (void);
> > + void (*indicator_service_manager_reserved3) (void);
> > + void (*indicator_service_manager_reserved4) (void);
> > +};
>
> Perhaps here and below would be nicer as:
> void (*reserved)[4] or
> void (*indicator_service_manager_reserved)(void)[4]

I'm not sure why it's done this way, but this matches the style that is
in GTK pretty much everywhere.

Cody, do know if there's a reason for this?

> > + } else {
> > + g_warning("Name is a string bud.");
>
> ^^^^ ???

Short for "buddy". Fixed r371 -- support i18n of English :)

> +loader-tester: test-loader libdummy-indicator-null.la
> libdummy-indicator-simple.la Makefile
> > + @echo "#!/bin/sh" > loader-tester
> > + @echo gtester -k --verbose -o=$(XML_REPORT) ./test-loader >>
> > loader-tester
> > + @chmod +x loader-tester
>
> Whats this indirection for ? seems like a simple
> check-local: test-loader libdummy-indicator-null.la \
> libdummy-indicator-simple.la
> gtester -k --verbose -o=$(XML_REPORT) ./test-loader >> loader-tester
>
> would be a lot cleaner

The problem is that if you use gtester with other tests then your
reporting kinda sucks. If you use it through TESTS then you get the
nice "1 of 3...

On Wed, 2009-11-04 at 04:48 +0000, Ted Gould wrote:
>
> The problem is that if you use gtester with other tests then your
> reporting kinda sucks. If you use it through TESTS then you get the
> nice "1 of 3 failed" or some other information from autotest. If you
> want to use TESTS then they have to be shell scripts.

Do you mean 'If we have some gtester and some non-gtester tests' ?

I think the indirection through a shell script is much less clear than
runnning the test directly, and the gain (autotester knows how many
groups of tests are being run) is minimal. If you have multiple
directories with tests, or complex environments, then autotester loses
this data anyway. If we want consolidated reporting, generating subunit
logs and reporting from them is probably a lot saner.

For now I think its better to keep things as clear and shallow as
possible: as long as the test fails causes a build failure, thats the
key thing.

-Rob

On Wed, 2009-11-04 at 05:24 +0000, Robert Collins wrote:
> On Wed, 2009-11-04 at 04:48 +0000, Ted Gould wrote:
> > The problem is that if you use gtester with other tests then your
> > reporting kinda sucks. If you use it through TESTS then you get the
> > nice "1 of 3 failed" or some other information from autotest. If you
> > want to use TESTS then they have to be shell scripts.
>
> Do you mean 'If we have some gtester and some non-gtester tests' ?

No, we have this situation for anything that uses dbus as we have to run
them under dbus-test-runner to setup unique busses for the testing. And
that's why it changed in this version as this is the first that does
dbus testing.

> I think the indirection through a shell script is much less clear than
> runnning the test directly, and the gain (autotester knows how many
> groups of tests are being run) is minimal. If you have multiple
> directories with tests, or complex environments, then autotester loses
> this data anyway. If we want consolidated reporting, generating subunit
> logs and reporting from them is probably a lot saner.

Sure, I'm not sure how to achieve that. I'm just looking for them
running and giving me readable errors :)