D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  A few more comments, but all in all seems to be getting there (beside the options to disable and/or to define the colors based on the QPalette and not the decorations)

INLINE COMMENTS

> breezehelper.cpp:1631
> +
> +            QMainWindow* window;
> +            if ((window = grabMainWindow(widget))) {

Coding style: 
I would rather: 
auto window = grabMainWindow( widget );
if( window ) …
This way you avoid the double () and the seemingly uninitialized window.

> breezehelper.cpp:1644
> +        auto checkMenubarInToolsArea = [grabMainWindow](const QWidget *widget) {
> +            QMainWindow* window;
> +            if ((window = grabMainWindow(widget))) {

Same remark.

> hpereiradacosta wrote in breezehelper.cpp:1613
> as far as I can tell you do not need the const_cast. just check the relevant methods to take a const as input. 
> Const_cast must really be avoided as much as possible. 
> I see that it is needed just for the call to window->toolBarArea. If so, just do the const_cast there and keep all the rest const.
> (window->toolBarArea(const_cast<QToolBar*>(toolbar)))

Not really done. The only place where you need the const_cast is in the window->toolbarArea part. I would do it there and there only.  (line 1637) all the other call to toolbar-> work with a const.

> breezestyle.cpp:4352
>  
> +        QPalette::ColorRole textRole( QPalette::ButtonText );
> +        if( flat ) textRole = ( ((hasFocus&&sunken) || (state & State_Sunken))&&!mouseOver) ? QPalette::HighlightedText: QPalette::WindowText;

Why has this code moved ? As far as I can tell it is used only line 4396, and so the initialization should remain in the corresponding if block.

> cblack wrote in breezestyle.cpp:4382
> If the tools area is enabled and a widget is in the tools area, then the palette needs changing. It is ugly, but that's what the best you can get when KIconLoader ignores widget palettes.

Not at every paint event. You should check if kiconloader already have a customPalette, if it matches the one you want, and update (or reset) only when necessary.

> breezetoolsareamanager.cpp:139
> +                    this, [this]() {
> +                        emit this->toolbarUpdated();
> +                    });

more unnecessary "this->"
Please try to remove them all.

> breezetoolsareamanager.cpp:159
> +
> +    void ToolsAreaManager::unregisterWidget(QWidget *widget)
> +    {

you should also remove the widget from widgetsWithPaletteForToolsAreaSet

> breezetoolsareamanager.h:19
> +    public:
> +        explicit ToolsAreaManager(QObject *parent = nullptr, Helper* helper = nullptr);
> +        ~ToolsAreaManager();

you don't need the default arguments.
and in fact passing nullptr for helper will crash the code everywhere.

> breezetoolsareamanager.h:28
> +        
> +        QMap<QWindow*,ToolsAreaAnimation> animationMap;
> +        QList<QWidget*> widgetsWithPaletteForToolsAreaSet;

don't put members public. Members should be private, and proper accessors/modifiers should be added.

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/20200310/3043e4a8/attachment-0001.html>