GNOME Bugzilla – Bug 368644
GossipAccount should have an "id" property
Last modified: 2006-11-16 09:28:55 UTC
Please describe the problem: Xavier recently generalized GossipAccount's parameters, the "id" parameter should become static again since it is anyway hard-coded in many parts. Besides the protocol specific code and widgets (GossipAccountWidgetJabber), account parameter names should not be accessed directly in the code. In the Telepathy branch I will have this property set to the string returned when examining the user's own handle. This way it will not be CM-specific, and should work no matter what the actual protocol parameters are. Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 75747 [details] [review] proposed patch I might have missed a few more references to "id" in the tree.
I don't really understand, the "account" param won't be used anymore in the telepathy branch ? you will get this information somewhere else (in the handle) ? I'm not sure this should be static. When I changed GossipAccount my idea was to keep static user-preferences like the account's name and the auto-connection... Everything protocol-specific should be dynamic. My opinion is the code in gossip where we assume the id exists should be changed with something like if (gossip_account_has_param (account, "id")){ blah } I'm not sure this is the right way, but it's my initial idea for GossipAccount. btw, the static proxy things should be removed and become dynamic.
Every CM is free to use any parameter names it chooses, it is only recommended that they use specific names like "account". A rogue developer could use a name like "login" or "screenname". More realistically, the CM telepathy-salut does not even have any parameters that the user is required to use. So the assumption that _any_ parameter exists is wrong. Gossip depends on the "id" parameter a lot. Philosophically speaking, I think it is safe to assume that in 98% of the cases a protocol will have a unique name for the user. But it is not safe to assume that every connection manager will have that name under the "account" parameter. A much safer way of retrieving that name is by examining the user's own handle (GetSelfHandle followed by InspectHandles). The account parameters in GossipAccount should only be used for protocol-specific parameters at connection time. Besides these parameters there are a few static GObject properties that are supposed to be universal to any account type: "name", "auto-connect", and as of this proposal, "id". Yes, the proxy thing should not be static :)
Created attachment 76689 [details] [review] updated to last HEAD The patch seems ok to me, ok to commit ?
Created attachment 76693 [details] [review] check for compatibility with old accounts.xml files
Fixed some issues in the patch and commited (went in as two commits as I had already reviewed the on under comment #4 when I saw the new one. Thanks!
Mmm, I was going to ask why this is needed? I mean before we had a generalisation for the "id" parameter and that it is not necessary, I don't really see what has changed accept that we now have the old API back? Surely it makes sense to leave it the way it was? Or have I misunderstood?
I consider a real API better than a string based one for anything that we know will be around all the time. Since the ID is the identifier for a specific account it makes sense to have it as part of the API rather than a generic prototype. I think any property that is required by Gossip should be made into a GObject property rather than an ad-hoc property. Using GObject properties and a C API ensures that we catch the cases where the plugin developers (or Gossip code) typos "id" or uses the wrong name entirely.