D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200317/eef9484f/attachment-0001.html>