From kde-core-devel Wed Jul 27 23:45:51 2011 From: "Hugo Pereira Da Costa" Date: Wed, 27 Jul 2011 23:45:51 +0000 To: kde-core-devel Subject: Re: Review Request: Fix logic with clear-button animation in klineedit Message-Id: <20110727234551.16152.52532 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=131181040619823 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============2028749043069252696==" --===============2028749043069252696== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > On July 26, 2011, 10:46 p.m., Aleix Pol Gonzalez wrote: > > kdeui/widgets/klineedit.cpp, line 358 > > > > > > Wouldn't it be better to put it this way? Just saying... > > = > > d->clearButton->animateVisible(d->wideEnoughForClear && text.length= () > 0); > = > Nicolas Alvarez wrote: > I think the original is clearer, to be honest. I agree with Nicolas. I have nothing against putting boolean test inside the method call, *in pri= nciple*, but I believe it's convenient only if the boolean condition is "sh= ort enough" to write. = - Hugo ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102095/#review5129 ----------------------------------------------------------- On July 26, 2011, 9:54 p.m., Hugo Pereira Da Costa wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102095/ > ----------------------------------------------------------- > = > (Updated July 26, 2011, 9:54 p.m.) > = > = > Review request for kdelibs. > = > = > Summary > ------- > = > Details: > - fixes the somewhat incorrect logic in KLineEditButton::animateVisible > - simplifies KLineEdit::updateClearButtonIcon consequently. > = > = > This addresses bug 268898. > http://bugs.kde.org/show_bug.cgi?id=3D268898 > = > = > Diffs > ----- > = > kdeui/widgets/klineedit.cpp 8f1c8a4 = > kdeui/widgets/klineedit_p.h 95016bd = > = > Diff: http://git.reviewboard.kde.org/r/102095/diff > = > = > Testing > ------- > = > tested with klineedittest found in kdelibs/kdeui/tests, this with and wit= hout the patch attached to comment #1 of bug 268898, used to actually trigg= er the mentionned bug. Also tested with other klineEdit implementation such= as Dolphin's location bar. > = > = > Thanks, > = > Hugo > = > --===============2028749043069252696== 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/102095/

On July 26th, 2011, 10:46 p.m., Aleix Pol G= onzalez wrote:

= = =
kdeui/widgets/klineedit.cpp (Diff revision 1) 
void KLineEdit::updateClearButtonIcon(const QString& text)
 

    349
    
    if (d->wideEnoughForClear && text.leng=
th() > 0) {

  


 




  
Wouldn=
9;t it be better to put it this way? Just saying...

d->clearButton->animateVisible(d->wideEnoughForClear && te=
xt.length() > 0);

 




 
On July 27th, 2011, 10:52 p.m., Nicolas Alvarez wrote:

 

  
I think t=
he original is clearer, to be honest.

 











I agree with Nicolas.
I have nothing against putting boolean test inside the method call, *in pri=
nciple*, but I believe it's convenient only if the boolean condition is=
 "short enough" to write. 








- Hugo






On July 26th, 2011, 9:54 p.m., Hugo Pereira Da Costa wrote:









 

  


Review request for kdelibs.


By Hugo Pereira Da Costa.




Updated July 26, 2011, 9:54 p.m.






Descripti=
on 



 

  
   
Details:
- fixes the somewhat incorrect logic in KLineEditButton::animateVisible
- simplifies KLineEdit::updateClearButtonIcon consequently.


  		
 






Testing <=
/h1>


 

  
   
tested with klineedittest found in kdelibs/kdeui/tests, this=
 with and without the patch attached to comment #1 of bug 268898, used to a=
ctually trigger the mentionned bug. Also tested with other klineEdit implem=
entation such as Dolphin's location bar.

  		
 








 Bugs: 


 268898







Diffs=
 


    

 
  • kdeui/widgets/klineedit.cpp (8f1c8a4)
    • 

 
  • kdeui/widgets/klineedit_p.h (95016bd)
    • 





View Diff





  		
 











  

 



--===============2028749043069252696==--