hpereiradacosta added a comment.
Hi,
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.
I have added a few more optimization comments below.
Appart from these, I think what's still missing are:
- an option to disable (both manually and automatically with non-null borders)
- 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,
INLINE COMMENTS
> breezestyle.cpp:4268
> const auto& rect = option->rect;
> - const auto& palette = option->palette;
> + auto palette = option->palette;
> +
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)
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
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)
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.
> breezetoolsareamanager.cpp:162
> + for (auto window : animationMap.keys()) {
> + bool hasWidget = false;
> + if (std::none_of(_registeredWidgets.begin(),
> _registeredWidgets.end(), [window](QWidget *widget) {
Unused
> breezetoolsareamanager.h:40
> + QMap<QWindow*,ToolsAreaAnimation> animationMap;
> + QList<QWidget*> m_widgetsWithPaletteForToolsAreaSet;
> + };
Should use a QSet rather than QList. It is faster on removal and contain check.
Should also use const QWidget* as a contained object to avoid unnecessary need
for const_caast
REPOSITORY
R31 Breeze
REVISION DETAIL
https://phabricator.kde.org/D27669
To: cblack, #plasma, #breeze, #vdg
Cc: 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
