Google Talk's Shared Status extension <http://code.google.com/apis/talk/jep_extensions/shared_status.html> allows the server to specify a maximum length for status messages. Gabble currently ignores this. This means that setting our status can fail. Unfortunately, Gabble still signals PresencesChanged for the new status. Here's what happens (I truncated my extraordinarily long status message to "aoeuaoeu..." for legibility): gabble/connection-DEBUG: 13/01/11 12:28:36.965871: set_shared_status: shared status invisibility is available wocky-DEBUG: 13/01/11 12:28:36.966175: _write_node_tree: Serializing tree: * iq xmlns='jabber:client' type='set' to='resiak@gmail.com' id='962323966003' * query xmlns='google:shared-status' version='2' * status "aoeuaoeu..." * show "default" * status-list show='dnd' * status "☕" * status "☕" * status "☕" * status "☕" * status "☕" * status-list show='default' * status "aoeuaoeu..." * invisible value='false' * iq xmlns='jabber:client' type='error' to='resiak@gmail.com/QueegF9AC7535' id='962323966003' from='resiak@gmail.com' * query xmlns='google:shared-status' version='2' * status "aoeuaoeu..." * show "default" * status-list show='dnd' * status "☕" * status "☕" * status "☕" * status "☕" * status "☕" * status-list show='default' * status "aoeuaoeu..." * invisible value='false' * error code='400' type='modify' * bad-request xmlns='urn:ietf:params:xml:ns:xmpp-stanzas' * text xmlns='urn:ietf:params:xml:ns:xmpp-stanzas' "Status text too long." gabble/connection-DEBUG: 13/01/11 12:28:37.148663: set_shared_status_presence_cb: gabble/connection-DEBUG: 13/01/11 12:28:37.148726: set_shared_status_presence_cb: Error setting shared status error setting Google shared status: WOCKY_XMPP_ERROR_BAD_REQUEST (#3): Status text too long. And yet we still emit PresencesChanged for the new status: static void set_shared_status_presence_cb (GObject *source_object, GAsyncResult *res, gpointer user_data) { GabbleConnection *self = GABBLE_CONNECTION (source_object); GError *error = NULL; DEBUG (" "); if (!set_shared_status_finish (self, res, &error)) { DEBUG ("Error setting shared status %s", error->message); g_error_free (error); error = NULL; } emit_presences_changed_for_self (self); } because the caller of set_shared_status_async() does this: if (gabble_presence_update (conn->self_presence, resource, i, message_str, prio)) { //... else if (priv->shared_statuses != NULL) { set_shared_status_async (conn, set_shared_status_presence_cb, NULL); //...
Possible fixes: • Gabble could truncate your status message (as long as PresencesChanged contains the message that actually got set). • SimplePresence could grow a MaximumMessageLength: u property, where '0' means no limit. I think we probably want both, actually. This means naïve clients work better without having to deal with setting your status failing, and better clients can do something amazing.
So here goes the status: - The following branches are ready for review: tp-spec - http://git.collabora.co.uk/gitweb/?p=user/andrunko/telepathy-spec.git;a=shortlog;h=refs/heads/presence-status-max tp-glib - http://git.collabora.co.uk/gitweb/?p=user/andrunko/telepathy-glib.git;a=shortlog;h=refs/heads/presence-status-max tp-gabble - http://git.collabora.co.uk/gitweb/?p=user/andrunko/telepathy-gabble.git;a=shortlog;h=refs/heads/presence-status-max tp-qt4 - http://git.collabora.co.uk/gitweb/?p=user/andrunko/telepathy-qt4.git;a=shortlog;h=refs/heads/presence-status-max Some notes about the impl: - The new property is called Conn.SimplePresence.MaximumStatusMessageLength. I chose this name as MaximumMessageLength seems vague to me. - The tp-glib impl adds a new callback to PresenceMixin. This callback ideally would be set in presence_mixin_class_init but we can't change its signature without breaking API, so I just added the callback and set it after calling class_init on gabble/test CM/etc. - The tp-glib version has a copy of the new interface Conn.SimplePresence as tp-glib requires it in order to access a property. Properties not generated from the interface cannot be used. - The tp-qt4 impl do not have a copy of the new interface as we don't require it, I just implemented the support internally. - Updated tp-glib contacts-conn to support the new property and copied an updated version of it to tp-qt4 (to properly test the support). - The docs for the new property in the spec probably needs some love. Missing (how to proceed?): - gabble and tp-qt4 (new tests) should update the tp-glib dependency to a proper tp-glib version once released. - tp-glib should probably update to spec-0.22.2 once available instead of having a copy of the new interface only (not sure how this works exactly in tp-glib) - tp-qt4 should update to spec-0.22.2 once available (not sure if this is a requirement but it would be good at least). I have a bitrotting branch to update to 0.22.0, which we should probably update and use it once the new spec is released.
The spec wording looks fine. It needs a <tp:added/> annotation. Also: (In reply to comment #2) > - The new property is called Conn.SimplePresence.MaximumStatusMessageLength. > I chose this name as MaximumMessageLength seems vague to me. On the contrary, I'd go for calling it MaximumLength or LengthLimit or generally something briefer. TP_PROP_CONNECTION_INTERFACE_SIMPLE_PRESENCE_MAXIMUM_STATUS_MESSAGE_LENGTH makes me sad.
Actually, I'd reword this: + <p>The connection manager MUST truncate the status message being set if + its length is bigger than the value of this property and + PresencesChanged MUST be emitted properly with the truncated + status message.</p> + to: <p>If a message passed to <tp:member-ref>SetPresence</tp:member-ref> is longer than allowed by this property, the connection manager MUST truncate the supplied message; when emitting <tp:member-ref>PresencesChanged</tp:member-ref>, the truncated version of the message MUST be used.</p>
(In reply to comment #2) > tp-glib - > http://git.collabora.co.uk/gitweb/?p=user/andrunko/telepathy-glib.git;a=shortlog;h=refs/heads/presence-status-max I'd made the corresponding shortening to the name of the vfunc. http://git.collabora.co.uk/gitweb/?p=user/andrunko/telepathy-glib.git;a=commitdiff;h=00764591b5141aa6161e232316105c61783277fe This claims that the get_maximum_status_message_length is a private field, but applications are meant to set it. You should add /*<public>*/ and /*<private>*/ annotations. You also need to add a definition of it to the TpPresenceMixinClass docstring.
The Gabble patches look fine, though the repeated calls to int() on a value that starts off as a string constant containing an integer … makes me think the field should be an int and it could be converted to a string when needed.
(In reply to comment #4) > Actually, I'd reword this: > > + <p>The connection manager MUST truncate the status message being set > if > + its length is bigger than the value of this property and > + PresencesChanged MUST be emitted properly with the truncated > + status message.</p> > + > > to: > > <p>If a message passed to <tp:member-ref>SetPresence</tp:member-ref> is longer > than allowed by this property, the connection manager MUST truncate the > supplied message; when emitting > <tp:member-ref>PresencesChanged</tp:member-ref>, the truncated version of the > message MUST be used.</p> Branch updated.
(In reply to comment #5) > (In reply to comment #2) > > tp-glib - > > http://git.collabora.co.uk/gitweb/?p=user/andrunko/telepathy-glib.git;a=shortlog;h=refs/heads/presence-status-max > > I'd made the corresponding shortening to the name of the vfunc. > > http://git.collabora.co.uk/gitweb/?p=user/andrunko/telepathy-glib.git;a=commitdiff;h=00764591b5141aa6161e232316105c61783277fe > > This claims that the get_maximum_status_message_length is a private field, > but applications are meant to set it. You should add /*<public>*/ and > /*<private>*/ annotations. You also need to add a definition of it to the > TpPresenceMixinClass docstring. Added the annotations and updated the docstring. As we discussed on IRC the name is still the same, so no change here.
(In reply to comment #6) > The Gabble patches look fine, though the repeated calls to int() on a value > that starts off as a string constant containing an integer … makes me think the > field should be an int and it could be converted to a string when needed. Here I used the same as max_statuses which uses string. It seems the reason is that self._send_status_list(req_iq['id']) requires strings instead of int, failing here when I change to int. Let me know if I should change it anyway.
The changes look fine. Please merge to spec 0.22 and to tp-glib 0.14; hold off on the gabble branch. I will be making releases of the spec and tp-glib later today; after that the Gabble branch can be merged with a dependency on the necessary version of tp-glib.
(In reply to comment #10) > The changes look fine. Please merge to spec 0.22 and to tp-glib 0.14; hold off > on the gabble branch. I will be making releases of the spec and tp-glib later > today; after that the Gabble branch can be merged with a dependency on the > necessary version of tp-glib. Thanks, merged spec and tp-glib.
Merged; will be part of telepathy-gabble 0.12.0 when `make maintainer-upload-release` finishes.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.