> 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. > > Martin Klapetek wrote: > Then we should find the cause of crash fix that instead of the > consequences.
The cause was m_busyOverlay(0) in constructor. Unreferencing null-pointer. I added this check and placed overlay allocation into constructor's initialization. But we'd rather leave this check. Who knows, maybe later some student will disable overlay by making delete m_busyOverlay. The world is a strange place. > 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. > > Martin Klapetek wrote: > Code readability is subjective, I find it more readable with the if > (!editable). And CPUs are optimizing those logical forkings good enough for > us ;) I'm tired of this arguing. Updated diff with all your suggestions accepted. - Roman ----------------------------------------------------------- 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 > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
