Review Request: Use dialog-less setup for salut account

[Dario Freddi]

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


Good start. Some issues and bitching below.


src/kcm-telepathy-accounts.cpp
<http://git.reviewboard.kde.org/r/103117/#comment7005>

    Hmm. Are you 100% sure everytime you'll reach this stage m_salutEnable won't carry a valid object? I would wrap m_salutEnabler in a QWeakPointer, check if m_salutEnabler != 0 and eventually deleteLater() it before creating a new one.



src/kcm-telepathy-accounts.cpp
<http://git.reviewboard.kde.org/r/103117/#comment7006>

    Thanks :)



src/kcm-telepathy-accounts.cpp
<http://git.reviewboard.kde.org/r/103117/#comment7007>

    As said above, here you should indeed deleteLater() it, and wrap it in a QWeakPointer



src/salut-details-dialog.cpp
<http://git.reviewboard.kde.org/r/103117/#comment6989>

    You are missing constness in the first two arguments



src/salut-details-dialog.cpp
<http://git.reviewboard.kde.org/r/103117/#comment6990>

    This part would obviously assert if salut is not on the user's PC. Is this whole dialog guaranteed to appear only if salut is found?



src/salut-enabler.cpp
<http://git.reviewboard.kde.org/r/103117/#comment6991>

    Missing constness



src/salut-enabler.cpp
<http://git.reviewboard.kde.org/r/103117/#comment6992>

    Same remark as before - is this class guaranteed to be created only if salut is found?



src/salut-enabler.cpp
<http://git.reviewboard.kde.org/r/103117/#comment6993>

    Mind the code style: if () {



src/salut-enabler.cpp
<http://git.reviewboard.kde.org/r/103117/#comment6994>

    Here you should use the defines from tp-qt4, to avoid magic strings.



src/salut-enabler.cpp
<http://git.reviewboard.kde.org/r/103117/#comment6995>

    %1 and %2, watch out



src/salut-enabler.cpp
<http://git.reviewboard.kde.org/r/103117/#comment6997>

    Mind the indentation



src/salut-enabler.cpp
<http://git.reviewboard.kde.org/r/103117/#comment6998>

    This should be i18n'ed or not using a magic string



src/salut-enabler.cpp
<http://git.reviewboard.kde.org/r/103117/#comment6999>

    Not a very good idea. You want to make salutMessageFrame a QWeakPointer and deleteLater() it, so that the 0 magic is already handled for you and the deletion happens the right way



src/salut-enabler.cpp
<http://git.reviewboard.kde.org/r/103117/#comment7000>

    I'm being picky here, but what about a better name for this variable? It looks like a bool when you first see it.



src/salut-enabler.cpp
<http://git.reviewboard.kde.org/r/103117/#comment7001>

    Normalize the connect declaration:
    
    connect(d->enableDialog, SIGNAL(dialogAccepted(QVariantMap)),
                this, SLOT(onDialogAccepted(QVariantMap)));



src/salut-message-widget.cpp
<http://git.reviewboard.kde.org/r/103117/#comment7002>

    These should possibly be KAction



src/salut-message-widget.cpp
<http://git.reviewboard.kde.org/r/103117/#comment7003>

    Urgh, magic! :D If you can't put something which is obtained from an expression and you need these magic numbers, at least add a comment explaining why



src/salut-message-widget.cpp
<http://git.reviewboard.kde.org/r/103117/#comment7004>

    Tbh, I'm not a real fan of these strings. First of all, I would keep a single string where the set name always has the format "First Last (Nick)", becoming "Nick" if no names are available and removing the parenthesis if nick is not available.
    
    The string could be something like "Your local network account will be configured as %1, do you want to change that?" or something similar. Maybe David can give a better insight here.


- Dario Freddi


On Nov. 12, 2011, 12:22 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103117/
> -----------------------------------------------------------
> 
> (Updated Nov. 12, 2011, 12:22 p.m.)
> 
> 
> Review request for Telepathy and Dario Freddi.
> 
> 
> Description
> -------
> 
> This tries to automate the setup of salut account. It takes first and last name from KUser, splits them by the last space (the issues have been already discussed) and displays a KMessageWidget (that's why the kdelibs min version bump to 4.7). This auto-closes after 8 secs, displaying a circular countdown. User still has a choice to modify the data by using the dialog.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 3690e31 
>   src/CMakeLists.txt 67de5e1 
>   src/kcm-telepathy-accounts.h 1e8d92b 
>   src/kcm-telepathy-accounts.cpp 23420fa 
>   src/main-widget.ui f333271 
>   src/salut-details-dialog.h PRE-CREATION 
>   src/salut-details-dialog.cpp PRE-CREATION 
>   src/salut-enable-dialog.h ca15d59 
>   src/salut-enable-dialog.cpp d1333a4 
>   src/salut-enabler.h PRE-CREATION 
>   src/salut-enabler.cpp PRE-CREATION 
>   src/salut-message-widget.h PRE-CREATION 
>   src/salut-message-widget.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103117/diff/diff
> 
> 
> Testing
> -------
> 
> Works.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

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