<table><tr><td style="">hpereiradacosta added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D27669">View Revision</a></tr></table><br /><div><div><p>Some more comments, mostly code related. <br />
Thanks for addressing all the comments so far !</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D27669#inline-158041">View Inline</a><span style="color: #4b4d51; font-weight: bold;">breezehelper.cpp:33</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#include</span> <span class="cpf"><QMainWindow></span><span style="color: #304a96"></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#include</span> <span class="cpf"><QScreen></span><span style="color: #304a96"></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This include is not needed any more as far as I can tell</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D27669#inline-158045">View Inline</a><span style="color: #4b4d51; font-weight: bold;">breezehelper.cpp:1723</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">painter</span><span style="color: #aa2211">-></span><span class="n">setPen</span><span class="p">(</span><span class="n">QPen</span><span class="p">(</span><span class="n">toolsAreaBorderColor</span><span class="p">(</span><span class="n">widget</span><span class="p">),</span> <span style="color: #601200">1</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">painter</span><span style="color: #aa2211">-></span><span class="n">setRenderHints</span><span class="p">(</span><span class="n">QPainter</span><span style="color: #aa2211">::</span><span class="n">Antialiasing</span><span class="p">,</span> <span style="color: #304a96">false</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">remove the unnecessary QPen</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D27669#inline-158042">View Inline</a><span style="color: #4b4d51; font-weight: bold;">breezestyle.cpp:59</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; ">#include <QItemDelegate>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">#include <QScreen>
</div><div style="padding: 0 8px; margin: 0 4px; ">#include <QSplitterHandle>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not needed</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D27669#inline-158048">View Inline</a><span style="color: #4b4d51; font-weight: bold;">breezestyle.cpp:174</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> , _tabBarData( new BreezePrivate::TabBarData( this ) )
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> , _toolsAreaManager ( new ToolsAreaManager( this, _helper ) )
</div><div style="padding: 0 8px; margin: 0 4px; "> #if BREEZE_HAVE_KSTYLE
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Compiler complains about variable being initialized in incorrect order. Member initialization must be in the same order as the declaration. (here toolsarea befor tabbardata)<br />
In general try compile with a recent gcc (I have gcc 9.2.1) and fix all warnings.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D27669#inline-158047">View Inline</a><span style="color: #4b4d51; font-weight: bold;">breezestyle.cpp:367</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> } else if ( qobject_cast<QMainWindow*> (widget) || qobject_cast<QDialog*> (widget) ) {
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> widget->setAttribute(Qt::WA_StyledBackground);
</div><div style="padding: 0 8px; margin: 0 4px; "> }
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Just for my understanding: why is it necessary ? <br />
Here at least, nothing changes if I take this out.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D27669#inline-158046">View Inline</a><span style="color: #4b4d51; font-weight: bold;">breezestyle.cpp:879</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> bool Style::drawWidgetPrimitive( const QStyleOption* option, QPainter* painter, const QWidget* widget ) const {
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> if (qobject_cast<const QMainWindow*>(widget) || qobject_cast<const QDialog*> (widget)) {
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">"option" is unused (and compiler complains about it)<br />
Either add Q_UNUSED(option) just put "QStyleOption*," (anonymous variable)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D27669#inline-158044">View Inline</a><span style="color: #4b4d51; font-weight: bold;">breezestyle.cpp:882</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> if (!_helper->toolsAreaHasContents(widget) && _helper->shouldDrawToolsArea(widget)) {
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> painter->setPen(QPen(_helper->toolsAreaBorderColor(widget), 1));
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> painter->setRenderHints(QPainter::Antialiasing, false);
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Just "setPen( _helper-> ...) <br />
you don't need the temporary QPen if it has a width 1.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D27669#inline-158022">View Inline</a><span style="color: #4b4d51; font-weight: bold;">cblack</span> wrote in <span style="color: #4b4d51; font-weight: bold;">breezetoolsareamanager.cpp:108</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I believe this is due to how it determines "should draw tools area" conflicting with your colourscheme.<br />
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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That's still needs fixing though. You cannot have things floating around depending on which color scheme is used. right ? <br />
There are many color schemes of all types in the wild. The widget style must work for all.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D27669#inline-158043">View Inline</a><span style="color: #4b4d51; font-weight: bold;">breezetoolsareamanager.h:9</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">namespace</span> <span class="n">Breeze</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">typedef</span> <span style="color: #aa4000">struct</span> <span class="n">ToolsAreaAnimation</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QPointer</span><span style="color: #aa2211"><</span><span class="n">QVariantAnimation</span><span style="color: #aa2211">></span> <span class="n">foregroundColorAnimation</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Please remove the typedef. Not needed</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R31 Breeze</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D27669">https://phabricator.kde.org/D27669</a></div></div><br /><div><strong>To: </strong>cblack, Plasma, Breeze, VDG<br /><strong>Cc: </strong>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<br /></div>