Bug 29981

Did you intend to change the Wocky submodule in the second commit?

Also, these commits could really do with being arranged more readably. I don't see that preserving the history of the first version of the interface is all that useful. Similarly, the ‘Return DBUS invocation context.’ commit fixes a bug introduced in the previous commit; it should be squashed. I think I'd basically expect this branch to be:

• Add the latest draft XML;
• Replace the code using GabbleSlacker with an implementation of the new interface, replacing the test;
• Remove GabbleSlacker.

git add -p, git commit --amend, git rebase are all your friends!

-    gboolean is_inactive)
+    gboolean start_queueing)

Much better argument name!

It's better not to use _gabble_connection_send_with_reply() in new code. Please call wocky_porter_send_iq_async() on the porter you get back from gabble_connection_get_porter(). Maybe we should add a gabble_connection_send_iq_async() wrapper around it, but we should avoid adding new code that uses the Lm compatibility stuff. (See below for a wrapper I've already written around _finish().)

Oh, there should also be one more patch that replaces lm_message_build_with_sub_type() with wocky_xmpp_stanza_build(). And, since conn-slacker.[ch] have been renamed, their introductory comments should be updated. (I notice that I was a bad person and claim that the .c is a .h in its comment...)

-  /* We can only cork presence updates on Google Talk. Of course, the Google
-   * Talk server doesn't advertise support for google:queue. So let's use the
-   * roster again... */
-  if (!(conn->features & GABBLE_CONNECTION_FEATURES_GOOGLE_ROSTER))
-    return;

This check appears to have been lost, so we're sending commands to the server without checking if it supports them. What we should do instead is not include Connection.Interface.PowerSave in Connection.Interfaces if we're not on a Google connection (so that MC knows not to bother telling Gabble the activity status has changed), and probably also return an error from the method on PowerSave() if someone decides to call it anyway.

+      g_error_free (error);
+
+      error = g_error_new (TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
+          "Failed to %sable power saving", enable ? "en" : "dis");
+
+      dbus_g_method_return_error (context, error);

It'd be nice to include information on why not in the error message, based on the error returned from sending the IQ. In a work-in-progress branch <http://git.collabora.co.uk/?p=user/wjt/telepathy-gabble-wjt.git;a=shortlog;h=refs/heads/search-cleanup>, I've got a wrapper around wocky_porter_send_iq_finish() that makes this easier to do. <http://git.collabora.co.uk/?p=user/wjt/telepathy-gabble-wjt.git;a=commitdiff;h=23455ed61a3994aebaf39032cbc402a2521fe189>


+  gboolean enable =lm_message_node_find_child (

Coding style: space after =.

The configure script should be updated not to check for MCE any more.

The test should check what happens if Gabble's still waiting for a reply to a <queue/> request when it's disconnected. I think the contact search tests have an example of how to do this: you pass some EventPatterns to a helper function that deals with the disconnection; so here we should presumably expect the Cancelled error or something.

The test could also check that no-op calls to TogglePowerSaving return without any network traffic.

This was a wip. Didn't mean for it to get reviewed, but thanks anyway! I'll integrate your feedback.

Ok, now it is ready for review!

(In reply to comment #1)
> It's better not to use _gabble_connection_send_with_reply() in new code. Please
> call wocky_porter_send_iq_async() on the porter you get back from
> gabble_connection_get_porter(). Maybe we should add a
> gabble_connection_send_iq_async() wrapper around it, but we should avoid adding
> new code that uses the Lm compatibility stuff. (See below for a wrapper I've
> already written around _finish().)
> 

This branch depends on another one in bug 30094. I made slick wocky_porter_send_iq_async wrappers to make life easier. I also stole some of your code.

> -  /* We can only cork presence updates on Google Talk. Of course, the Google
> -   * Talk server doesn't advertise support for google:queue. So let's use the
> -   * roster again... */
> -  if (!(conn->features & GABBLE_CONNECTION_FEATURES_GOOGLE_ROSTER))
> -    return;
> 
> This check appears to have been lost, so we're sending commands to the server
> without checking if it supports them. What we should do instead is not include
> Connection.Interface.PowerSave in Connection.Interfaces if we're not on a
> Google connection (so that MC knows not to bother telling Gabble the activity
> status has changed), and probably also return an error from the method on
> PowerSave() if someone decides to call it anyway.
> 

I am not sure why that comment is there. "google:queue" does not get advertised in the connection disco. So the only way to know is trial and error (ie. SetPowerSaving and catch NotAvailable exception).

This is probably not such a bad thing. I need to amend the spec documentation, but I made it possible to set power save mode when the connection is offline. The method will always succeed, once we go online, if we encounter an error PowerSavingChanged(false) is emitted.

On second thought, maybe that is not very useful, since MC will know when a connection goes online, and could explicitly set the mode each time.

> The test should check what happens if Gabble's still waiting for a reply to a
> <queue/> request when it's disconnected. I think the contact search tests have
> an example of how to do this: you pass some EventPatterns to a helper function
> that deals with the disconnection; so here we should presumably expect the
> Cancelled error or something.
> 

This would not really be regarded as an error, so if the connection drops before we get an ack, we will reënable presence queuing the next time it connects.

> The test could also check that no-op calls to TogglePowerSaving return without
> any network traffic.

Added that.

(In reply to comment #3)
> > -  /* We can only cork presence updates on Google Talk. Of course, the Google
> > -   * Talk server doesn't advertise support for google:queue. So let's use the
> > -   * roster again... */
> > -  if (!(conn->features & GABBLE_CONNECTION_FEATURES_GOOGLE_ROSTER))
> > -    return;

> I am not sure why that comment is there. "google:queue" does not get advertised
> in the connection disco.

Read the comment more carefully: it confirms that, and explains (very briefly) what the code is doing instead. We're basically assuming that support for google:roster implies support for google:queue - as the comment alludes, this is a common pattern in Gabble.

(In reply to comment #3)
> Ok, now it is ready for review!

I'll review it later; here are some drive-by comments having not read the branch.

> (In reply to comment #1)
> > -  /* We can only cork presence updates on Google Talk. Of course, the Google
> > -   * Talk server doesn't advertise support for google:queue. So let's use the
> > -   * roster again... */
> > -  if (!(conn->features & GABBLE_CONNECTION_FEATURES_GOOGLE_ROSTER))
> > -    return;
> > 
> > This check appears to have been lost, so we're sending commands to the server
> > without checking if it supports them. What we should do instead is not include
> > Connection.Interface.PowerSave in Connection.Interfaces if we're not on a
> > Google connection (so that MC knows not to bother telling Gabble the activity
> > status has changed), and probably also return an error from the method on
> > PowerSave() if someone decides to call it anyway.
> > 
> 
> I am not sure why that comment is there. "google:queue" does not get advertised
> in the connection disco. So the only way to know is trial and error (ie.
> SetPowerSaving and catch NotAvailable exception).

I know google:queue is not advertised. That's why the quoted-as-deleted code above says exactly that, and explains that it's using the presence google:roster to indicate that we're on a Google server. This isn't strictly right, but it does work.

I've pinged a contact at Google to see about them actually advertising google:queue.

> > The test should check what happens if Gabble's still waiting for a reply to a
> > <queue/> request when it's disconnected. I think the contact search tests have
> > an example of how to do this: you pass some EventPatterns to a helper function
> > that deals with the disconnection; so here we should presumably expect the
> > Cancelled error or something.
> > 
> 
> This would not really be regarded as an error, so if the connection drops
> before we get an ack, we will reënable presence queuing the next time it
> connects.

Yes, but it might crash. Stuff crashing if it's waiting for an IQ reply when we disconnect is incredibly common.

Aside from my/Simon's comments above, just trivia about the updated code.

In toggle_queueing_cb():

+  DEBUG (" ");
+

Surely better just to log something in both branches of:

+  if (error)

(which should be "if (error != NULL)", or actually "if (reply == NULL)". Ditto later in the file: if you check the reply's NULL-ness, you don't need the extra conditional as to whether to unref it).

Hmm also, if we've got a stanza in flight to enable queueing, we probably don't want to send a second one redundantly... but then this gets complicated. What happens if someone calls SetPowerSaving(True), SPS(False), SPS(True), all before the server responds to the first one. Probably not worth it.

+    gboolean in_Active,

This is a misleading name for the argument. The code-genned examples shouldn't include the in_, really. But in this case it not only looks ugly, but means the opposite of what was intended!

I think the GabbleConnection should have a "power-saving" property... and then we could implement the D-Bus property using tp_dbus_properties_mixin_getter_gobject_properties(). Much less faffing around with quarks.

(In reply to comment #5)
> > > -  /* We can only cork presence updates on Google Talk. Of course, the Google
> > > -   * Talk server doesn't advertise support for google:queue. So let's use the
> > > -   * roster again... */
> > > -  if (!(conn->features & GABBLE_CONNECTION_FEATURES_GOOGLE_ROSTER))
> > > -    return;
> > > 
> > > This check appears to have been lost, so we're sending commands to the server
> > > without checking if it supports them. What we should do instead is not include
> > > Connection.Interface.PowerSave in Connection.Interfaces if we're not on a
> > > Google connection (so that MC knows not to bother telling Gabble the activity
> > > status has changed), and probably also return an error from the method on
> > > PowerSave() if someone decides to call it anyway.
> > > 
> > 
> > I am not sure why that comment is there. "google:queue" does not get advertised
> > in the connection disco. So the only way to know is trial and error (ie.
> > SetPowerSaving and catch NotAvailable exception).
> 
> I know google:queue is not advertised. That's why the quoted-as-deleted code
> above says exactly that, and explains that it's using the presence
> google:roster to indicate that we're on a Google server. This isn't strictly
> right, but it does work.

I guess I should read more than 3 words in before commenting :P

I see Conn.I.PowerSaving being useful for offline connections as well, but I am not totally convinced about it. You could sway me either way. If we use this iface on offline accounts, we don't need to piggyback on top of google:roster, we will just always sport the iface, and raise NotAvailable if the service does not support it. Added bonus - non-google servers could support google:queue and we would interop with them, even if they don't support google:roster.

> 
> > > The test should check what happens if Gabble's still waiting for a reply to a
> > > <queue/> request when it's disconnected. I think the contact search tests have
> > > an example of how to do this: you pass some EventPatterns to a helper function
> > > that deals with the disconnection; so here we should presumably expect the
> > > Cancelled error or something.
> > > 
> > 
> > This would not really be regarded as an error, so if the connection drops
> > before we get an ack, we will reënable presence queuing the next time it
> > connects.
> 
> Yes, but it might crash. Stuff crashing if it's waiting for an IQ reply when we
> disconnect is incredibly common.

I added a test locally, it doesn't crash Gabble, but I don't know how to programmatically test for this. The search tests asserts that CreateChannel raises a Disconnected error. I guess if Gabble crashed we would catch the non-zero exit?

(In reply to comment #7)
> > Yes, but it might crash. Stuff crashing if it's waiting for an IQ reply when we
> > disconnect is incredibly common.
> 
> I added a test locally, it doesn't crash Gabble, but I don't know how to
> programmatically test for this. The search tests asserts that CreateChannel
> raises a Disconnected error. I guess if Gabble crashed we would catch the
> non-zero exit?

Never mind, that was a stupid question, disconnect_conn() makes sure the connection goes away nicely. I added a test, 51e9d6b

(In reply to comment #6)
> Aside from my/Simon's comments above, just trivia about the updated code.
> 
> In toggle_queueing_cb():
> 
> +  DEBUG (" ");
> +
> 
> Surely better just to log something in both branches of:
> 
> +  if (error)
> 
> (which should be "if (error != NULL)", or actually "if (reply == NULL)". Ditto
> later in the file: if you check the reply's NULL-ness, you don't need the extra
> conditional as to whether to unref it).
> 

cc39e14

> Hmm also, if we've got a stanza in flight to enable queueing, we probably don't
> want to send a second one redundantly... but then this gets complicated. What
> happens if someone calls SetPowerSaving(True), SPS(False), SPS(True), all
> before the server responds to the first one. Probably not worth it.

Yeah, although no real harm would come. It would just be stupid.

> 
> +    gboolean in_Active,
> 
> This is a misleading name for the argument. The code-genned examples shouldn't
> include the in_, really. But in this case it not only looks ugly, but means the
> opposite of what was intended!

f18e130

> 
> I think the GabbleConnection should have a "power-saving" property... and then
> we could implement the D-Bus property using
> tp_dbus_properties_mixin_getter_gobject_properties(). Much less faffing around
> with quarks.

3a863ce

(In reply to comment #7)
> I see Conn.I.PowerSaving being useful for offline connections as well, but I am
> not totally convinced about it. You could sway me either way. If we use this
> iface on offline accounts, we don't need to piggyback on top of google:roster,
> we will just always sport the iface, and raise NotAvailable if the service does
> not support it. Added bonus - non-google servers could support google:queue and
> we would interop with them, even if they don't support google:roster.

I would make it raise NotAvailable() until Connected, using TP_BASE_CONNECTION_ERROR_IF_NOT_CONNECTED(). When we get the server features, check for 'google:queue' or 'google:roster'; if either is present, add the interface using tp_base_connection_add_interfaces().

By checking for both features we would do the right thing with the deployed server that implements this feature, and allow for hypothetical servers to do the right thing.

This would mean MC should check for this interface only once the connection is connected. That seems fine: we can't exactly save power during the connection process. :)

Otherwise looks good.

(In reply to comment #10)
> (In reply to comment #7)
> > I see Conn.I.PowerSaving being useful for offline connections as well, but I am
> > not totally convinced about it. You could sway me either way. If we use this
> > iface on offline accounts, we don't need to piggyback on top of google:roster,
> > we will just always sport the iface, and raise NotAvailable if the service does
> > not support it. Added bonus - non-google servers could support google:queue and
> > we would interop with them, even if they don't support google:roster.
> 
> I would make it raise NotAvailable() until Connected, using
> TP_BASE_CONNECTION_ERROR_IF_NOT_CONNECTED(). When we get the server features,
> check for 'google:queue' or 'google:roster'; if either is present, add the
> interface using tp_base_connection_add_interfaces().
> 

Great! Made the appropriate changes. 0545f7c

> By checking for both features we would do the right thing with the deployed
> server that implements this feature, and allow for hypothetical servers to do
> the right thing.

This introduces false positives when a server genuinely supports google:roster, but not google:queue. This is all hypothetical, so never mind :)

> 
> This would mean MC should check for this interface only once the connection is
> connected. That seems fine: we can't exactly save power during the connection
> process. :)

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > I see Conn.I.PowerSaving being useful for offline connections as well, but I am
> > > not totally convinced about it. You could sway me either way. If we use this
> > > iface on offline accounts, we don't need to piggyback on top of google:roster,
> > > we will just always sport the iface, and raise NotAvailable if the service does
> > > not support it. Added bonus - non-google servers could support google:queue and
> > > we would interop with them, even if they don't support google:roster.
> > 
> > I would make it raise NotAvailable() until Connected, using
> > TP_BASE_CONNECTION_ERROR_IF_NOT_CONNECTED(). When we get the server features,
> > check for 'google:queue' or 'google:roster'; if either is present, add the
> > interface using tp_base_connection_add_interfaces().
> > 
> 
> Great! Made the appropriate changes. 0545f7c

+  if (base->status != TP_CONNECTION_STATUS_CONNECTED)
+    {
+      GError *error = g_error_new_literal (TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
+          "Cannot enter power saving mode when not connected.");
+      dbus_g_method_return_error (context, error);
+      g_error_free (error);
       return;
     }

This is TP_BASE_CONNECTION_ERROR_IF_NOT_CONNECTED(), expanded out. And you don't need to heap-allocate a GError with a literal string, you can just say GError e = { TP_ERRORS, TP_ERROR_NOT_AVAILABLE, "..." }; if you want to keep the more-specific error message.

+                conn->features |= GABBLE_CONNECTION_FEATURES_GOOGLE_MAIL_NOTIFY;              else if (0 == strcmp (var, NS_GOOGLE_QUEUE))
+                conn->features |= GABBLE_CONNECTION_FEATURES_GOOGLE_QUEUE;

I think you're missing a newline before the 'else if' here.

> > By checking for both features we would do the right thing with the deployed
> > server that implements this feature, and allow for hypothetical servers to do
> > the right thing.
> 
> This introduces false positives when a server genuinely supports google:roster,
> but not google:queue. This is all hypothetical, so never mind :)

Yeah, the worst that happens is spurious updates. If this turns out to be a problem... Gabble can remember failures and stop sending them.

I don't suppose that, while you're touching this code, you could check if there's any reference to http://mail.jabber.org/pipermail/summit/2010-February/000528.html and if not add it?

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > (In reply to comment #7)
> > > > I see Conn.I.PowerSaving being useful for offline connections as well, but I am
> > > > not totally convinced about it. You could sway me either way. If we use this
> > > > iface on offline accounts, we don't need to piggyback on top of google:roster,
> > > > we will just always sport the iface, and raise NotAvailable if the service does
> > > > not support it. Added bonus - non-google servers could support google:queue and
> > > > we would interop with them, even if they don't support google:roster.
> > > 
> > > I would make it raise NotAvailable() until Connected, using
> > > TP_BASE_CONNECTION_ERROR_IF_NOT_CONNECTED(). When we get the server features,
> > > check for 'google:queue' or 'google:roster'; if either is present, add the
> > > interface using tp_base_connection_add_interfaces().
> > > 
> > 
> > Great! Made the appropriate changes. 0545f7c
> 
> +  if (base->status != TP_CONNECTION_STATUS_CONNECTED)
> +    {
> +      GError *error = g_error_new_literal (TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
> +          "Cannot enter power saving mode when not connected.");
> +      dbus_g_method_return_error (context, error);
> +      g_error_free (error);
>        return;
>      }
> 
> This is TP_BASE_CONNECTION_ERROR_IF_NOT_CONNECTED(), expanded out. And you
> don't need to heap-allocate a GError with a literal string, you can just say
> GError e = { TP_ERRORS, TP_ERROR_NOT_AVAILABLE, "..." }; if you want to keep
> the more-specific error message.

Yeah, I should make that a habit. 846a938

> 
> +                conn->features |=
> GABBLE_CONNECTION_FEATURES_GOOGLE_MAIL_NOTIFY;              else if (0 ==
> strcmp (var, NS_GOOGLE_QUEUE))
> +                conn->features |= GABBLE_CONNECTION_FEATURES_GOOGLE_QUEUE;
> 
> I think you're missing a newline before the 'else if' here.

846a938

> 
> > > By checking for both features we would do the right thing with the deployed
> > > server that implements this feature, and allow for hypothetical servers to do
> > > the right thing.
> > 
> > This introduces false positives when a server genuinely supports google:roster,
> > but not google:queue. This is all hypothetical, so never mind :)
> 
> Yeah, the worst that happens is spurious updates. If this turns out to be a
> problem... Gabble can remember failures and stop sending them.
> 
> I don't suppose that, while you're touching this code, you could check if
> there's any reference to
> http://mail.jabber.org/pipermail/summit/2010-February/000528.html and if not
> add it?

846a938

Sorry for the review-delay. These new patches look good to me.

(for onlookers: this branch can't be merged until MC supports poking the new PowerSaving interface.)

(In reply to comment #16)
> (for onlookers: this branch can't be merged until MC supports poking the new
> PowerSaving interface.)

Support is in mc. Could we merge this?

Yes, go for it. If you're quick, 0.11.0 will have it :-)

(In reply to comment #18)
> Yes, go for it. If you're quick, 0.11.0 will have it :-)

Merged. Yikes! Did I make it?

Ugh, sorry, I've done the release without it now. I'll do a 0.11.1.

REOPENED for undrafted implementation. We shouldn't ship draft API in a stable branch.

Here, have a branch.

Looks great, thanks.

Fixed in git for 0.11.1