Review Request: Use dialog-less setup for salut account

[Dario Freddi]

> On Nov. 12, 2011, 8:09 p.m., Dario Freddi wrote:
> > src/salut-details-dialog.cpp, lines 64-74
> > <http://git.reviewboard.kde.org/r/103117/diff/1/?file=40866#file40866line64>
> >
> >     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?
> 
> Martin Klapetek wrote:
>     Yes, the whole "enable" UI is disabled if telepathy-salut is not found, see KCMTelepathyAccounts::onSalutConnectionManagerReady(...), this check is done from KCMTelepathyAccounts constructor.

Perfect


> On Nov. 12, 2011, 8:09 p.m., Dario Freddi wrote:
> > src/salut-enabler.cpp, lines 246-248
> > <http://git.reviewboard.kde.org/r/103117/diff/1/?file=40870#file40870line246>
> >
> >     Mind the indentation
> 
> Martin Klapetek wrote:
>     What's wrong with it? Or, rather, what's the correct indentation with multiple line method call?

Maybe is reviewboard screwing up, but it appears as if Q_EMIT is indented 12 spaces after the bracket. The multiple line indentation is perfect as it is


> On Nov. 12, 2011, 8:09 p.m., Dario Freddi wrote:
> > src/salut-message-widget.cpp, lines 72-95
> > <http://git.reviewboard.kde.org/r/103117/diff/1/?file=40872#file40872line72>
> >
> >     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.
> 
> Martin Klapetek wrote:
>     I don't really understand this one..anyway, since you have to input first and last name separated for salut, I thought letting the user know what was actually used as the first and last name is better for the user. That's why it's done in this way.

The fact you have to input them separately is just an implementation detail the average user will never know: consider this UI is meant to be a self-configuration for users who don't care about these details. Your question should be: what is the user interested in in this case? My answer would be: "knowing how I will appear in a contact list".

For this reason I proposed a single format for the name - while I agree with you that salut works differently than having a single string with all the infos together, the user will not care about that, besides for the final result. The implementation of the underlying protocol should not prevent you from using a simpler approach, especially because the user should still be able to configure first, last and nick separately if he feels like doing so - in that case, of course the implementation matters.

Besides that, changing "okay?" with something slightly more professional still remains a good idea to me :P


- Dario


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


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/20111113/677d740a/attachment-0001.html>