Bug 69705

Created attachment 86723 [details] [review]
include telepathy-glib-dbus.h

Created attachment 86724 [details] [review]
stop using NUM_TP_*

Created attachment 86725 [details] [review]
stop using tp_account_is_prepared()

Created attachment 86726 [details] [review]
move tp_tests_copy_dir() to logger-test-helper

Those patches should be merged to 'master' before branching for 'next'.

Comment on attachment 86723 [details] [review]
include telepathy-glib-dbus.h

Review of attachment 86723 [details] [review]:
-----------------------------------------------------------------

Sure, as long as it depends on at least 0.20

Comment on attachment 86724 [details] [review]
stop using NUM_TP_*

Review of attachment 86724 [details] [review]:
-----------------------------------------------------------------

ya rly

(In reply to comment #6)
> Comment on attachment 86723 [details] [review] [review]
> include telepathy-glib-dbus.h
> 
> Review of attachment 86723 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Sure, as long as it depends on at least 0.20

We do, merging.

Comment on attachment 86725 [details] [review]
stop using tp_account_is_prepared()

Review of attachment 86725 [details] [review]:
-----------------------------------------------------------------

yes

Comment on attachment 86726 [details] [review]
move tp_tests_copy_dir() to logger-test-helper

Review of attachment 86726 [details] [review]:
-----------------------------------------------------------------

::: tests/lib/logger-test-helper.c
@@ +69,5 @@
> +tp_tests_copy_dir (const gchar *from_dir, const gchar *to_dir)
> +{
> +  gchar *command;
> +
> +  // If destination directory exist erase it

We use /* */ comments, but this isn't a regression so never mind.

@@ +70,5 @@
> +{
> +  gchar *command;
> +
> +  // If destination directory exist erase it
> +  command = g_strdup_printf ("rm -rf %s", to_dir);

Ugh, horrible. But it's not a regression so yeah I suppose so. Hopefully we don't call this on anything with shell metacharacters in?

g_spawn_sync() would be better. Something like this maybe:

gchar *argv[] = { "rm", "-rf", to_dir, NULL };
gchar *stdout;
gchar *stderr;
gint status;

g_spawn_sync (NULL, argv, NULL, 0, NULL, NULL, &stdout, &stderr, &status, &error);
g_assert_no_error (error);

if (stdout != NULL)
  g_assert_cmpstr (stdout, ==, "");

if (stderr != NULL)
  g_assert_cmpstr (stderr, ==, "");

g_spawn_check_exit_status (status, &error);
g_assert_no_error (error);
g_free (stdout);
g_free (stderr);

(You could have a helper function that takes argv.)

(In reply to comment #11)
> g_spawn_check_exit_status (status, &error);

requires GLib 2.34, but I think that'd be OK for master now

Here is what I have so far (not finished or ready for review)
http://cgit.collabora.com/git/user/cassidy/telepathy-logger/log/?h=next

TODO:
- fix "make check"
- remove all instances of "org.freedesktop.Telepathy"
- Suffix library binary file with -1
- Same for the .pc files
- rename gir to TelepathyLogger-1.gir
- Install headers to telepathy-logger-1/telepathy-logger

If the introspected API is sufficiently different that API users will have to alter their code anyway, then TelepathyLogger1-1.gir might be better. I'm probably going to do a similar rename on telepathy-glib.

Don't forget to change the installation location for the docs (we forgot to do that for telepathy-glib).

Review of your branch so far:

1) telepathy-logger/telepathy-logger-0.2-uninstalled.pc.in:
-Requires: telepathy-glib libxml-2.0
+Requires: telepathy-glib telepathy-glib-1-dbus libxml-2.0

You forgot to change to telepathy-glib-1

2) telepathy-logger/text-channel.c:
 - depends_on[1] = TP_CHANNEL_FEATURE_CONTACTS;

Actually no, CORE is not enough because GROUP is not part of CORE anymore. To get all channel's TpContact prepared you now have to prepare GROUP feature.

http://cgit.collabora.com/git/user/cassidy/telepathy-logger/log/?h=next updated and should be ready now.

Xavier: I incorporated your branch into mine.

Simon: done; assuming that's the proper way to do it.

For now, ship it. However, after that:

> -#define GSETTINGS_SCHEMA "org.freedesktop.Telepathy.Logger"
> +#define GSETTINGS_SCHEMA "im.telepathy1.Logger"

We'll need a migration path for this later, but for now I think it's OK to have it be completely independent.

> + <interface name="im.telepathy1.Logger.DRAFT"

Please rename to im.telepathy1.Logger1 (or just Logger if we think it has the same level of stability as Account, but I doubt that) and copy it into telepathy-spec next.

> + namespace="...">FavouriteContactsIsReady</tp:dbus-ref> property.

Can we rename this to FavouriteContactsReady or FavouriteContactsAreReady so I don't get annoyed every time I read it? :-)

(In reply to comment #17)
> For now, ship it. However, after that:

I just noticed that there is already an old 'next' branch upstream :\
http://cgit.freedesktop.org/telepathy/telepathy-logger/log/?h=next

I'd be tempted to just drop it and replace it with mine tbh.

> > -#define GSETTINGS_SCHEMA "org.freedesktop.Telepathy.Logger"
> > +#define GSETTINGS_SCHEMA "im.telepathy1.Logger"
> 
> We'll need a migration path for this later, but for now I think it's OK to
> have it be completely independent.

Actually do we really want to want that? The GSettings's schema path is completely unrelated to the TP D-Bus namespace. Can't we just keep the current schema as it?

> > + <interface name="im.telepathy1.Logger.DRAFT"
> 
> Please rename to im.telepathy1.Logger1 (or just Logger if we think it has
> the same level of stability as Account, but I doubt that) and copy it into
> telepathy-spec next.
> 
> > + namespace="...">FavouriteContactsIsReady</tp:dbus-ref> property.
> 
> Can we rename this to FavouriteContactsReady or FavouriteContactsAreReady so
> I don't get annoyed every time I read it? :-)

Will do.

(In reply to comment #17)
+ <interface name="im.telepathy1.Logger.DRAFT"
> 
> Please rename to im.telepathy1.Logger1 (or just Logger if we think it has
> the same level of stability as Account, but I doubt that) and copy it into
> telepathy-spec next.

done (same branch)

> > + namespace="...">FavouriteContactsIsReady</tp:dbus-ref> property.
> 
> Can we rename this to FavouriteContactsReady or FavouriteContactsAreReady so
> I don't get annoyed every time I read it? :-)

Actually it's has been removed so I just updated the doc.

(In reply to comment #18)
> (In reply to comment #17)
> > > -#define GSETTINGS_SCHEMA "org.freedesktop.Telepathy.Logger"
> > > +#define GSETTINGS_SCHEMA "im.telepathy1.Logger"
> > 
> > We'll need a migration path for this later, but for now I think it's OK to
> > have it be completely independent.
> 
> Actually do we really want to want that? The GSettings's schema path is
> completely unrelated to the TP D-Bus namespace. Can't we just keep the
> current schema as it?

If it's co-installable (i.e. you can install the old and new Logger on the same machine), fine.

If it isn't co-installable, it's still OK for now, but it's an issue we'll need to solve before 1.0.

I reverted the schema change and pushed to origin.