Bug 28197

Reviewing Danielle's branch from the perspective of the spec, as if it had been a tp-spec branch:

If this is being worked on actively, it should get into the spec as a draft.

The <interface name=""> should end with .DRAFT for draft API.

It's not instantly clear whether this is an interface that will exist (in the Interfaces property) on all accounts (so its semantics are really more like "PotentiallyExternal"), or only on accounts that are in fact External. Either way, the introductory docstring should explain.

I actually think that since MC core changes are needed for this interface *anyway*, it should probably be omitted from Interfaces if External would be FALSE; this means the External property is unnecessary. (It'll still need to appear in the keyfile.)

Before undrafting this, we need a usable implementation in MC (which is Bug #28192).

+    <property name="ExternalName" tp:name-for-bindings="External_Name"
+      type="s" access="read">
+      <tp:docstring>
+        A human-readable name for program/interface used to edit the
+        account.

Is this localizable (how?), or is it "in the C locale" like Protocol.EnglishName in my branch for Bug #20774? (I'd suggest the latter, in which case please consider stealing the wording from that branch.)

+        A D-Bus well-known name that can be activated and requested to show
+        the appropriate account settings page over D-Bus.
+
+        FIXME: specify the object-path and interface for account editing.
+
+        <tp:rationale>
+          Using

Needs more <p>.

Is the intention that activating the process with org.freedesktop.DBus.Peer.Ping is sufficient, or are you going to define an object path and method that should be called? Alternatively, you could have some text like this:

    No semantics are currently defined for the object paths and interfaces
    implemented by processes with this well-known name: it acts as an
    opaque identifier for the interface used to edit the account.

(In reply to comment #1)

> Is this localizable (how?), or is it "in the C locale" like
> Protocol.EnglishName in my branch for Bug #20774? (I'd suggest the latter, in
> which case please consider stealing the wording from that branch.)

I figure the localised name will be provided by the MC plugin. Since this interface will only be used by MC plugins (surely?), I'm thinking that should work. i18n is not really something I'm strong on though.

> Is the intention that activating the process with
> org.freedesktop.DBus.Peer.Ping is sufficient, or are you going to define an
> object path and method that should be called? Alternatively, you could have
> some text like this:
> 
>     No semantics are currently defined for the object paths and interfaces
>     implemented by processes with this well-known name: it acts as an
>     opaque identifier for the interface used to edit the account.

The eventual intention is to provide an object-path and method that can be called to select a specific account (by account path? by some other detail we expose?), but I didn't want to get dragged down in specifying that just now, and was planning in the short term to use it as a unique identifier.

Updated this per feedback.

(In reply to comment #0)
> +++ This bug was initially created as a clone of Bug #28192 +++
> 
> With the introduction of account plugins for MC it is now possible to have MC
> exporting accounts that cannot be edited directly (i.e. account parameters are
> calculated using some other information, e.g. for single-sign-on style
> accounts).

Why is this called external? what does external mean? Using Backend as a name might be better (then we can have one for MC itself as well with just a default value and we don't have an interface that's only sometimes there)

> This patch adds a new optional interface, Account.Interface.External, which
> presents 3 properties:
>  - b:External - indicates whether the account is external, and thus immutable;

I'm not sure if this implication is correct. We might need a bit more fine-grained control. For some backend we might be able to disable and enable the account for example, but not change it's parameters. 

>  - s:ExternalName - a name for the interface that will be opened to edit the
> account; and

It seems like this is a human readable name, which i'm not entirely sure of as we have no way to actually launch the external helper atm?

Could this be the (dbus like name) name of the backend instead? In that case a UI could have a translation internally (like we do for protocols) and/or know how to edit it.

>  - s:ExternalEditor - a D-Bus well-known name for a program that can be
> activated to edit the account -- the mechanism for doing this hasn't been
> specified yet, in the short term we intend to simply use these as unique
> identifiers.

In that case it might be best to leave it out for now? In general i'm not sure if we'll always ahve some external thing to edit it or you UI might internally load some widgets/helper to edit it itself.. I'm not generally against this but if you add it also specify how to use it please :) 

Also can we add:

a{sv}: BackendAccountProperties, which contains a backend specific mapping for misc? (Could included for example the account ID from the perspective of the backend and some other backend specific meta-data)

(In reply to comment #4)
> 
> Also can we add:
> 
> a{sv}: BackendAccountProperties, which contains a backend specific mapping for
> misc? (Could included for example the account ID from the perspective of the
> backend and some other backend specific meta-data)

Maybe it's crazy talk, but it seems like every backend will have to include some kind of identifier, so maybe we could have an Identifier:s property?

It seems like ExternalEditor, or whatever will replace it, will need the identifier.

(In reply to comment #5)
> (In reply to comment #4)
> > 
> > Also can we add:
> > 
> > a{sv}: BackendAccountProperties, which contains a backend specific mapping for
> > misc? (Could included for example the account ID from the perspective of the
> > backend and some other backend specific meta-data)
> 
> Maybe it's crazy talk, but it seems like every backend will have to include
> some kind of identifier, so maybe we could have an Identifier:s property?
> 
> It seems like ExternalEditor, or whatever will replace it, will need the
> identifier.

Sounds useful to me, i was thinking something like that would be in the properties dict, but we can also make that one a first class citizen. I'd make it a variant though to prevent needing to define a serialisation from your storages unique identifier to a string.

So based on a discussion with Smcv and Danni this morning we came to the following conclusions:

First change the name to Account.Interface.Storage and have this interface on all Accounts (External implied it was something external which is't necessarily true and simon didn't like the name Backend, so Storage it is).

interface: org.freedesktop.Telepathy.Account.Interface.Storage
  + Immutable property: StorageProvider, string
      Dbus namespaced name of the plugin in the AccountManager implementation 
      providing the storage. When this is the empty string the account is internally stored.

  + Immutable property: StorageIdentifier, variant
      Unique identification of the account within the backends Storage. What's stored in the variant
     is specific to the storage implementation.

     Rationale: Different storage systems will have their own way of uniquely identifying an account, 
     some might use an integer others a string.  Given that all users of this property should have 
     direct knowledge of the backend they should know what types to expect and how to handle it.

  + property: StorageSpecificInformation , a{sv}

      Map containing information specific to one Storage system. This map contains extra information 
      specific to the storage method. The possible keys depend solely on the used Storage method 
      and won't be further defined by the Tp Spec.

  + immutable property: StorageRestrictions, flags
     
      Bitfield which defines what restrictions this Storage method has, initial set:
         IMMUTABLE_PARAMETERS: The account parameters can't be changed via Telepathy
         IMMUTABLE_ENABLEDNESS: The account can't be enabled/disabled via Telepathy
         IMMUTABLE_PRESENCE: Presence can't be changed via Telepathy

Ok,

Here is a branch up for review with the new spec. I'll add it to bug #28192 as well for the mc review.

http://git.collabora.co.uk/?p=user/eitan/telepathy-mission-control.git;a=shortlog;h=refs/heads/account-storage

Linking to spec branch.

http://git.collabora.co.uk/?p=user/eitan/telepathy-spec.git;a=shortlog;h=refs/heads/account-storage

Each property should say whether it can change, and if so, how you get notified. I infer that the intention is (and here's how I'd phrase it):

StorageProvider: This property cannot change once an Account has been created.

StorageIdentifier: This property cannot change once an Account has been created.

StorageRestrictions: This property cannot change once an Account has been created.

StorageSpecificInformation: Whether values in this map can change is defined by the StorageProvider. There is no change notification, so the property must be re-retrieved using Get or GetAll if an up to date value is needed.

> +        Dbus namespaced name of the plugin in the AccountManager implementation

It's spelled D-Bus, and I'd phrase this more like: The name of the account storage implementation, which SHOULD start with a reversed domain name in the same way as D-Bus interface names.

> +      <tp:docstring>
> +        Unique identification of the account within the backends Storage.
> +        What's stored in the variant is specific to the storage implementation.
> +
> +        <tp:rationale>

... within the storage backend. The contents of the variant are defined by the <tp:member-ref>StorageProvider</tp:member-ref>.

Spec-lint: Please put <tp:docstring xmlns="http://www.w3.org/1999/xhtml"> on any docstring with multiple paragraphs (rationale counts as a <div>), and put <p> around each paragraph (including inside the <tp:rationale>).

> +          identifying an account, some might use an integer others a string.

... an account, typically an integer or a string.

> +    <property name="StorageSpecificInformation"
> +      <tp:docstring>
> +        Map containing information specific to one Storage system. This map
> +        contains extra information specific to the storage method. The possible
> +        keys depend solely on the used Storage method and won't be further
> +        defined by this spec.
> +      </tp:docstring>

This should also say that the AM won't use this information, and should have a rationale. I'd phrase this as:

<p>Map containing information specific to the storage backend. The keys and the types of their values are defined by the <tp:member-ref>StorageProvider</tp:member-ref>, and are not interpreted by the AccountManager implementation.</p>

<tp:rationale>
  <p>This can be used to provide additional hints to user interfaces aware of a specific storage provider, without requiring those user interfaces to use the <tp:member-ref>StorageIdentifier</tp:member-ref> to query the storage provider directly.</p>
</tp:rationale>

That rationale seems a bit weak, though... apart from fewer round-trips, that property just gives us the ability for two cooperating backends to agree on a hint name.

> +    <tp:flags name="Storage_Restriction_Flags" value-prefix="Storage_Flag"

Either change the value-prefix to Storage_Restriction_Flag or the name to Storage_Flags, please. I think I prefer the former; or possibly it could be Storage_Restriction and Storage_Restrictions?

I'm not sure that I like the names starting with "Immutable" - the enabledness/parameters/presence can presumably change, just not via Telepathy? We generally use Immutable to mean "can't change under any circumstances".

Perhaps Cannot_Set_Parameters, Cannot_Set_Enabled, Cannot_Set_Presence?

Spec-lint: I'd prefer a blank line between each <tp:flag> and the next.

(In reply to comment #10)
> Each property should say whether it can change, and if so, how you get
> notified. I infer that the intention is (and here's how I'd phrase it):
> 
> StorageProvider: This property cannot change once an Account has been created.
> 
> StorageIdentifier: This property cannot change once an Account has been
> created.
> 
> StorageRestrictions: This property cannot change once an Account has been
> created.
> 
> StorageSpecificInformation: Whether values in this map can change is defined by
> the StorageProvider. There is no change notification, so the property must be
> re-retrieved using Get or GetAll if an up to date value is needed.

Added in 432f3c9.

> 
> > +        Dbus namespaced name of the plugin in the AccountManager implementation
> 
> It's spelled D-Bus, and I'd phrase this more like: The name of the account
> storage implementation, which SHOULD start with a reversed domain name in the
> same way as D-Bus interface names.

Rephrased in 9db53de.
> 
> > +      <tp:docstring>
> > +        Unique identification of the account within the backends Storage.
> > +        What's stored in the variant is specific to the storage implementation.
> > +
> > +        <tp:rationale>
> 
> ... within the storage backend. The contents of the variant are defined by the
> <tp:member-ref>StorageProvider</tp:member-ref>.
> 
> Spec-lint: Please put <tp:docstring xmlns="http://www.w3.org/1999/xhtml"> on
> any docstring with multiple paragraphs (rationale counts as a <div>), and put
> <p> around each paragraph (including inside the <tp:rationale>).
> 
> > +          identifying an account, some might use an integer others a string.
> 
> ... an account, typically an integer or a string.
> 

Rephrased in bb5aa76.

> > +    <property name="StorageSpecificInformation"
> > +      <tp:docstring>
> > +        Map containing information specific to one Storage system. This map
> > +        contains extra information specific to the storage method. The possible
> > +        keys depend solely on the used Storage method and won't be further
> > +        defined by this spec.
> > +      </tp:docstring>
> 
> This should also say that the AM won't use this information, and should have a
> rationale. I'd phrase this as:
> 
> <p>Map containing information specific to the storage backend. The keys and the
> types of their values are defined by the
> <tp:member-ref>StorageProvider</tp:member-ref>, and are not interpreted by the
> AccountManager implementation.</p>
> 
> <tp:rationale>
>   <p>This can be used to provide additional hints to user interfaces aware of a
> specific storage provider, without requiring those user interfaces to use the
> <tp:member-ref>StorageIdentifier</tp:member-ref> to query the storage provider
> directly.</p>
> </tp:rationale>
> 
> That rationale seems a bit weak, though... apart from fewer round-trips, that
> property just gives us the ability for two cooperating backends to agree on a
> hint name.
> 

Rephrased in 8efb836.


> > +    <tp:flags name="Storage_Restriction_Flags" value-prefix="Storage_Flag"
> 
> Either change the value-prefix to Storage_Restriction_Flag or the name to
> Storage_Flags, please. I think I prefer the former; or possibly it could be
> Storage_Restriction and Storage_Restrictions?
> 
> I'm not sure that I like the names starting with "Immutable" - the
> enabledness/parameters/presence can presumably change, just not via Telepathy?
> We generally use Immutable to mean "can't change under any circumstances".
> 
> Perhaps Cannot_Set_Parameters, Cannot_Set_Enabled, Cannot_Set_Presence?
> 
> Spec-lint: I'd prefer a blank line between each <tp:flag> and the next.

Fixed in 59d7ea9.

Thanks, please merge the revised version as a draft.

Things I'm unsure about, which I'd like to sort out before undrafting (possibly by someone overruling me):

"Cannot_Set_Enabledness" seems a bit awkward: do the rest of the spec cabal prefer that, or the "Cannot_Set_Enabled" that I suggested?

Given that the interpretation of StorageSpecificInformation is backend-specific, why would clients prefer to use it rather than going directly to the backend? The only benefit I can think of is if you know that two backends agree on the meaning of some keys, which seems fairly tenuous.

The lack of change-notification make StorageSpecificInformation somewhat useless for round-trip avoidance, unless you happen to know that the keys you're interested in are immutable.

(In reply to comment #12)
> Thanks, please merge the revised version as a draft.
> 
> Things I'm unsure about, which I'd like to sort out before undrafting (possibly
> by someone overruling me):
> 
> "Cannot_Set_Enabledness" seems a bit awkward: do the rest of the spec cabal
> prefer that, or the "Cannot_Set_Enabled" that I suggested?
> 

I agree, I'll change it.

> Given that the interpretation of StorageSpecificInformation is
> backend-specific, why would clients prefer to use it rather than going directly
> to the backend? The only benefit I can think of is if you know that two
> backends agree on the meaning of some keys, which seems fairly tenuous.
> 

There is that, and also the fact that you don't need any hard dependencies in the UI, just an awareness of the other stores out there. It makes what I am doing now much easier.

> The lack of change-notification make StorageSpecificInformation somewhat
> useless for round-trip avoidance, unless you happen to know that the keys
> you're interested in are immutable.

Maybe this should be immutable as well.

Merge to master.

(In reply to comment #12)

> Given that the interpretation of StorageSpecificInformation is
> backend-specific, why would clients prefer to use it rather than going directly
> to the backend? The only benefit I can think of is if you know that two
> backends agree on the meaning of some keys, which seems fairly tenuous.

Your given client might be hardcoded to use this information without having to hardcode the information itself. Especially since we haven't yet specced out generic name/launch details that clients can use, this property allows us to store that information without having to constantly release updated to Empathy when it changes (just the storage plugin).

(Reopening: please make any further changes in this bug, and leave it open until this is stable API.)

(In reply to comment #13)
> (In reply to comment #12)
> > The lack of change-notification make StorageSpecificInformation somewhat
> > useless for round-trip avoidance, unless you happen to know that the keys
> > you're interested in are immutable.
> 
> Maybe this should be immutable as well.

I think SSI is useful to have. If in practice it is always immutable then it could be worth just defining it to be immutable (which makes bindings nicer), and say that for mutable stuff clients can go talk to the storage backend directly (or have a new property for that later) if needed.

Otherwise, this is fine for undrafting by me: it's been implemented, and looks sane.

(In reply to comment #17)
> (In reply to comment #13)
> > Maybe this should be immutable as well.
> 
> I think SSI is useful to have. If in practice it is always immutable then it
> could be worth just defining it to be immutable (which makes bindings nicer),
> and say that for mutable stuff clients can go talk to the storage backend
> directly (or have a new property for that later) if needed.
> 

I guess I take it back. Of course we cannot guarantee any external account property to be immutable, the external account editing app could change things at any given moment.

I think a disclaimer of "this property is only relevant for the moment it is retrieved" would help.

Yeah, I think changing:

> Whether values in this map can change is defined by the StorageProvider. There is no change notification, so the property must be re-retrieved using Get or GetAll if an up to date value is needed.

to something like:

> As the values in this map may change at any time (due to an external application manipulating the storage provider directly), this property should not be cached; it should instead be retrieved each time it is needed.

And then we're good to go. If we want to add change notification later, we could do so.

Created attachment 36494 [details] [review]
Explained StorageSpecificInformation's mutable tendancies better.

(In reply to comment #20)
> Created an attachment (id=36494) [details]
> Explained StorageSpecificInformation's mutable tendancies better.

It's in my branch as well. I attached it here for a quick read.

Review of attachment 36494 [details] [review]:

Looks good to me, please merge this patch. Is there anything else pending?

Review of attachment 36494 [details] [review]:

Looks good to me, please merge this patch. Is there anything else pending?

Review of attachment 36494 [details] [review]:

Looks good to me, please merge this patch. Is there anything else pending?

I've had another look through this and I think it's fine to undraft, so: approved for undrafting by smcv/wjt.

Text changes which would be nice to have, but don't change the meaning:

* Cannot_Set_Parameters should have a hyperlink: The account's _Parameters_ can't be changed via Telepathy

* Cannot_Set_Enabled should be: The account's _Enabled_ property can't be changed via Telepathy

* Cannot_Set_Presence should be: The account's _AutomaticPresence_ and _RequestedPresence_ can't be changed via Telepathy

My branch smcv/external undrafts the interface, adds NEWS, and makes these changes.

Branch (also in URL field):
http://git.collabora.co.uk/?p=user/smcv/telepathy-spec-smcv.git;a=shortlog;h=refs/heads/external
HTML for your convenience:
http://people.freedesktop.org/~smcv/telepathy-spec-external/spec/

(Eitan: in future, if you're going to say "in my branch" please reference it in the URL field, or provide a link, or something :-)

Excuse my newbness, I only have a partial idea of how to undraft.
Besides removing .DRAFT and the causes-havoc attribute, does this need to get copied into tp-glib? What is the process for that?

(In reply to comment #26)
> Excuse my newbness, I only have a partial idea of how to undraft.
> Besides removing .DRAFT and the causes-havoc attribute, does this need to get
> copied into tp-glib? What is the process for that?

telepathy-glib should never include spec versions that aren't a numbered release, so the process is:

* merge the branch that undrafts it
* time passes (depending whether the undrafted stuff and other changes justify an immediate release, or whether we're trying to get other things into a release at the same time, or whatever)
* telepathy-spec 0.19.x is released
* someone (in practice, me) updates telepathy-glib to spec 0.19.x and enables code generation for the new interfaces

So, anything beyond the branch I've added here is a job for the tp-spec release manager (in practice, usually me) or for tp-glib/tp-qt4 developers (in practice, usually me and Andre).

(In reply to comment #27)
> So, anything beyond the branch I've added here is a job for the tp-spec release
> manager (in practice, usually me) or for tp-glib/tp-qt4 developers (in
> practice, usually me and Andre).

Great! (I'm assuming you will merge your own branch).

Will be in 0.19.8.