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

[Thomas Richard]

> On Jan. 28, 2011, 6:27 p.m., George Kiagiadakis wrote:
> > Looks ok in general, although I'm not very familiar with the code.

Thanks for the review!


> On Jan. 28, 2011, 6:27 p.m., George Kiagiadakis wrote:
> > src/KCMTelepathyAccounts/feedback-widget.h, line 34
> > <http://git.reviewboard.kde.org/r/100455/diff/1/?file=7782#file7782line34>
> >
> >     Since you are not installing a header for this class, it shouldn't have KDE_EXPORT.

Ok, didn't know that


> On Jan. 28, 2011, 6:27 p.m., George Kiagiadakis wrote:
> > src/KCMTelepathyAccounts/parameter-edit-model.h, lines 76-77
> > <http://git.reviewboard.kde.org/r/100455/diff/1/?file=7785#file7785line76>
> >
> >     Is this signal actually connected somewhere? I can't find where.

account-edit-widget.cpp:85


> On Jan. 28, 2011, 6:27 p.m., George Kiagiadakis wrote:
> > src/KCMTelepathyAccounts/parameter-edit-model.cpp, line 135
> > <http://git.reviewboard.kde.org/r/100455/diff/1/?file=7786#file7786line135>
> >
> >     Why did you comment this out?

Because we're using a QDataWidgetMapper to map the parameters from the model to the edit widgets. When dataChanged is emitted, the value inside the edit widget is updated to the value that the model currently has. So while you're typing, your newly typed characters will disappear because they are updated to what's in the model again.
Just commenting this out might not be the right solution but I have no idea how to fix this otherwise. We're not using the valid state in the model to actually show anything.


> On Jan. 28, 2011, 6:27 p.m., George Kiagiadakis wrote:
> > src/KCMTelepathyAccounts/parameter-edit-model.cpp, line 229
> > <http://git.reviewboard.kde.org/r/100455/diff/1/?file=7786#file7786line229>
> >
> >     Don't use "" to specify empty strings. Use QString().

Will do


> On Jan. 28, 2011, 6:27 p.m., George Kiagiadakis wrote:
> > src/KCMTelepathyAccounts/validated-line-edit.cpp, line 108
> > <http://git.reviewboard.kde.org/r/100455/diff/1/?file=7788#file7788line108>
> >
> >     That should be QLatin1String instead of QString.

Will do


> On Jan. 28, 2011, 6:27 p.m., George Kiagiadakis wrote:
> > src/KCMTelepathyAccounts/validated-line-edit.cpp, lines 121-128
> > <http://git.reviewboard.kde.org/r/100455/diff/1/?file=7788#file7788line121>
> >
> >     This is getting a bit too long. Why not stick to the kdelibs coding style?
> >     
> >     switch(foo) {
> >     case Bar:
> >         statement();
> >         break:
> >     case Foo:
> >         break;
> >     }

Didn't know about that one. Will change it


> On Jan. 28, 2011, 6:27 p.m., George Kiagiadakis wrote:
> > src/add-account-assistant.cpp, line 208
> > <http://git.reviewboard.kde.org/r/100455/diff/1/?file=7790#file7790line208>
> >
> >     Don't use "" to specify empty strings. Use QString().

Will do


> On Jan. 28, 2011, 6:27 p.m., George Kiagiadakis wrote:
> > src/add-account-assistant.cpp, line 262
> > <http://git.reviewboard.kde.org/r/100455/diff/1/?file=7790#file7790line262>
> >
> >     This is too long for one line. Please try to keep it under 100 columns per line, as the kdelibs coding style suggests.

Will change that too


- Thomas


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


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/20110129/30927a2f/attachment-0001.htm