From kde-core-devel Sun Dec 04 08:30:08 2011 From: "Jaime Torres Amate" Date: Sun, 04 Dec 2011 08:30:08 +0000 To: kde-core-devel Subject: Re: Review Request: fix infinite recurssion in kcategorizedview Message-Id: <20111204083008.29800.5467 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=132298757304763 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============5989729986963032045==" --===============5989729986963032045== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > On Dec. 3, 2011, 1:12 p.m., Ruurd Pels wrote: > > kdeui/itemviews/kcategorizedview.cpp, line 1378 > > > > > > Argh. Exit method halfway. I'd prefer reworking the trailing part o= f the function (refactoring a bit that is) instead of exiting halfway. Shou= ld 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 als= o 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 i= s 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 d= o 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 i= s 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=3D213068 > http://bugs.kde.org/show_bug.cgi?id=3D287847 > = > = > 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 no= t test with amarok (I've hit by an amarok start bug). > Please, test with other programs. > = > = > Thanks, > = > Jaime Torres Amate > = > --===============5989729986963032045== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: http://git.revie= wboard.kde.org/r/103313/

On December 3rd, 2011, 1:12 p.m., Ruurd Pel= s wrote:

= = =
kdeui/itemviews/kcategorizedview.cpp (Diff revision 2) 
void KCategorizedView::rowsAboutToBeRemoved(const QModelIndex &=
parent,
 

    1376
    
        return;

  


 




  
Argh. Exi=
t method halfway. I'd prefer reworking the trailing part of the functio=
n (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 noti=
ce.

 









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 th=
e else part", and another one that says "express the if then else=
 in positive". There is the main rule that says "make your code r=
eadable and maintainable", that I think wins here.
By the way, there are two infinite loops with break; to exit from them to d=
o binary search. You are free to create a patch for them.








- Jaime Torres






On December 3rd, 2011, 10:55 a.m., Jaime Torres Amate wrote:









 

  


Review request for kdelibs.


By Jaime Torres Amate.




Updated Dec. 3, 2011, 10:55 a.m.








Descripti=
on 

 

 

  
   
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.

  		
 






Testing <=
/h1>


 

  
   
Krunner config does not loop (neither kgetnewstuff from ksta=
rs). I can not test with amarok (I've hit by an amarok start bug).
Please, test with other programs.

  		
 








 Bugs: 


 213068, =


 287847







Diffs=
 


    

 
  • kdeui/itemviews/kcategorizedview.cpp (46a1=
cde)
    • 

 
  • kutils/kpluginselector.cpp (ca0691d)
    • 





View Diff





  		
 











  

 



--===============5989729986963032045==--