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

[David Edmundson]

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


Still a few minor comments, but that's to be expected with such a large patch.

I suggest we send an email to KDE usability when merged, just to show that we've listened. I imagine they'll like that.


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

    Not sure this is right. event->rect() is the area to be repainted not always the whole area of the widget, if you redrew half of it, all your rounded corners would be wrong.
    
    Just 
    r = rect(); would get the widget area.
    
    then you can do
    
    event->rect() is normally used in:
    p.setClippingRect(event->rect());
    



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

    coding convention is normally
    
    case Foo:
      myStuff();
      break;



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

    Does KColorScheme/QPallete have a better way of selected a warning color?



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

    I don't understand what this is achieving. 
    
    
    Possibly a side effect of the rect comment above.



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

    If you stored KIcons instead of QPixmaps I think you wouldn't need to do resizing, you could just export to the correctly sized pixmap to start with.



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

    there's little point writing
    
    this->doSomething();
    when you can write 
    
    doSomething();



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

    Not sure I like this message. It doesn't say how to proceed.
    
    A user should never see the name telepathy. Not that they should ever see this message.
    
    Maybe something more generic on the lines of "internal error, check your system setup". 



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

    Why does this need moving?
    
    


- David


On Feb. 3, 2011, 7:41 p.m., Thomas Richard wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100455/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2011, 7:41 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 51389226299db38012f7f326b3e57709f4d3e244 
>   src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp 9f5b0d4f9b27c0502783895ee0f52e320829ffc3 
>   src/KCMTelepathyAccounts/account-edit-widget.h 11d80778e4ed0fc7f80943d2248d3f08b27feb20 
>   src/KCMTelepathyAccounts/account-edit-widget.cpp 5c09e5dade33ae383d63827add757c8688c24fb7 
>   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 d904facc2c958440d193fe86182bd50c0ee6721f 
>   src/KCMTelepathyAccounts/parameter-edit-model.cpp 441b6e58daf13b3fcf65ab344544ae36e50b940b 
>   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 eb9a644d38b0020ce3158c99f3714dddd1b8047a 
>   src/edit-account-dialog.cpp bcfaca26ec9fbf8385b4d43d2c673319c8bdc9a9 
> 
> 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/20110207/979d9eba/attachment-0001.htm