Bug 31467 - implement Account.I.Addressing in MC
Summary: implement Account.I.Addressing in MC
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: mission-control (show other bugs)
Version: unspecified
Hardware: Other All
: low enhancement
Assignee: Vivek Dasmohapatra
QA Contact: Telepathy bugs list
URL:
Whiteboard: review+ with trivial change
Keywords: patch
Depends on: 24898
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-08 06:31 UTC by Simon McVittie
Modified: 2010-12-09 09:50 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Simon McVittie 2010-11-08 06:31:38 UTC
+++ This bug was initially created as a clone of Bug #24898 +++

Bug #24898 needs a trial implementation before it can be undrafted.

Once it's been proved sensibly implementable, we'll reverse the dependency and repurpose this bug for "implement the undrafted version".
Comment 1 Simon McVittie 2010-11-08 06:32:21 UTC
wjt wrote:

Can the MC implementation branch also bin the property and code from
Account.Interface.Compat that it replaces?


+  if (schemes == NULL)
+    {
+      gchar *empty[] = { NULL };
+      schemes = empty;
+    }

Forgive my C ignorance: is dereferencing 'schemes' actually kosher once we
leave the if {} block? Wouldn't it be more obvious to just not run the
following loop...

+  for(scheme = schemes[pos++]; scheme != NULL; scheme = schemes[pos++])
+    {
+      if (!tp_strdiff (scheme, uri_scheme))
+        old_association = TRUE;
+    }

...which is secret code for old_association = tp_strv_contains (schemes,
uri_scheme);. tp_strv_contains() does the right thing if you pass it a NULL
strv, so the previous block becomes unnecessary.

And then actually I found the pointer arithmetic that followed very hard to
read, so I rewrote the whole function. :) The result is shorter, and (IMHO)
more readable. Top two patches of
<http://git.collabora.co.uk/?p=user/wjt/telepathy-mission-control-wjt.git;a=shortlog;h=refs/heads/account-interface-addressing>.

+    uri_schemes = get_schemes (account_props)
+    assertContains ('scheme-a', uri_schemes)
+    assertContains ('scheme-b', uri_schemes)
+    assertContains ('scheme-c', uri_schemes)
+    assertEquals (len(uri_schemes), 3)

How about assertEquals(set(['scheme-a', 'scheme-b', 'scheme-c']),
set(uri_schemes))? In fact Gabble's copy of servicetest.py has:

def assertSameSets(expected, value):
    exp_set = set(expected)
    val_set = set(value)

    if exp_set != val_set:
        raise AssertionError(
            "expected contents:\n%s\ngot:\n%s" % (
                pretty(exp_set), pretty(val_set)))

to make this easier. Maybe import this to MC's, just after assertEquals() in
servicetest.py (to avoid divergence).

(I don't really like the Association naming, but I can't think of a better one
off-hand.)
Comment 2 Simon McVittie 2010-11-17 02:58:21 UTC
A branch seems to exist. Is it ready for review/merge? If so, please set the patch keyword; I'm happy to review it.
Comment 3 Vivek Dasmohapatra 2010-11-17 04:21:15 UTC
Reviewed by wjt, changes made, I think it's ready for merge.
Comment 4 Simon McVittie 2010-11-17 05:28:00 UTC
> +	mcd-account-addressing.h \

This should ideally be in libmcd_convenience_la_SOURCES rather than mc_headers, because it doesn't contain any libmissioncontrol-server API.

Other than that, r+. Let's have this in 5.7.0.
Comment 5 Simon McVittie 2010-12-09 09:50:43 UTC
Appears to have been fixed in 5.7.0.


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.