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

David Edmundson kde at davidedmundson.co.uk
Fri Jan 14 22:19:51 CET 2011 

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


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.




src/KCMTelepathyAccounts/abstract-account-parameters-widget.h
<http://git.reviewboard.kde.org/r/100389/#comment718>

    It's not really a profile you're passing, 
    a profile is a set of:
     connectionManager, protocol and serviceName. I would say this is a serviceName.
    
    Unfortunately that's quite a big change...



src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp
<http://git.reviewboard.kde.org/r/100389/#comment729>

    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.



src/KCMTelepathyAccounts/account-edit-widget.cpp
<http://git.reviewboard.kde.org/r/100389/#comment730>

    you don't need .data() here.



src/add-account-assistant.cpp
<http://git.reviewboard.kde.org/r/100389/#comment731>

    don't do this. We have nice APIs so that we don't have to write dbus paths ourselves.
    
    account->setSericeName();
    account->setEnabled



src/edit-account-dialog.cpp
<http://git.reviewboard.kde.org/r/100389/#comment721>

    don't write ".data()->"
    
    AccountPtr has overloaded the "->" operator, so you only need to
    
    d->item->account()->profile();


- David


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/8bc09b56/attachment-0001.htm

More information about the KDE-Telepathy mailing list