----------------------------------------------------------- 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 > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
