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

[Thomas Richard]

> On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote:
> > src/KCMTelepathyAccounts/feedback-widget.cpp, line 74
> > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8312#file8312line74>
> >
> >     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());
> >

I agree on this one. The code was copied from the bluetooth dialog, didn't really check it.


> On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote:
> > src/KCMTelepathyAccounts/feedback-widget.cpp, line 88
> > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8312#file8312line88>
> >
> >     coding convention is normally
> >     
> >     case Foo:
> >       myStuff();
> >       break;

Fixed


> On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote:
> > src/KCMTelepathyAccounts/feedback-widget.cpp, line 92
> > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8312#file8312line92>
> >
> >     Does KColorScheme/QPallete have a better way of selected a warning color?

Seems like there is a color for that:
"NeutralBackground : Seventh color; for example, warnings, secure/encrypted content."
The yellow for warning seems ok.

I can't really find one for InfoMessage though


> On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote:
> > src/KCMTelepathyAccounts/feedback-widget.cpp, line 97
> > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8312#file8312line97>
> >
> >     I don't understand what this is achieving. 
> >     
> >     
> >     Possibly a side effect of the rect comment above.

I think you're right. This was copied as well. When using rect(), this doesn't seem to have any effect. 


> On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote:
> > src/KCMTelepathyAccounts/validated-line-edit.cpp, line 163
> > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8317#file8317line163>
> >
> >     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.

Why would that be better? I assume that textboxes are not equally large in every style/with every font. You'd still need to convert them to a pixmap


> On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote:
> > src/KCMTelepathyAccounts/validated-line-edit.cpp, line 176
> > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8317#file8317line176>
> >
> >     there's little point writing
> >     
> >     this->doSomething();
> >     when you can write 
> >     
> >     doSomething();

Agreed. I usually do this because i think it reads better. Anyway, will remove it.


> On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote:
> > src/add-account-assistant.cpp, line 204
> > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8319#file8319line204>
> >
> >     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".

With this message people can at least try to find help. "Internal error" says nothing at all, not even what went wrong.


> On Feb. 7, 2011, 11:04 a.m., David Edmundson wrote:
> > src/edit-account-dialog.cpp, line 91
> > <http://git.reviewboard.kde.org/r/100455/diff/3/?file=8320#file8320line91>
> >
> >     Why does this need moving?
> >     
> >

I can't really remember but I'm pretty sure it had a reason. I think it had something to with making sure that data is submitted to the model before validating.


- Thomas


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


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/20110324/6351c006/attachment-0001.htm