<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>Hi, <br />
finally had some time to double-check into kiconloader, and confirmed that setting the custom palette and reseting is pretty harmless since it does not invalidate the cache. So to me it is fine to leave this part as it is now.<br />
I have added a few more optimization comments below. <br />
Appart from these, I think what's still missing are:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">an option to disable (both manually and automatically with non-null borders)</li>
<li class="remarkup-list-item">adressing the remaining comments (such as the unrelated metrics change, which IMO should really go into a separate commit and be justified on its own rather than within this (large) changeset,</li>
</ul></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-159271">View Inline</a><span style="color: #4b4d51; font-weight: bold;">breezestyle.cpp:4268</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; "> const auto& rect = option->rect;
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright">const </span>auto<span class="bright">&</span> palette = option->palette;
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> auto palette = option->palette;
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">as far as I can tell, this palette is used only later, in "drawItemText" and only if text is shown. I would move this all block there (line 4395 or so)<br />
Rational is that every time you call setColor to the palette, you actually detach the palette and create a new one, which is expensive. So one should do it as little as possible<br />
In this case, when there are no text on the toolbar items, you would then never create the palette (and dont actually need it at all)<br />
I have tried to look at the other places where you call palette.setColor(), but have found no easy way to avoid it (or minimize its impact). Still, feel free to have a look too.</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-159251">View Inline</a><span style="color: #4b4d51; font-weight: bold;">breezetoolsareamanager.cpp:162</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: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">auto</span> <span style="color: #a0a000">window</span> <span class="p">:</span> <span class="n">animationMap</span><span class="p">.</span><span class="n">keys</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">bool</span> <span class="n">hasWidget</span> <span style="color: #aa2211">=</span> <span style="color: #304a96">false</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">std</span><span style="color: #aa2211">::</span><span class="n">none_of</span><span class="p">(</span><span class="n">_registeredWidgets</span><span class="p">.</span><span class="n">begin</span><span class="p">(),</span> <span class="n">_registeredWidgets</span><span class="p">.</span><span class="n">end</span><span class="p">(),</span> <span class="p">[</span><span class="n">window</span><span class="p">](</span><span class="n">QWidget</span> <span style="color: #aa2211">*</span><span class="n">widget</span><span class="p">)</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Unused</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-159249">View Inline</a><span style="color: #4b4d51; font-weight: bold;">breezetoolsareamanager.h:40</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">QMap</span><span style="color: #aa2211"><</span><span class="n">QWindow</span><span style="color: #aa2211">*</span><span class="p">,</span><span class="n">ToolsAreaAnimation</span><span style="color: #aa2211">></span> <span class="n">animationMap</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QList</span><span style="color: #aa2211"><</span><span class="n">QWidget</span><span style="color: #aa2211">*></span> <span class="n">m_widgetsWithPaletteForToolsAreaSet</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">};</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Should use a QSet rather than QList. It is faster on removal and contain check. <br />
Should also use const QWidget* as a contained object to avoid unnecessary need for const_caast</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>IlyaBizyaev, 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>