Review Request: Use dialog-less setup for salut account

[Martin Klapetek]

> 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?

Yes, the whole "enable" UI is disabled if telepathy-salut is not found, see KCMTelepathyAccounts::onSalutConnectionManagerReady(...), this check is done from KCMTelepathyAccounts constructor.


> On Nov. 12, 2011, 8:09 p.m., Dario Freddi wrote:
> > src/salut-enabler.cpp, line 69
> > <http://git.reviewboard.kde.org/r/103117/diff/1/?file=40870#file40870line69>
> >
> >     Same remark as before - is this class guaranteed to be created only if salut is found?

Yep, the whole SalutEnabler won't be created until the UI is enabled and that is enabled only if salut is found (see the previous comment).


> On Nov. 12, 2011, 8:09 p.m., Dario Freddi wrote:
> > src/salut-enabler.cpp, line 195
> > <http://git.reviewboard.kde.org/r/103117/diff/1/?file=40870#file40870line195>
> >
> >     %1 and %2, watch out

Good catch.


> 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

What's wrong with it? Or, rather, what's the correct indentation with multiple line method call?


> On Nov. 12, 2011, 8:09 p.m., Dario Freddi wrote:
> > src/salut-enabler.cpp, line 263
> > <http://git.reviewboard.kde.org/r/103117/diff/1/?file=40870#file40870line263>
> >
> >     This should be i18n'ed or not using a magic string

In fact this shouldn't use 'Online' at all, removing.


> On Nov. 12, 2011, 8:09 p.m., Dario Freddi wrote:
> > src/salut-enabler.cpp, line 274
> > <http://git.reviewboard.kde.org/r/103117/diff/1/?file=40870#file40870line274>
> >
> >     I'm being picky here, but what about a better name for this variable? It looks like a bool when you first see it.

Yeah, you're right. detailsDialog it is.


> On Nov. 12, 2011, 8:09 p.m., Dario Freddi wrote:
> > src/salut-message-widget.cpp, lines 60-61
> > <http://git.reviewboard.kde.org/r/103117/diff/1/?file=40872#file40872line60>
> >
> >     Urgh, magic! :D If you can't put something which is obtained from an expression and you need these magic numbers, at least add a comment explaining why

Ok :P


> 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.

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.


- Martin


-----------------------------------------------------------
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/5c8fe547/attachment-0001.html>