Review Request: Make the accounts kcm use profiles instead of protocols

Thomas Richard thomas.richard at proan.be
Sat Jan 15 00:16:35 CET 2011 


> On Jan. 14, 2011, 9:19 p.m., David Edmundson wrote:
> > All the changes to the profile selection widget and use of the profilemanager look great. Except for a few minor review comments, mostly about how Tp-Qt4 works.
> > 
> > Your changes to the plugin interface I'm less convinced by.
> > 1) I think we've mixed up "profile" and "serviceName" here.
> > 
> > 2) The interfaces changes weren't what I was expecting:
> > I was imagining, 
> > 
> >     virtual AbstractAccountUi* accountUi(const QString &connectionManager, const QString &protocol); in AbstractAccountUiPlugin changing to have a third parameter called serviceName.
> > 
> > This then returns a different AbstractAccountUi per service which can be different classes. 
> > If a plugin wants to use the same AbstractAccountUi for all services and just make minor tweaks, this can be done at a plugin level. 
> >  The GabbleAccountUi class can take an extra parameter (serviceName), without the superclass changing.
> > 
> > This means we don't have to update /all/ the plugins with a parameter that they don't use.
> > Especially as I think gabble is going to be the only one with multiple services.
> > 
> >

1) I was doubting about that one but you're right. I'll change those.

2) Sounds good to me. I updated all plugins already though. (was done in like 5 minutes). Shall i revert those changes and edit AbstractAccountUiPlugin instead?


> On Jan. 14, 2011, 9:19 p.m., David Edmundson wrote:
> > src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp, line 119
> > <http://git.reviewboard.kde.org/r/100389/diff/1/?file=6973#file6973line119>
> >
> >     whilst I agree we should have a debug line here, it's not really a warning. 
> >     
> >     Having 'missing' parameters is sort of expected if a user is using an older version of the connection manager than the newest connection manager we support. It's not really a problem because we handle it.

I'd suggest we leave it there until we make a release. I already found a parameter that get's hidden in the Gabble plugin that shouldn't be hidden. The other option would be to write an automated test case..


> On Jan. 14, 2011, 9:19 p.m., David Edmundson wrote:
> > src/add-account-assistant.cpp, line 196
> > <http://git.reviewboard.kde.org/r/100389/diff/1/?file=7003#file7003line196>
> >
> >     don't do this. We have nice APIs so that we don't have to write dbus paths ourselves.
> >     
> >     account->setSericeName();
> >     account->setEnabled

setServiceName and setEnabled are both pending operation. Can i do them at once? Otherwise it'd be chain of pending set operations. Doesn't look any neater to me.


> On Jan. 14, 2011, 9:19 p.m., David Edmundson wrote:
> > src/edit-account-dialog.cpp, line 73
> > <http://git.reviewboard.kde.org/r/100389/diff/1/?file=7004#file7004line73>
> >
> >     don't write ".data()->"
> >     
> >     AccountPtr has overloaded the "->" operator, so you only need to
> >     
> >     d->item->account()->profile();

Good to know. Thanks


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100389/#review888
-----------------------------------------------------------


On Jan. 14, 2011, 7:59 p.m., Thomas Richard wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100389/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2011, 7:59 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This change makes the accounts kcm use profiles. In the near future, distros should start shipping profiles. 
> 
> This will make it possible to show 'Google talk' and 'Facebook chat' in the new accounts dialog instead of 'Jabber/XMPP/Google Talk'. 
> It also allows to do small modifications in the plugins, depending on the selected profile. 
> It can provide different default parameters for different profiles.
> Last but not least, the icons used can (or must) be specified in the profile.
> 
> 
> Diffs
> -----
> 
>   src/KCMTelepathyAccounts/CMakeLists.txt 1e3a282f64bc66a18765a0499492296c222e94dd 
>   src/KCMTelepathyAccounts/abstract-account-parameters-widget.h 6071de379f1733b4428f4f3363a664993a3e0e15 
>   src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp fb29cc3fc95c3543bd2dda66d474a7ebba19c94a 
>   src/KCMTelepathyAccounts/abstract-account-ui.h a86ce6945150b5c72683ec31ca953b0128c0d3c5 
>   src/KCMTelepathyAccounts/abstract-account-ui.cpp 500452b7dc85535483f8eecce0b99eed5c582132 
>   src/KCMTelepathyAccounts/account-edit-widget.h 95e29238ffd83530a6005c290c1e3e8ed00d87c0 
>   src/KCMTelepathyAccounts/account-edit-widget.cpp 224dab9e40c282b20db69287f8c59a4c04b84d07 
>   src/KCMTelepathyAccounts/connection-manager-item.h aa3480eeecff4be8433d5a11236c40f1ffbbbe16 
>   src/KCMTelepathyAccounts/connection-manager-item.cpp 91eea62bd4e0fbf2a746424d99a025ef4d4261d3 
>   src/KCMTelepathyAccounts/dictionary.cpp 2eeff58ee489fe1b95c46d9c53e83ce42afc0ae4 
>   src/KCMTelepathyAccounts/generic-advanced-options-widget.h ac5ff029ba8e86e0adf6f6a3988609d5c1ae6665 
>   src/KCMTelepathyAccounts/generic-advanced-options-widget.cpp 0e39b47fbd214ef66b3af1732bf7dc0c1858a8c4 
>   src/KCMTelepathyAccounts/parameter-edit-model.h 44c9d4dcc5300f0734c2c7aa12e4d0d050b48aec 
>   src/KCMTelepathyAccounts/parameter-edit-model.cpp d64fea0532f0b3a615ee69aa3236d32582c1a839 
>   src/KCMTelepathyAccounts/parameter-edit-widget.cpp 34e161a48c63c02b35f4660af9826e9a09387051 
>   src/KCMTelepathyAccounts/parameter-item.cpp 65e4334de625804c3277424dbca28b5f2d4dbf28 
>   src/KCMTelepathyAccounts/profile-item.h PRE-CREATION 
>   src/KCMTelepathyAccounts/profile-item.cpp PRE-CREATION 
>   src/KCMTelepathyAccounts/profile-list-model.h PRE-CREATION 
>   src/KCMTelepathyAccounts/profile-list-model.cpp PRE-CREATION 
>   src/KCMTelepathyAccounts/profile-select-widget.h PRE-CREATION 
>   src/KCMTelepathyAccounts/profile-select-widget.cpp PRE-CREATION 
>   src/KCMTelepathyAccounts/profile-select-widget.ui PRE-CREATION 
>   src/KCMTelepathyAccounts/protocol-item.h 759956e11e05e57ccafb2baae3314816b9122b49 
>   src/KCMTelepathyAccounts/protocol-item.cpp 2a6844349ee8e16b4e8c6a9d274c77d51ffb69fb 
>   src/KCMTelepathyAccounts/protocol-list-model.h 3718e8fc0b4093ab70e4528efecdf3538abb0825 
>   src/KCMTelepathyAccounts/protocol-list-model.cpp 168c6c57c2db993a1d71f56678e2fbaa061aa953 
>   src/KCMTelepathyAccounts/protocol-select-widget.h ca0bd8a8468a76b89f24b477c050c39acba6aa0b 
>   src/KCMTelepathyAccounts/protocol-select-widget.cpp f53c5b7529f288efec0b9c1bd232d34b6b380a60 
>   src/KCMTelepathyAccounts/protocol-select-widget.ui d6aee67d461c574f139ebf74908acaecd193a0e2 
>   src/account-item.cpp a670a1be35bd2edb71683f918756de9edb91a1c8 
>   src/add-account-assistant.h b16d76b14ff5d871287610ce0be400188f84d3f8 
>   src/add-account-assistant.cpp 69edc1171798f80ee5ac8c5c9853907ff039d76e 
>   src/edit-account-dialog.cpp 979742196c880f34a5df3345b509ffb945328613 
>   src/kcm-telepathy-accounts.cpp 554f6d1d719cca651f86a7df6e9700970552cec6 
> 
> Diff: http://git.reviewboard.kde.org/r/100389/diff
> 
> 
> Testing
> -------
> 
> Compiling
> Adding new accounts
> Editing accounts
> Added the sample profile from:
> http://telepathy.freedesktop.org/wiki/service-profile-v1
> Everything seems to work as seen in the screenshots
> 
> 
> Screenshots
> -----------
> 
> 
>   http://git.reviewboard.kde.org/r/100389/s/43/
> 
>   http://git.reviewboard.kde.org/r/100389/s/44/
> 
>   http://git.reviewboard.kde.org/r/100389/s/45/
> 
> 
> Thanks,
> 
> Thomas
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110114/b0616528/attachment-0001.htm

More information about the KDE-Telepathy mailing list