Bug 24898 - replace or remove MC 5's Account.Interface.Compat.SecondaryVCardFields
Summary: replace or remove MC 5's Account.Interface.Compat.SecondaryVCardFields
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-spec (show other bugs)
Version: unspecified
Hardware: Other All
: low enhancement
Assignee: Vivek Dasmohapatra
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/vi...
Whiteboard: review+ as draft
Keywords: patch
Depends on:
Blocks: 24894 31467
  Show dependency treegraph
 
Reported: 2009-11-04 05:32 UTC by Simon McVittie
Modified: 2010-11-10 06:38 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Simon McVittie 2009-11-04 05:32:42 UTC
Maemo 5 uses various extended interfaces beyond what's in telepathy-spec, including those in:

http://git.collabora.co.uk/?p=telepathy-mission-control.git;a=tree;f=xml;hb=master

One of those is Account.Interface.Compat, which includes the "SecondaryVCardFields" property. We should work out why this property exists and decide whether it makes sense to store it.
Comment 1 Travis Reitter 2010-06-08 08:34:22 UTC
(In reply to comment #0)
> Maemo 5 uses various extended interfaces beyond what's in telepathy-spec,
> including those in:
> 
> http://git.collabora.co.uk/?p=telepathy-mission-control.git;a=tree;f=xml;hb=master
> 
> One of those is Account.Interface.Compat, which includes the
> "SecondaryVCardFields" property. We should work out why this property exists
> and decide whether it makes sense to store it.

I vaguely recall this was used for SIP and Skype, where the primary vCard field was X-SIP/X-SKYPE and the secondary was TEL (as Will mentioned in a recent mailing list post).
Comment 2 Simon McVittie 2010-06-08 09:44:19 UTC
As implemented in MC, SecondaryVCardFields can be set, too. I believe this is so the accounts-ui can flag whether the address book should (offer to) use the SIP and Skype accounts for telephony, or not.
Comment 3 Will Thompson 2010-07-08 03:10:49 UTC
It's slightly awkward to edit this property to configure whether or not to use this account for TEL; but as mentioned on bug 26866 we might want to configure whether or not to use an XMPP account for SIP calls. (Gabble supports this.)

So maybe to make it easier to update:

  Account.UseForFields: as
  Account.SetUseFor(s: Field, b: yes or no)

would do? (Though in practice it would probably mostly be called with 'TEL'.)
Comment 4 Will Thompson 2010-09-16 04:09:56 UTC
So I think that, given this plus bug 26866 (which is merged as a draft), TelepathyQt4 could pretty easily have some sensible convenience API to show the library user a list which is basically the intersection of this property (the vCard fields we'd *like* to use this account for) and Protocol.I.Addressing.AddressableVCardFields.
Comment 5 Vivek Dasmohapatra 2010-10-22 10:07:39 UTC
The story so far (spec branch to follow)
Comment 7 Vivek Dasmohapatra 2010-10-25 04:52:19 UTC
keyword, not whiteboard.
Comment 8 Simon McVittie 2010-10-25 05:24:24 UTC
Spec:

> +    <tp:added version="0.21.3">(draft 1)</tp:added>

0.21.UNRELEASED please; you're assuming that this'll be merged as a draft before the next spec release, which isn't necessarily true.

> +      <p>The Compat interface holds properties necessary for maintaining the
> +        libmissioncontrol compatible layer.</p>

This isn't the Compat interface, and "remain compatible with libmissioncontrol" isn't the reason for this functionality. Why did libmissioncontrol have it? You may be able to derive some useful wording from the Conn.I.Addressing interface.

Something like this, perhaps:

    Some accounts can be used for multiple protocols; for instance, SIP
    and Skype accounts can often be used to contact the PSTN, MSN and
    Yahoo accounts can contact each other, and XMPP accounts can potentially
    contact many protocols via a transport.

    However, if the user does not intend to make use of this functionality,
    user interfaces can improve clarity by not displaying it: for instance,
    if a user prefers to call phone numbers via a particular SIP account,
    when an address book displays a contact with a phone number, it is
    desirable to display a "call with SIP" button for that account, but
    avoid displaying similar buttons for any other configured SIP or
    Skype accounts.

I'd assumed that this interface would have parallel API for vCard fields and for URI schemes, like Conn.I.Addressing does, but I can see that that raises the possibility that they get out of sync: if my account has UseForFields = [] and UseForSchemes = ['tel'], a vCard-field-based UI would not offer it for telephony, but a URI-scheme-based UI would.

If you have any thoughts on whether vCard fields, URI schemes, or both make most sense, I'd like to hear them!

> +        <p>A list of fields indicating the type of URI addressing scheme
> +          the the account can be used for (eg 'tel') indicating the
> +          account is suitable for use by apps offering a telephony UI,
> +          or 'sip' or 'xmpp' for those protocols</p>

Please make it clearer that this is a matter of user preference rather than technical capability - every Skype account can potentially contact the PSTN, but it's up to the user whether they want to use it as such or not.

> +      <arg name="Old_URI_Association" direction="out" type="b">
> +        <tp:docstring><p>The old URI association value</p></tp:docstring>
> +      </arg>

We don't normally do this. Is there a special reason to do so?

> +      <arg name="Association" direction="in" type="b">
> +        <tp:docstring>
> +          <p>Whether to associate or disassociate with/from the URI scheme</p>
> +        </tp:docstring>
> +      </arg>

I prefer wording of the form "True if this account should be offered as a possible way to contact this URI scheme; False if it should not".

-----------------------------------------------

MC:

Your branch seems to be against telepathy-mission-control-5.6, which seems rather inappropriate for a new feature.

> +  <interface name="org.freedesktop.Telepathy.Account.Interface.Addressing">

You need a .DRAFT in there (and the draft used in a trial implementation should generally be byte-for-byte identical to your latest spec proposal). I haven't reviewed the implementation in detail.
Comment 9 Simon McVittie 2010-10-25 05:27:11 UTC
(In reply to comment #8)
> If you have any thoughts on whether vCard fields, URI schemes, or both make
> most sense, I'd like to hear them!

Bear in mind that:

- clients (UIs) can be assumed to understand either specific vCard fields, or specific URI schemes, or both

- for Conn.I.Addressing, connection managers must understand specific vCard fields *and* specific URI schemes

- MC has no particular knowledge of either of those things
Comment 10 Vivek Dasmohapatra 2010-10-25 05:59:50 UTC
Argh, this does not appear to be the right version. Obviously not had enough coffee. Fixing...
Comment 11 Vivek Dasmohapatra 2010-10-25 08:02:57 UTC
> > + <tp:docstring><p>The old URI association value</p></tp:docstring>
> We don't normally do this. Is there a special reason to do so?

No particular reason, it just seemed like a useful bit of info to return:

   The state you asked for is already the case (and maybe you therefore 
   don't need to take some other action)
Comment 12 Will Thompson 2010-10-25 10:00:04 UTC
Can the MC implementation branch also bin the property and code from Account.Interface.Compat that it replaces?
Comment 13 Vivek Dasmohapatra 2010-10-25 10:12:26 UTC
I wasn't sure if that was how we rolled, but if it is, then I don't see why not.
Comment 14 Will Thompson 2010-10-25 10:49:22 UTC
+  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 15 Simon McVittie 2010-11-08 06:37:27 UTC
Let's use this bug for the spec and its clone for the MC implementation.

Spec review:

This looks good enough to be a draft if someone is working on pushing it forward. However, I'd like to move towards a policy of "stable-branch releases never contain draft API", so to have this in MC 5.8 it needs finishing and undrafting.

I'd always assumed that this interface would be in terms of either vCard fields as per Comment #3, or both vCard fields and URI schemes together as per Conn.I.Addressing. Is there a reason to prefer URI schemes over vCard fields? Is there a reason not to have both?
Comment 16 Simon McVittie 2010-11-10 06:38:16 UTC
Undrafted in git for 0.21.5


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.