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
