> On Dec. 3, 2011, 1:12 p.m., Ruurd Pels wrote: > > kdeui/itemviews/kcategorizedview.cpp, line 1378 > > <http://git.reviewboard.kde.org/r/103313/diff/2/?file=42637#file42637line1378> > > > > Argh. Exit method halfway. I'd prefer reworking the trailing part of > > the function (refactoring a bit that is) instead of exiting halfway. Should > > be as easy as moving the rest of the function in the else clause AFAICS on > > short notice.
Normally I would say yes in short functions. But in this case, there is also another rule that says "do not do your main computation in the else part", and another one that says "express the if then else in positive". There is the main rule that says "make your code readable and maintainable", that I think wins here. By the way, there are two infinite loops with break; to exit from them to do binary search. You are free to create a patch for them. - Jaime Torres ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103313/#review8683 ----------------------------------------------------------- On Dec. 3, 2011, 10:55 a.m., Jaime Torres Amate wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103313/ > ----------------------------------------------------------- > > (Updated Dec. 3, 2011, 10:55 a.m.) > > > Review request for kdelibs. > > > Description > ------- > > Basically, what I do is: > If there are one or zero columns, hide the horizontalScrollBar until it is > needed. (it has worked in the past, but in another file). > Apply the same strategy with files. > > Additional stuff: > Moved the common calculus of itemSize outside of the if then else. > > > This addresses bugs 213068 and 287847. > http://bugs.kde.org/show_bug.cgi?id=213068 > http://bugs.kde.org/show_bug.cgi?id=287847 > > > Diffs > ----- > > kdeui/itemviews/kcategorizedview.cpp 46a1cde > kutils/kpluginselector.cpp ca0691d > > Diff: http://git.reviewboard.kde.org/r/103313/diff/diff > > > Testing > ------- > > Krunner config does not loop (neither kgetnewstuff from kstars). I can not > test with amarok (I've hit by an amarok start bug). > Please, test with other programs. > > > Thanks, > > Jaime Torres Amate > >
