D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Some more comments, mostly code related. 
  Thanks for addressing all the comments so far !

INLINE COMMENTS

> breezehelper.cpp:33
> +#include <QMainWindow>
> +#include <QScreen>
>  

This include is not needed any more as far as  I can tell

> breezehelper.cpp:1723
> +
> +        painter->setPen(QPen(toolsAreaBorderColor(widget), 1));
> +        painter->setRenderHints(QPainter::Antialiasing, false);

remove the unnecessary QPen

> breezestyle.cpp:59
>  #include <QItemDelegate>
> +#include <QScreen>
>  #include <QSplitterHandle>

Not needed

> breezestyle.cpp:174
>          , _tabBarData( new BreezePrivate::TabBarData( this ) )
> +        , _toolsAreaManager ( new ToolsAreaManager( this, _helper ) )
>          #if BREEZE_HAVE_KSTYLE

Compiler complains about variable being initialized in incorrect order. Member initialization must be in the same order as the declaration. (here toolsarea befor tabbardata)
In general try compile with a recent gcc (I have gcc 9.2.1) and fix all warnings.

> breezestyle.cpp:367
> +        } else if ( qobject_cast<QMainWindow*> (widget) || qobject_cast<QDialog*> (widget) ) {
> +            widget->setAttribute(Qt::WA_StyledBackground);
>          }

Just for my understanding: why is it necessary ? 
Here at least, nothing changes if I take this out.

> breezestyle.cpp:879
>  
> +    bool Style::drawWidgetPrimitive( const QStyleOption* option, QPainter* painter, const QWidget* widget ) const {
> +        if (qobject_cast<const QMainWindow*>(widget) || qobject_cast<const QDialog*> (widget)) {

"option" is unused (and compiler complains about it)
Either add Q_UNUSED(option) just put "QStyleOption*," (anonymous variable)

> breezestyle.cpp:882
> +            if (!_helper->toolsAreaHasContents(widget) && _helper->shouldDrawToolsArea(widget)) {
> +                painter->setPen(QPen(_helper->toolsAreaBorderColor(widget), 1));
> +                painter->setRenderHints(QPainter::Antialiasing, false);

Just "setPen( _helper-> ...) 
you don't need the temporary QPen if it has a width 1.

> cblack wrote in breezetoolsareamanager.cpp:108
> I believe this is due to how it determines "should draw tools area" conflicting with your colourscheme.
> When active, the window and the titlebar colours are different, thus causing the tools are to be drawn. When inactive, the window and the titlebar colours are the same, causing the tools area to not be drawn.

That's still needs fixing though. You cannot have things floating around depending on which color scheme is used. right ? 
There are many color schemes of all types in the wild. The widget style must work for all.

> breezetoolsareamanager.h:9
> +namespace Breeze {
> +    typedef struct ToolsAreaAnimation {
> +        QPointer<QVariantAnimation> foregroundColorAnimation;

Please remove the typedef. Not needed

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: davidre, davidedmundson, hpereiradacosta, gvgeo, ngraham, manueljlin, niccolove, ndavis, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200310/493bf21c/attachment.html>