From d37ec9bd4551e9cfcdb2fb033babff46e0c1f82e Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 11 Sep 2013 12:39:02 +0100 Subject: [PATCH 2/3] Presence: use a sequence number, not a timestamp, to test "most recent" This means the client-types test deterministically gets a different result: it was previously relying on the PC and the phone coming online near-simultaneously, which resulted in the PC "winning" the comparison (except when the test ran close to an integer number of seconds-since-the-epoch, when it failed). Now, the phone has a more recent "most recently available" sequence number, so it "wins". This might be considered to make regress, depending which tie-breaker you consider to be more important. The change to prefer_higher_priority_resources() illustrates one effect of this change: the priority never actually matters unless neither of the resources under consideration has ever been "available", because the "most recently available" sequence number is considered more important. It's not completely clear whether the "most recently available" sequence number is intended to tie-break between resources that are away/busy (display the one that went away more recently), or that are available (display the one that arrived more recently) - at the moment it does both. --- src/conn-presence.c | 4 +-- src/connection.c | 7 +++-- src/presence-cache.c | 3 +-- src/presence.c | 19 +++++++++----- src/presence.h | 3 +-- tests/test-presence.c | 60 +++++++++++++++---------------------------- tests/twisted/client-types.py | 9 +++---- 7 files changed, 42 insertions(+), 63 deletions(-) diff --git a/src/conn-presence.c b/src/conn-presence.c index e06062f..394172e 100644 --- a/src/conn-presence.c +++ b/src/conn-presence.c @@ -889,7 +889,7 @@ store_shared_statuses (GabbleConnection *self, { /* Update with the remote presence */ rv = gabble_presence_update (self->self_presence, resource, presence_id, - status_message, prio, NULL, time (NULL)); + status_message, prio, NULL); } g_free (resource); @@ -1799,7 +1799,7 @@ set_own_status_cb (GObject *obj, } if (gabble_presence_update (conn->self_presence, resource, i, - message_str, prio, NULL, time (NULL))) + message_str, prio, NULL)) { if (tp_base_connection_get_status (base) != TP_CONNECTION_STATUS_CONNECTED) diff --git a/src/connection.c b/src/connection.c index 5556b1e..e703e28 100644 --- a/src/connection.c +++ b/src/connection.c @@ -544,7 +544,7 @@ gabble_connection_constructed (GObject *object) self->self_presence = gabble_presence_new (); g_assert (priv->resource); gabble_presence_update (self->self_presence, priv->resource, - GABBLE_PRESENCE_AVAILABLE, NULL, priv->priority, NULL, time (NULL)); + GABBLE_PRESENCE_AVAILABLE, NULL, priv->priority, NULL); } static void @@ -740,17 +740,16 @@ gabble_connection_set_property (GObject *object, { gchar *old_resource = priv->resource; gchar *new_resource = g_value_dup_string (value); - time_t now = time (NULL); priv->resource = new_resource; /* Add self presence for new resource... */ gabble_presence_update (self->self_presence, new_resource, self->self_presence->status, self->self_presence->status_message, - priv->priority, NULL, now); + priv->priority, NULL); /* ...and remove it for the old one. */ gabble_presence_update (self->self_presence, old_resource, - GABBLE_PRESENCE_OFFLINE, NULL, 0, NULL, now); + GABBLE_PRESENCE_OFFLINE, NULL, 0, NULL); g_free (old_resource); } diff --git a/src/presence-cache.c b/src/presence-cache.c index f7d02ee..b3a0f05 100644 --- a/src/presence-cache.c +++ b/src/presence-cache.c @@ -2136,8 +2136,7 @@ gabble_presence_cache_do_update ( old_cap_set = gabble_presence_dup_caps (presence); ret = gabble_presence_update (presence, resource, presence_id, - status_message, priority, update_client_types, - time (NULL)); + status_message, priority, update_client_types); new_cap_set = gabble_presence_peek_caps (presence); diff --git a/src/presence.c b/src/presence.c index db9830c..f2b6baa 100644 --- a/src/presence.c +++ b/src/presence.c @@ -54,10 +54,11 @@ struct _Resource { GabblePresenceId status; gchar *status_message; gint8 priority; - /* The last time we saw an available (or chatty! \o\ /o/) presence for - * this resource. - */ - time_t last_available; + /* The last sequence_number when we saw an available (or chatty! \o\ /o/) + * presence for this resource. It seems OK to assume this won't wrap + * around, since that'd take > 4000 million presence updates in a + * single session. */ + guint last_available; }; struct _GabblePresencePrivate { @@ -72,6 +73,10 @@ struct _GabblePresencePrivate { guint olpc_views; gchar *active_resource; + + /* Used to get a monotonically increasing sequence number for + * last_available. */ + guint sequence_number; }; static Resource * @@ -144,6 +149,7 @@ gabble_presence_init (GabblePresence *self) priv->data_forms = g_ptr_array_new_with_free_func ( (GDestroyNotify) g_object_unref); priv->resources = NULL; + priv->sequence_number = 1; self->status = GABBLE_PRESENCE_UNKNOWN; } @@ -462,8 +468,7 @@ gabble_presence_update (GabblePresence *presence, GabblePresenceId status, const gchar *status_message, gint8 priority, - gboolean *update_client_types, - time_t now) + gboolean *update_client_types) { GabblePresencePrivate *priv = presence->priv; Resource *res; @@ -539,7 +544,7 @@ gabble_presence_update (GabblePresence *presence, res->priority = priority; if (res->status >= GABBLE_PRESENCE_AVAILABLE) - res->last_available = now; + res->last_available = (priv->sequence_number++); } /* select the most preferable Resource and update presence->* based on our diff --git a/src/presence.h b/src/presence.h index e2d5181..c9ec8ea 100644 --- a/src/presence.h +++ b/src/presence.h @@ -90,8 +90,7 @@ GabblePresence* gabble_presence_new (void); gboolean gabble_presence_update (GabblePresence *presence, const gchar *resource, GabblePresenceId status, const gchar *status_message, gint8 priority, - gboolean *update_client_types, - time_t now); + gboolean *update_client_types); void gabble_presence_set_capabilities (GabblePresence *presence, const gchar *resource, diff --git a/tests/test-presence.c b/tests/test-presence.c index 8319cb2..2d9fdab 100644 --- a/tests/test-presence.c +++ b/tests/test-presence.c @@ -19,7 +19,6 @@ big_test_of_doom (void) const gchar *resource; GabblePresence *presence; GabbleCapabilitySet *cap_set; - time_t now = time (NULL); GPtrArray *data_forms = g_ptr_array_new (); /* When we create a new presence, we know nothing about the contact in @@ -33,16 +32,16 @@ big_test_of_doom (void) * contact's presence. */ g_assert (TRUE == gabble_presence_update (presence, NULL, - GABBLE_PRESENCE_OFFLINE, NULL, 0, NULL, now)); + GABBLE_PRESENCE_OFFLINE, NULL, 0, NULL)); g_assert (GABBLE_PRESENCE_OFFLINE == presence->status); g_assert (NULL == presence->status_message); /* offline presence from unknown resource: no change */ g_assert (FALSE == gabble_presence_update (presence, "foo", - GABBLE_PRESENCE_OFFLINE, NULL, 0, NULL, now)); + GABBLE_PRESENCE_OFFLINE, NULL, 0, NULL)); /* available presence from unknown resource: change */ g_assert (TRUE == gabble_presence_update (presence, "foo", - GABBLE_PRESENCE_AVAILABLE, NULL, 0, NULL, now)); + GABBLE_PRESENCE_AVAILABLE, NULL, 0, NULL)); /* accumulated presence has changed; status message unchanged */ g_assert (GABBLE_PRESENCE_AVAILABLE == presence->status); @@ -50,10 +49,10 @@ big_test_of_doom (void) /* available presence again; no change */ g_assert (FALSE == gabble_presence_update (presence, "foo", - GABBLE_PRESENCE_AVAILABLE, NULL, 0, NULL, now)); + GABBLE_PRESENCE_AVAILABLE, NULL, 0, NULL)); /* available presence again, but with status message: change */ g_assert (TRUE == gabble_presence_update (presence, "foo", - GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL, now)); + GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL)); /* accumulated presence unchanged; status message changed */ g_assert (GABBLE_PRESENCE_AVAILABLE == presence->status); @@ -61,15 +60,12 @@ big_test_of_doom (void) /* same presence again; no change */ g_assert (FALSE == gabble_presence_update (presence, "foo", - GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL, now)); - - /* time passes */ - now++; + GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL)); /* presence from different resource, but equal present-ness and equal * status message; unchanged */ g_assert (FALSE == gabble_presence_update (presence, "bar", - GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL, now)); + GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL)); g_assert (GABBLE_PRESENCE_AVAILABLE == presence->status); g_assert (0 == strcmp ("status message", presence->status_message)); @@ -78,49 +74,34 @@ big_test_of_doom (void) g_assert (0 == strcmp ("bar", gabble_presence_pick_resource_by_caps (presence, 0, NULL, NULL))); - /* mountain ranges form */ - now++; - /* presence from different resource, but equal present-ness and different * status message; changed */ g_assert (TRUE == gabble_presence_update (presence, "baz", - GABBLE_PRESENCE_AVAILABLE, "dingbats", 0, NULL, now)); + GABBLE_PRESENCE_AVAILABLE, "dingbats", 0, NULL)); g_assert (GABBLE_PRESENCE_AVAILABLE == presence->status); g_assert (0 == strcmp ("dingbats", presence->status_message)); - /* babies are born */ - now++; - /* presence with higher priority; presence and message changed */ g_assert (TRUE == gabble_presence_update (presence, "bar", - GABBLE_PRESENCE_AVAILABLE, "dingoes", 1, NULL, now)); + GABBLE_PRESENCE_AVAILABLE, "dingoes", 1, NULL)); g_assert (GABBLE_PRESENCE_AVAILABLE == presence->status); g_assert (0 == strcmp ("dingoes", presence->status_message)); - /* third-world dictators are deposed */ - now++; - /* now foo is newer, so the next voip call would prefer that */ g_assert (FALSE == gabble_presence_update (presence, "foo", - GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL, now)); + GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL)); g_assert (0 == strcmp ("foo", gabble_presence_pick_resource_by_caps (presence, 0, NULL, NULL))); - /* portal 2 is released */ - now++; - /* presence from first resource with greated present-ness: change */ g_assert (TRUE == gabble_presence_update (presence, "foo", - GABBLE_PRESENCE_CHAT, "status message", 0, NULL, now)); - - /* seasons change */ - now++; + GABBLE_PRESENCE_CHAT, "status message", 0, NULL)); /* make bar be the latest presence: no change, since foo is more present */ g_assert (FALSE == gabble_presence_update (presence, "bar", - GABBLE_PRESENCE_AVAILABLE, "dingoes", 1, NULL, now)); + GABBLE_PRESENCE_AVAILABLE, "dingoes", 1, NULL)); /* we still prefer foo for the voip calls, because it's more present */ g_assert (0 == strcmp ("foo", @@ -136,7 +117,7 @@ big_test_of_doom (void) /* give voice cap to second resource, but make priority negative */ g_assert (FALSE == gabble_presence_update (presence, "bar", - GABBLE_PRESENCE_AVAILABLE, "dingoes", -1, NULL, now)); + GABBLE_PRESENCE_AVAILABLE, "dingoes", -1, NULL)); cap_set = gabble_capability_set_new (); gabble_capability_set_add (cap_set, NS_GOOGLE_FEAT_VOICE); gabble_presence_set_capabilities (presence, "bar", cap_set, data_forms, 0); @@ -161,7 +142,7 @@ big_test_of_doom (void) /* presence turns up from null resource; it trumps other presence regardless * of whether status is more present or not */ g_assert (TRUE == gabble_presence_update (presence, NULL, - GABBLE_PRESENCE_OFFLINE, "gone", 0, NULL, now)); + GABBLE_PRESENCE_OFFLINE, "gone", 0, NULL)); g_assert (GABBLE_PRESENCE_OFFLINE == presence->status); g_assert (0 == strcmp ("gone", presence->status_message)); @@ -187,15 +168,14 @@ static void prefer_higher_priority_resources (void) { GabblePresence *presence = gabble_presence_new (); - time_t now = time (NULL); - /* 'foo' and 'bar' are equally available, at the same time, but bar has a - * lower priority. + /* 'foo' and 'bar' are equally available, and were last available + * at the same time (i.e. never), but bar has a lower priority. */ - gabble_presence_update (presence, "foo", GABBLE_PRESENCE_AVAILABLE, "foo", 10, - NULL, now); - gabble_presence_update (presence, "bar", GABBLE_PRESENCE_AVAILABLE, "bar", 5, - NULL, now); + gabble_presence_update (presence, "foo", GABBLE_PRESENCE_AWAY, "foo", 10, + NULL); + gabble_presence_update (presence, "bar", GABBLE_PRESENCE_AWAY, "bar", 5, + NULL); /* We should be sure to prefer "foo"'s status message to "bar"'s. */ diff --git a/tests/twisted/client-types.py b/tests/twisted/client-types.py index a1c698b..6e56106 100644 --- a/tests/twisted/client-types.py +++ b/tests/twisted/client-types.py @@ -214,12 +214,12 @@ def test2(q, bus, conn, stream): dbus_interface=cs.CONN_IFACE_CLIENT_TYPES) assertSameSets(['pc'], types[handle]) - # phone comes online + # Phone comes online. It "was available more recently" so it wins. contact_online(q, conn, stream, marco_phone, PHONE, initial=False) types = conn.GetClientTypes([handle], dbus_interface=cs.CONN_IFACE_CLIENT_TYPES) - assertSameSets(['pc'], types[handle]) + assertSameSets(['phone'], types[handle]) sync_stream(q, stream) @@ -228,10 +228,7 @@ def test2(q, bus, conn, stream): # no presence signal - q.expect('dbus-signal', signal='ClientTypesUpdated', - args=[handle, ['phone']]) - - # pidgin comes back online + # Pidgin comes back online. Now it's the more recently available. caps, _, _ = build_stuff(PC) stream.send(make_presence(marco_pidgin, status='hello', caps=caps)) -- 1.8.4.rc3