After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 379154 - Remove some functions from GossipProtocol that can't be implemented in TELEPATHY
Remove some functions from GossipProtocol that can't be implemented in TELEPATHY
Status: RESOLVED WONTFIX
Product: gossip
Classification: Deprecated
Component: General
0.19
Other Linux
: Normal normal
: ---
Assigned To: Gossip Maintainers
Gossip Maintainers
Depends on:
Blocks: 409498
 
 
Reported: 2006-11-25 14:33 UTC by Xavier Claessens
Modified: 2007-05-26 07:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch (23.02 KB, patch)
2006-11-25 14:36 UTC, Xavier Claessens
none Details | Review

Description Xavier Claessens 2006-11-25 14:33:44 UTC
I suggest to remove those functions from GossipProtocol and move them to gossip-account-widget-jabber:

_is_valid_username : TELEPATHY can't check for validity. Some protocols (link-local) don't even have an username. So this function make no sense in a protocol-independent API.

_get_default_server : At account creating a default server is set. This function was manly used to convert gmail.com to talk.google.com but this can't be done in TELEPATHY. Make more sense in the jabber specific account setting widget.

_get_default_port : Same as above.

I'm not sure for:

is_ssl_supported : The current implementation don't depend on the server, seems to only check if loudmouth supports ssl ? With TELEPATHY we can't check that, for protocols that supports ssl we just have an account parameter to set it. Can't we remove this function and assume ssl is supported for Jabber ?

get_example_username : This is not supported by TELEPATHY, but it was discussed and a solution proposed is to set an example to all account parameters (or let this field empty if example don't make sense). Seems a good idea to have a more general API to get examples, not only for the username.
Comment 1 Xavier Claessens 2006-11-25 14:36:02 UTC
Created attachment 77127 [details] [review]
proposed patch

This patch removes the 3 first functions. It also setup the GossipProtocol once it's added to the session (no need to wait a connection) because some protocol's functions was used before it's initialised...
Comment 2 Martyn Russell 2006-11-26 19:12:02 UTC
Hmm, I have had a think about this, and I think it is right the way it is.

Not all protocols will have all the features we provide in GossipProtocol, but that still doesn't mean we should just remove them, they can simply not use that API.

Having an API for validating the username and providing an example user name IMO should belong in the backend since that has intimate knowledge about the the protocol. If all it means is duplicating those functions in modules like gossip-account-widget-jabber, then I think that is unnecessary and it has always been my preference to NOT mix protocol technicalities and UI code, it can get ugly.

I am not completely against this patch of course, I just don't really think it makes sense. I mean, functions like _get_example_username() are great for multiple places (adding contacts, adding accounts, etc).

I would like to know what Richard and/or Micke think about this?
Comment 3 Mikael Hallendal 2006-12-23 14:05:41 UTC
Hmm, is_valid_username makes a lot of sense in a protocol independant API, in a protocol dependant API it doesn't though. However, since we are adding a protocol dependant UI we can also do it at that level.

get_default_server and get_default_port makes sense in a protocol independant API (GossipProtocol). It can be that an MSN backend returns the default MSN-server to connect to and the port to connect but have it user overridable in case they change that in a future version.

is_ssl_supported is a bit tricky and we should probably have a better way for this than just is_ssl_supported. It might be that we should only remove this and make sure that an error from Loudmouth (or other protocol backends) is propagated up if you try to connect and it's not supported by the local protocol or the remote server.

Get example name is similar to is_valid_username. Since Gossip will have a protocol dependant UI and we won't support connecting to any protocols that we don't know of we can have this in the UI layer.
Comment 4 Mikael Hallendal 2006-12-23 14:10:46 UTC
To summarize: 

* Moving is_valid_username and get_example_username to the protocol specific account settings widgets makes sense. 

* Having get_default_server makes sense to keep in the backend. Could return NULL if unknown or not feasible (link-local for example).

* Same as get_default_server

* is_ssl_supported should be handled in a better way to be able to show whether a certain backend supports a secure connection or not. Or change it the other way around and have a way of saying "Don't connect if you can't setup a secure connection".
Comment 5 Xavier Claessens 2007-01-25 14:50:26 UTC
(In reply to comment #4)
> To summarize: 
> 
> * Moving is_valid_username and get_example_username to the protocol specific
> account settings widgets makes sense. 

Ok.

> * Having get_default_server makes sense to keep in the backend. Could return
> NULL if unknown or not feasible (link-local for example).

In fact, we already have a default value for every parameter when creating the account (with both telepathy and jabber backend). The problem here is extracting the server from the jid when configuring the account. There is a little code in jabber backend to convert gmail.com to talk.google.com server, this code should be the same for telepathy accounts so should go to the jabbet settings widget I think.

for _get_default_port that's the same kind of thing, accounts already has a default value when creating it, the problem here is changing the port depending on if ssl is activated or not. Actually it's just if (ssl) return 5223 else return 5222. Would be great to have the same thing when configuring a telepathy jabber account, but it's impossible to get the default value of a parameter depending on the value of another parameter. This can easily be done in the jabber settings widget but I agree it's more a backend stuff... what can we do here to have a sane behavious using telepathy ?
Comment 6 Martyn Russell 2007-05-26 07:49:01 UTC
Marking as WONTFIX, since we already do all the points you outlined Micke.