Review Request: check in AddAccountAssistant for installed connection managers and optional install them

[Daniele E. Domenichelli]

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


Great stuff! I love it!
A general comment... I'd rather have the message in the main window (too?), rather than in the add-account dialog.


src/KCMTelepathyAccounts/CMakeLists.txt
<http://git.reviewboard.kde.org/r/107995/#comment19039>

    Does the option() command allow you to set a value different from ON and OFF? It doesn't look so from the "cmake --help-command option" output, so perhaps it works if you set it from the command line, but you won't be able to set it from the GUI. Even worse if you set it to TRUE it gives you a FATAL_ERROR
    
    I'd rather have an option ON/OFF WITH_PACKAGE_INSTALLATION and a string variable PACKAGE_INSTALLATION_BACKEND that appears only if the first one is true...
    
    Also adding a further -DWITH_PACKAGE_INSTALLATION define allows to avoid the "#ifdefined ... || ..." it will make things easier if we should ever support another package-manager or if packagers want to add theirs.



src/KCMTelepathyAccounts/package-install-action.cpp
<http://git.reviewboard.kde.org/r/107995/#comment19041>

    Perhaps those should be configurable from cmake, packagers might be using their own names?



src/KCMTelepathyAccounts/package-install-action.cpp
<http://git.reviewboard.kde.org/r/107995/#comment19040>

    Perhaps this should be a full path and maybe even a configurable cmake variable, packagers might have that installed in some weird place?



src/KCMTelepathyAccounts/package-install-action.cpp
<http://git.reviewboard.kde.org/r/107995/#comment19042>

    You should give a feedback to the user so that he knows that he has to log out from the desktop session and login again. Perhaps you could just prompt a message-box "To use the newly installed packages, at the end of the installation you should ..."



src/KCMTelepathyAccounts/simple-profile-select-widget.cpp
<http://git.reviewboard.kde.org/r/107995/#comment19043>

    I think you should delay the creation of the profileListModel right after the creation of the UI and before this connect, otherwise if the model is ready before the ui, the connect won't be called



src/KCMTelepathyAccounts/simple-profile-select-widget.cpp
<http://git.reviewboard.kde.org/r/107995/#comment19044>

    Perhaps, since you are installing them, you should show the message also if salut or rakia are not found


- Daniele E. Domenichelli


On Jan. 5, 2013, 1:13 p.m., Florian Reinhard wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107995/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2013, 1:13 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Check in SimpleProfileSelectWidget if required connection managers are installed
>     
> * new cmake option -DPACKAGE_INSTALLATION=[apt,packagekit]
> * default: no package installation enabled just promt a message
> * on the first page of AddAccountAssistant profiles with no CM installed will be disabled
> 
> 
> Diffs
> -----
> 
>   src/KCMTelepathyAccounts/CMakeLists.txt e19f3accf465f852012d251f69bbfa2ff372bb7f 
>   src/KCMTelepathyAccounts/package-install-action.h PRE-CREATION 
>   src/KCMTelepathyAccounts/package-install-action.cpp PRE-CREATION 
>   src/KCMTelepathyAccounts/profile-list-model.h 8b313d19765b63071408047b5d183ecc419500de 
>   src/KCMTelepathyAccounts/profile-list-model.cpp 12752928f377552a323a502557d2717819c257de 
>   src/KCMTelepathyAccounts/simple-profile-select-widget.h 52eede1c4d5b7a39143b71c19f33dcb965827bf9 
>   src/KCMTelepathyAccounts/simple-profile-select-widget.cpp 5f9a516e74a17c8cf86a5f5382d5071baff1c8a8 
>   src/KCMTelepathyAccounts/simple-profile-select-widget.ui 5ad8e79a7bd4fd6d52dac61cbd88becb0cd569ae 
> 
> Diff: http://git.reviewboard.kde.org/r/107995/diff/
> 
> 
> Testing
> -------
> 
> * remove telepathy-haze
> * start the kcm
> * result see screenshot
> 
> 
> Screenshots
> -----------
> 
> telepathy-haze not installed
>   http://git.reviewboard.kde.org/r/107995/s/937/
> telepathy-haze
>   http://git.reviewboard.kde.org/r/107995/s/978/
> 
> 
> Thanks,
> 
> Florian Reinhard
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130105/9900633b/attachment-0001.html>