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
