Bug 28469 - [PATCH] add support for PurpleAccountUserSplit
Summary: [PATCH] add support for PurpleAccountUserSplit
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: haze (show other bugs)
Version: 0.10
Hardware: Other All
: medium enhancement
Assignee: Stefan Becker
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/wj...
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2010-06-09 11:18 UTC by Stefan Becker
Modified: 2010-06-11 02:02 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Output from git-format-patch (8.47 KB, patch)
2010-06-09 11:18 UTC, Stefan Becker
Details | Splinter Review
connection(-manager): PurpleAccountUserSplits should be optional parameters (2.24 KB, patch)
2010-06-10 09:58 UTC, Stefan Becker
Details | Splinter Review

Description Stefan Becker 2010-06-09 11:18:27 UTC
Created attachment 36180 [details] [review]
Output from git-format-patch

On Maemo/MeeGo (at least on the N900) it is impossible to enter "," in the Account Setup UI, as it is rejected as illegal character. This poses a problem for protocols that use PurpleAccountUserSplit, because telepathy-haze maps the account name to one string attribute and that must contain the real Purple account name *with* ",", e.g. 

   joe.d.user@company.com,DOMAIN\jduser

The attached patch contains a proposal how to map user splits automatically to multiple fields:

  - backward compatible, i.e. it is only enabled for known protocols that define a mapping

  - known_protocols_info: add "usersplit1:param1,usersplit2:param2,..." to the mapping string, one "usersplitX" for each PurpleAccountUserSplit used by the protocol

  - "param1" is the name visible to the telepathy UI as mandatory parameter

  - when the account is created merge all "usersplitX" fields to the real Purple account name

I have added "sipe" (http://sipe.sourceforge.net) to the list of known protocols and used it as an example to test the implementation. The patch is based on todays git HEAD.
Comment 1 Will Thompson 2010-06-09 15:10:39 UTC
Hey, thanks for the patch! I had an outstanding branch I forgot to merge that refactored how the parameter mappings worked, so I've merged that branch and am rebasing your patch onto it now. I'll attach it here for you to take a look when I'm done.
Comment 2 Will Thompson 2010-06-09 16:11:46 UTC
So I've got a git branch at <http://git.collabora.co.uk/?p=user/wjt/telepathy-haze;a=shortlog;h=refs/heads/usersplit> in which I've rebased your branch onto master-as-of-an-hour-ago. ☺

I made a couple, changes to your patches in the process — removed tabs, used g_list_length (), misc. coding style stuff (I'm aware Haze is not exactly the epitome of style consistency; I'm vaguely hoping it'll move towards Telepathy style), fixed leaking 'username' in haze_connection_create_account() if the account already exists — but (I hope) nothing functional. It seems to still work, but I don't actually have a SIPE server to connect to. Any chance you could test it against your SIPE server? If it flies, I'll merge the branch.

Thanks again!
Comment 3 Stefan Becker 2010-06-10 05:58:20 UTC
Thanks, I cloned the git and your rebase seems to work in my quick test.

BTW: this is not a SIPE only issue. A quick scan through Pidgin protocol plugin source code reveals:

bonjour/bonjour.c:      PurpleAccountUserSplit *split;
irc/irc.c:      PurpleAccountUserSplit *split;
jabber/libxmpp.c:       PurpleAccountUserSplit *split;
null/nullprpl.c:  PurpleAccountUserSplit *split = purple_account_user_split_new(
silc10/silc.c:  PurpleAccountUserSplit *split;
silc/silc.c:    PurpleAccountUserSplit *split;
simple/simple.c:        PurpleAccountUserSplit *split;

So these changes might help those protocols too...


I realized that making them mandatory parameters is not correct, i.e. it will not work for users that need to leave one of the parameters empty. I'll submit another patch based on your re-base once I'm done.


As for the style: have you considered adding at least some hints for Emacs to end of each file? Then the Emacs C mode will make sure that the code is entered using the correct style. Here is the settings we use for pidgin-sipe code:

/*
  Local Variables:
  mode: c
  c-file-style: "bsd"
  indent-tabs-mode: t
  tab-width: 8
  End:
*/
Comment 4 Will Thompson 2010-06-10 06:06:10 UTC
(In reply to comment #3)
> Thanks, I cloned the git and your rebase seems to work in my quick test.

Great!

> BTW: this is not a SIPE only issue. A quick scan through Pidgin protocol plugin
> source code reveals:
> 
> bonjour/bonjour.c:      PurpleAccountUserSplit *split;
> irc/irc.c:      PurpleAccountUserSplit *split;
> jabber/libxmpp.c:       PurpleAccountUserSplit *split;
> null/nullprpl.c:  PurpleAccountUserSplit *split =
> purple_account_user_split_new(
> silc10/silc.c:  PurpleAccountUserSplit *split;
> silc/silc.c:    PurpleAccountUserSplit *split;
> simple/simple.c:        PurpleAccountUserSplit *split;
> 
> So these changes might help those protocols too...

Yeah, that's true. (That said... I don't really expect anyone to use the first three through Haze (we have native CMs for Bonjour, IRC and Jabber, which work better), although IRC is bug 14213 and Jabber is bug 14212, and I had a quick stab at making them work with Jabber yesterday. Simple: well, we have a SIP CM. And does anyone use SILC? ☺)

> I realized that making them mandatory parameters is not correct, i.e. it will
> not work for users that need to leave one of the parameters empty. I'll submit
> another patch based on your re-base once I'm done.

Great, thanks.

> As for the style: have you considered adding at least some hints for Emacs to
> end of each file? Then the Emacs C mode will make sure that the code is entered
> using the correct style.

Yep, that's a good idea. I think the modelines for emacs and Vim should follow <http://telepathy.freedesktop.org/wiki/Style>, since that's the direction I'd like Haze to head.
Comment 5 Stefan Becker 2010-06-10 09:58:48 UTC
Created attachment 36204 [details] [review]
connection(-manager): PurpleAccountUserSplits should be optional parameters

Works OK with Empathy. I'm trying to test the whole shebang on my N900 ASAP.
Comment 6 Stefan Becker 2010-06-10 13:13:09 UTC
I've backported the patch series ontop of telepathy-haze_0.3.4-1maemo2 and installed the resulting .deb file on my N900. Works like a charm, i.e. I can now set up my Office Communicator account completely from the Account Setup UI.
Comment 7 Will Thompson 2010-06-11 02:02:54 UTC
Great! Merged; will be in haze 0.3.5. Thanks!


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.