Review Request 109262: busyOverlay now hides if presence chooser is editable.

[Martin Klapetek]

> On March 3, 2013, 5:05 p.m., Martin Klapetek wrote:
> > global-presence-chooser.cpp, line 303
> > <http://git.reviewboard.kde.org/r/109262/diff/2/?file=116716#file116716line303>
> >
> >     You don't need this check as this method won't be ever called before the constructor, which constructs this widget. But won't hurt either.
> 
> Roman Nazarenko wrote:
>     It was crashing once because of that check miss, so we'd rather leave it so. CPUs are optimizing those logical forkings good enough for us to leave those things.

Then we should find the cause of crash fix that instead of the consequences.


> On March 3, 2013, 5:05 p.m., Martin Klapetek wrote:
> > global-presence-chooser.cpp, lines 305-307
> > <http://git.reviewboard.kde.org/r/109262/diff/2/?file=116716#file116716line305>
> >
> >     This needs to be called only when editable is false; also please add a comment that we're restarting the spinner if it was spinning before
> 
> Roman Nazarenko wrote:
>     If it's false, overlay's widget will be set to NULL, and start() function will exit anyway - it checks widget==null in the first line, so it's not a really a big deal. 
>     I leaved it so because adding if (editable) will add us third nesting level, what leads to poor readability. May add a comment about it in the sources though.

Code readability is subjective, I find it more readable with the if (!editable). And CPUs are optimizing those logical forkings good enough for us ;)


- Martin


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


On March 3, 2013, 4:55 p.m., Roman Nazarenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109262/
> -----------------------------------------------------------
> 
> (Updated March 3, 2013, 4:55 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> busyOverlay now hides if presence chooser is editable.
> 
> 
> This addresses bug 292282.
>     http://bugs.kde.org/show_bug.cgi?id=292282
> 
> 
> Diffs
> -----
> 
>   global-presence-chooser.h d7a19c4 
>   global-presence-chooser.cpp 9e7994f 
> 
> Diff: http://git.reviewboard.kde.org/r/109262/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Roman Nazarenko
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130303/007a5160/attachment-0001.html>