Review Request: Give user visible feedback when creating/editing accounts

[George Kiagiadakis]

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


Looks ok in general, although I'm not very familiar with the code.


src/KCMTelepathyAccounts/feedback-widget.h
<http://git.reviewboard.kde.org/r/100455/#comment924>

    Since you are not installing a header for this class, it shouldn't have KDE_EXPORT.



src/KCMTelepathyAccounts/feedback-widget.cpp
<http://git.reviewboard.kde.org/r/100455/#comment920>

    Why is that commented?
    Also, the call to hide() doesn't make sense to me. Widgets are hidden by default until you show them, right?
    



src/KCMTelepathyAccounts/parameter-edit-model.h
<http://git.reviewboard.kde.org/r/100455/#comment926>

    Is this signal actually connected somewhere? I can't find where.



src/KCMTelepathyAccounts/parameter-edit-model.cpp
<http://git.reviewboard.kde.org/r/100455/#comment917>

    Why did you comment this out?



src/KCMTelepathyAccounts/parameter-edit-model.cpp
<http://git.reviewboard.kde.org/r/100455/#comment918>

    Don't use "" to specify empty strings. Use QString().



src/KCMTelepathyAccounts/validated-line-edit.cpp
<http://git.reviewboard.kde.org/r/100455/#comment925>

    That should be QLatin1String instead of QString.



src/KCMTelepathyAccounts/validated-line-edit.cpp
<http://git.reviewboard.kde.org/r/100455/#comment919>

    This is getting a bit too long. Why not stick to the kdelibs coding style?
    
    switch(foo) {
    case Bar:
        statement();
        break:
    case Foo:
        break;
    }



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

    Don't use "" to specify empty strings. Use QString().



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

    This is too long for one line. Please try to keep it under 100 columns per line, as the kdelibs coding style suggests.


- George


On Jan. 28, 2011, 5:47 p.m., Thomas Richard wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100455/
> -----------------------------------------------------------
> 
> (Updated Jan. 28, 2011, 5:47 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This patch will give some visible feedback in a non obtrusive way. There is also the possibility to provide some custom validation on parameters using the ParameterEditModel.
> 
> The screenshots speak a 1000 words ;)
> 
> 
> Diffs
> -----
> 
>   src/KCMTelepathyAccounts/CMakeLists.txt ce768604d2a2666695dde8b741972dd11023b5c4 
>   src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp 95091e67b83f4aaca72603945610ba01f94b9304 
>   src/KCMTelepathyAccounts/account-edit-widget.h 42dcd92890be22012a1dc7424e501b9718eead63 
>   src/KCMTelepathyAccounts/account-edit-widget.cpp a6673c955d10ee364018462b9506ed9ba164b101 
>   src/KCMTelepathyAccounts/feedback-widget.h PRE-CREATION 
>   src/KCMTelepathyAccounts/feedback-widget.cpp PRE-CREATION 
>   src/KCMTelepathyAccounts/include/ValidatedLineEdit PRE-CREATION 
>   src/KCMTelepathyAccounts/parameter-edit-model.h 2ca58c228447caae08785db5ca3b1460cf89dbe5 
>   src/KCMTelepathyAccounts/parameter-edit-model.cpp 5ee3a568de45260ff544a28f7fee135d200fe7fa 
>   src/KCMTelepathyAccounts/validated-line-edit.h PRE-CREATION 
>   src/KCMTelepathyAccounts/validated-line-edit.cpp PRE-CREATION 
>   src/add-account-assistant.h 973bad46e11697135c331c632e9cb63dcb790232 
>   src/add-account-assistant.cpp b7f38b32fe792e049ae5fd57b88d4403e709dfaf 
> 
> Diff: http://git.reviewboard.kde.org/r/100455/diff
> 
> 
> Testing
> -------
> 
> When all parameters are valid, an account still gets added
> 
> 
> Screenshots
> -----------
> 
> Valid email address
>   http://git.reviewboard.kde.org/r/100455/s/56/
> Invalid email address
>   http://git.reviewboard.kde.org/r/100455/s/57/
> Invalid email address and clicking apply
>   http://git.reviewboard.kde.org/r/100455/s/58/
> When an account did not get accepted by telepathy itself
>   http://git.reviewboard.kde.org/r/100455/s/59/
> 
> 
> Thanks,
> 
> Thomas
> 
>

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