--===============2442078204265621504== Content-Type: multipart/alternative; boundary="===============2976073055457416244==" --===============2976073055457416244== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit > On Aug. 31, 2013, 12:14 p.m., Hugo Pereira Da Costa wrote: > > kstyles/oxygen/oxygenstyle.cpp, line 612 > > > > > > Rather than looking up the property multiple times, alongside the q_object_cast, I would rather add the test just before the last 'if' (when everything else has failed), and possibly caching the property in a QString. > > > > Something like > > > > else if( !widget && option && option->styleObject ) { > > const QString elementType = option->styleObject->property( elementType ); > > > > ... your code (all ifs, duplicated from the widget case) > > > > } else { > > > > return 1; > > > > } > > > > This way, you don't lookup the property if the widget is defined, > > and You don't lookup the property multiple times. > > This comes at the price of some code duplication, but it is a small block of code, so I'd deem that acceptable. > > > > Oppinion ? Makes sense. Will change. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112375/#review39005 ----------------------------------------------------------- On Aug. 30, 2013, 3:56 p.m., David Edmundson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/112375/ > ----------------------------------------------------------- > > (Updated Aug. 30, 2013, 3:56 p.m.) > > > Review request for Plasma and Hugo Pereira Da Costa. > > > Description > ------- > > Correctly determine frame width when called from QtQuickControls > > When oxygen is used in QtQuickControls the widget parameter is empty > as such we need to determine the widget type (lineedit etc. ) in a > different way > > > Diffs > ----- > > kstyles/oxygen/oxygenstyle.cpp 86b5cdf3054f5d362d90f0f76c30bfb4f2646911 > > Diff: http://git.reviewboard.kde.org/r/112375/diff/ > > > Testing > ------- > > > File Attachments > ---------------- > > QML After > http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/spell1.png > QML_Before > http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/spell1_1.png > Normal oxygen demo > http://git.reviewboard.kde.org/media/uploaded/files/2013/08/30/oxygen1.png > > > Thanks, > > David Edmundson > > --===============2976073055457416244== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit
This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112375/

On August 31st, 2013, 12:14 p.m. UTC, Hugo Pereira Da Costa wrote:

kstyles/oxygen/oxygenstyle.cpp (Diff revision 4) 
namespace Oxygen
612 

    612
    
                if( qobject_cast<const QLineEdit*>( widget ) ||

  


 




  
Rather than looking up the property multiple times, alongside the q_object_cast, I would rather add the test just before the last 'if' (when everything else has failed), and possibly caching the property in a QString. 

Something like

else if( !widget && option && option->styleObject ) {
    const QString elementType = option->styleObject->property( elementType );

    ... your code (all ifs, duplicated from the widget case)

} else { 

    return 1;

}

This way, you don't lookup the property if the widget is defined,
and You don't lookup the property multiple times. 
This comes at the price of some code duplication, but it is a small block of code, so I'd deem that acceptable.

Oppinion ?

 









Makes sense. Will change.








- David






On August 30th, 2013, 3:56 p.m. UTC, David Edmundson wrote:











 

  


Review request for Plasma and Hugo Pereira Da Costa.


By David Edmundson.




Updated Aug. 30, 2013, 3:56 p.m.








Description 

 

 

  
   
Correctly determine frame width when called from QtQuickControls

When oxygen is used in QtQuickControls the widget parameter is empty
as such we need to determine the widget type (lineedit etc. ) in a
different way

  		
 









Diffs 


    

 
  • kstyles/oxygen/oxygenstyle.cpp (86b5cdf3054f5d362d90f0f76c30bfb4f2646911)
    • 





View Diff





File Attachments 








  		
 











  

 



--===============2976073055457416244==--

--===============2442078204265621504==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

--===============2442078204265621504==--