Bug 27167

(In reply to comment #0)
> The solution I wrote is to return from Subscribe call after the request for
> data terminates.

> Another solution I could have used is to return from the
> RequestInboxURL after the data has arrived.

I'd prefer it if this was what happened: I think it should be valid to call Subscribe and immediately follow up with RequestInboxURL.

(It's clearly wrong to call RequestMailURL before you have any mails, though :-)

> The benefit of the first solution
> over the second is that you never get the initial unread-count right, instead
> of getting 0 and then the real number.

I can see that that's an advantage of having Subscribe() wait, although I think it's orthogonal to how the other methods behave; all the methods "can be slow", so logically they all ought to be prepared to wait.

(In practice this probably means that instead of a queue of contexts, you need a queue of struct { enum, context } where the enum takes values SUBSCRIBE and REQUEST_INBOX.)

> The cons of this method is that
> according to the spec Subscribe cannot return a network error, which could
> happen if the server fails to respond.

I consider the error conditions in the spec to be advisory, rather than exhaustive: any D-Bus method call can fail at any time for any reason. If you think it's useful for Subscribe to wait for initial setup where necessary, I'd welcome a spec patch to say so.

(In reply to comment #1)
> I'd prefer it if this was what happened: I think it should be valid to call
> Subscribe and immediately follow up with RequestInboxURL.

Both solution makes it valid, that was the bug I was trying to fix ;)

> 
> (It's clearly wrong to call RequestMailURL before you have any mails, though
> :-)

Well no, it's right since you want have access to the web interface at anytime, not only when count >= 0.

> I consider the error conditions in the spec to be advisory, rather than
> exhaustive: any D-Bus method call can fail at any time for any reason. If you
> think it's useful for Subscribe to wait for initial setup where necessary, I'd
> welcome a spec patch to say so.

It's clear that the best solution is to allow all method to be aliased (as long as Subscribe is called first as stated by the spec). This would give more flexibility on the API client side.

The only thing I can't stand is having to add more non-public crap into connection.h. What I would propose is to create a precedent, where an extension would define a private pointer to it's private data using an opaque structure. This way, you don't have to change the connection every-time a new private member must be added. Please let me know if you agree or not, so I can start the rewrite of this patch.

(In reply to comment #2)
> (In reply to comment #1)
> > I'd prefer it if this was what happened: I think it should be valid to call
> > Subscribe and immediately follow up with RequestInboxURL.
> 
> Both solution makes it valid, that was the bug I was trying to fix ;)

Sorry, I wasn't clear enough there. What I mean is:

* call Subscribe
* call RequestInboxURL
* time passes
* Subscribe succeeds
* RequestInboxURL succeeds

(to get an inbox URL without having to wait for two D-Bus round-trips).

> > (It's clearly wrong to call RequestMailURL before you have any mails, though
> > :-)
> 
> Well no, it's right since you want have access to the web interface at anytime,
> not only when count >= 0.

It's OK to call RequestInboxURL before you have mail, but RequestMailURL not so much :-)

> The only thing I can't stand is having to add more non-public crap into
> connection.h. What I would propose is to create a precedent, where an extension
> would define a private pointer to it's private data using an opaque structure.
> This way, you don't have to change the connection every-time a new private
> member must be added.

Given that we're not writing a library, I think the current approach is fine; there's considerably less pointer-chasing this way.

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > I'd prefer it if this was what happened: I think it should be valid to call
> > > Subscribe and immediately follow up with RequestInboxURL.
> > 
> > Both solution makes it valid, that was the bug I was trying to fix ;)
> 
> Sorry, I wasn't clear enough there. What I mean is:
> 
> * call Subscribe
> * call RequestInboxURL
> * time passes
> * Subscribe succeeds
> * RequestInboxURL succeeds
> 
> (to get an inbox URL without having to wait for two D-Bus round-trips).

That is the right solution. Removing patch keyword until a proper patch is written.

Finally I managed to write a proper fix to that bug. It can be found at:

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=shortlog;h=refs/heads/fix-27167

There is three patches in this branch.

1. 9318b1dc26232da87b0a4ad6d01a8f5fc77a9b66

I simply move all the MailNotification stuff into private structure so I can stop "pouting" the GabbleConnection public structure. Mostly trivial change.

2. b58630d87f7dc7de9ac12fead4356c798876db86

I actually fix the issue. This is non-trivial change that consist of delaying RequestInboxURL() calls until the initial mail data has been received.

2. 3056d4099e7728bfae0c0f3405d5f1fe0cd674cc

This one is just a small bug I found. There was a dirty check based on the fact something has changed in the UnreadMails array. That should have caused issue when max is reached (30), but by luck the check was bugged and would always return true (dirty). I simply removed that check since it gives the same result as before and that everything has been proven to work well this way.

> +  GList *pending_requests;

This needs a comment about what its contents are (/* list of DBusGMethodInvocation */ is sufficient), and should be called inbox_url_requests or something (since it doesn't contain enough context to be used for anything else).

> +  if (priv->inbox_url != NULL && priv->inbox_url[0] == 0)
> +      {
> +        error = g_error_new (TP_ERRORS, TP_ERROR_NETWORK_ERROR,
> +            "Failed to obtain base URL.");
> +      }
> +  else if (priv->inbox_url == NULL)
> +      {
> +        error = g_error_new (TP_ERRORS, TP_ERROR_NETWORK_ERROR,
> +            "Failed to obtain base URL.");
> +      }

What's the point of doing this? Just have one check, using the fact that || short-circuits:

  if (priv->inbox_url == NULL || priv->inbox_url[0] == '\0')

or if your intention was to have different error messages/codes in the two cases, do that :-)

(I also prefer the zero'th char to be spelled '\0' rather than 0, as seen in my suggested version, even though strictly speaking they're equivalent.)

> +  if (priv->inbox_url)
> +    return_from_request_inbox_url (conn);

if (priv->inbox_url != NULL)

> +          "Failed to retreive URL from server."};

retrieve

> +  /* Use empty string to differientiate pending request from server failing to

differentiate

> > +  if (priv->inbox_url != NULL && priv->inbox_url[0] == 0)
> > +      {
> > +        error = g_error_new (TP_ERRORS, TP_ERROR_NETWORK_ERROR,
> > +            "Failed to obtain base URL.");
> > +      }
> > +  else if (priv->inbox_url == NULL)
> > +      {
> > +        error = g_error_new (TP_ERRORS, TP_ERROR_NETWORK_ERROR,
> > +            "Failed to obtain base URL.");
> > +      }
> 
> What's the point of doing this? Just have one check, using the fact that ||
> short-circuits:

Oops, this was supposed to be different errors. The goal was to have an error when server fails to give the mandatory information (hopefully never) and the other for call being cancelled (connection dropped, Unsubscribe was called, object has been destroyed, etc.)

Hopefully I'll fix that soon, stay tunned ;)

Done.

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=ee9b0806a4110f2c814fca04463cd80ce63bd9eb;hp=0dece99bb5d12407c117c43017785cd1d8c835a0

Tell me if you need anything else to be fixed.

Thanks, ship it.

Shipped