D28651: Load and use global animation settings

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Thanks for this change !
  Can the same be done in the breeze window decoration ? It is strange to have 
an animation settings there and not in the style.
  
  Hugo

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D28651

To: sandsmark, #breeze, ngraham
Cc: meven, cblack, davidedmundson, ngraham, hpereiradacosta, ndavis, 
plasma-devel, #breeze, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  I have no idea how this is supposed to work. But the current fix overwrites 
what's in the breeze configutation, right ? Would it not lead to utter 
confusion ? 
  These settings should be set at one place and one only. Whether breeze or 
global I have no strong oppinion (so no strong objection to this approach), but 
then the other setting must go. (presently: the one in breeze)
  
  Also, one should double check if this is really what we wants, for animation 
speed, or if additional factors are needed. (not all animations must have the 
same speed).
  
  I have no time to test this though. Someone else should have a look (and 
check conflicts with the breeze option)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D28651

To: sandsmark, #breeze
Cc: hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Hi 
  I don't think it is very satisfactory to say that the perforrmance hit it is 
"the apps fault" ... It is not there without the change, and with other widgets 
styles (or is it ? Did any one check, e.g. oxygen which is quite resource heavy 
because of gradients and so ?)
  You can't expect applcations dev to go optimize their app for the need of a 
given style. especially if it is not trivial. 
  I don't see another choice, if such popular apps as kate and kdevelop are 
affected, than to go do the optimization 'ourself', if indeed this is the issue.
  At the minimum someone should try to profile this, using e.g. callgrind, to 
see where the curlprit is ...

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg, hpereiradacosta, davidre
Cc: maartens, abstractdevelop, IlyaBizyaev, davidre, davidedmundson, 
hpereiradacosta, 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D27669#637164 , @ndavis wrote:
  
  > I've discovered another bug. When you move a window by dragging on an empty 
area, all hover effects stop working.
  
  
  This bug has been here since forever and is not related to the changes in 
this patch.
  It is due to X11 eating one of Qt's mouse release events when starting to 
drag the window from empty areas. 
  It was fixed at some point in the distant past, and the re-introduced ...

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg, hpereiradacosta, davidre
Cc: abstractdevelop, IlyaBizyaev, davidre, davidedmundson, hpereiradacosta, 
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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> breezehelper.cpp:1687
> +while (parent != nullptr) {
> +if (qobject_cast(widget) || qobject_cast QDockWidget*>(widget)) {
> +return false;

mmm shouldn't you test on parent rather than widget here ?

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg, hpereiradacosta, davidre
Cc: IlyaBizyaev, davidre, davidedmundson, hpereiradacosta, 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D27669#634758 , @ngraham wrote:
  
  > So I'm still having an issue with the feature um, not working. :( I have it 
turned on in the Breeze settings but it behaves as if off; that is to say, I 
see no different appearance compared to the status quo.
  >
  > Is anyone else experiencing this, or just me?
  
  
  ... it certainly is working here. 
  F8199260: Screenshot_20200325_210812.png 

  
  So ... can't help you (not to say that there isn't any problem. just that it 
is not completely trivial to find)

REPOSITORY
  R31 Breeze

BRANCH
  cblack/toolsarea

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg, hpereiradacosta
Cc: IlyaBizyaev, davidre, davidedmundson, hpereiradacosta, 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.


  All good, as far as I am concerned. Thanks !

REPOSITORY
  R31 Breeze

BRANCH
  cblack/toolsarea

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg, hpereiradacosta
Cc: IlyaBizyaev, davidre, davidedmundson, hpereiradacosta, 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  I finally got some time to look at the code. Some minor comments below 
(compiler warnings)
  Also there is a problem with menubar text color when option is disabled. Here 
at least it is still set to the decoration color, leading to invisible text 
with default breeze color scheme.
  See: 
  F8199046: Screenshot_20200325_155010.png 

  F8199048: Screenshot_20200325_155034.png 


INLINE COMMENTS

> breezehelper.cpp:50
> +_config( std::move( config ) ),
> +_decorationConfig( new InternalSettings() ),
> +_kwinConfig( KSharedConfig::openConfig("kwinrc") ),

compiler complains about wrong initialization order

> breezestyle.cpp:4935
> +
> +bool Style::drawMenuBarEmptyAreaControl( const QStyleOption* option, 
> QPainter* painter, const QWidget* widget ) const
> +{

same remark about option being unused

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg, hpereiradacosta
Cc: IlyaBizyaev, davidre, davidedmundson, hpereiradacosta, 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Hi Carson, 
  Thanks for the update. At first glance this all look pretty good. I should be 
able to do some more in depth testing and code review by the end of tomorrow. 
  Hugo

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: IlyaBizyaev, davidre, davidedmundson, hpereiradacosta, 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> cblack wrote in breeze.h:104
> The tools area's separator has to separate two large areas of the window and 
> thus should be stronger than the separators that only have to separate 
> borderless and backgroundless buttons.

As I said, to me at least it does not look so good in the two screenshots you 
posted above. 
In general I think it is not a good idea to arbitrarily multiply the various 
shades/colors used in a single ui, to fullfil the same role. Beside, the two 
areas in questions have a different background already which in itself 
constitutes a strong separator. 
Anyway I'll let other VDG members comments/decide on this. 
Just posting my opinion.
Not a show stopper on my side

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: IlyaBizyaev, davidre, davidedmundson, hpereiradacosta, 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> cblack wrote in breeze.h:104
> With: F8185479: image.png 
> Without: F8185480: image.png 

Sorry for the many postings, I had another unrelated comment on these 
screeshot: 
I find it strange that the toolarea separator color in this screenshot is 
brighter than the toolbar separators (the vertical lines between tool buttons). 
IMO it would make more sense to have both have the exact same color, since they 
fullfil the same role (separating ...)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: IlyaBizyaev, davidre, davidedmundson, hpereiradacosta, 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> cblack wrote in breezetoolsareamanager.cpp:133
> This only triggers when the window has no items in the tools area—it's used 
> in conjunction with line 878 of breezestyle.cpp in order to render a border 
> at the top of the window.

Clear enough ! Sorry for the noise ! (and not having read the code with enough 
attention).

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: IlyaBizyaev, davidre, davidedmundson, hpereiradacosta, 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D27669#630974 , @ngraham wrote:
  
  > Nice, the colors are better now. I still see a difference in animation 
speed when the titlebar and toolbar change color though. It's especially 
visible with the current Breeze color scheme.
  >
  > F8185590: vokoscreenNG-2020-03-19_20-53-04.webm 

  
  
  Here at least (also X11) not only is the window decoration out of sync with 
the toolarea, but when you have both a menubar and a toolbar (as in e.g. 
okular), both are also out of sync. I think it is because rendering the 
background is expensive and both widgets are not rendered at the exact same 
time. 
  one solution could be to render the background directly on the mainwindow 
(leaving the toolbar and the menubars transparent, as they were before). I have 
not investigated further how difficult this would be to implement though (if 
even possible)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: IlyaBizyaev, davidre, davidedmundson, hpereiradacosta, 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Hi Carlston, 
  Thanks for the updated patch and screenshot. 
  First I agree that the new (latest) checks on whether the toolbar palette was 
changed or not are much more elegant and just as efficient as the previous 
implementation.
  Second, some more comments below.

INLINE COMMENTS

> cblack wrote in breeze.h:104
> With: F8185479: image.png 
> Without: F8185480: image.png 

Hi,
thanks for posting the screenshots. If I understand right this is really an 
issue with the bottom margin only, and in particular with respect to the 
separator. In that case I agree this is part of this patch. 
I don't think the solution is the right one though. First, changing the metric 
will also affect widget rendering when

- the effect is disabled (and I don't think it should in that case)
- when there is no separator line (and IMO I don't think the extra margin is 
needed in that case either).
- when toolbars are located elsewhere (on the sides of the main window for 
instance).

Second, it affects all sides, whereas only the bottom one needs changed. 
i would rather change the "contentsmargin" of the toolbars when necessary, 
while leaving the metrics unchanged. This could be done whenever you check that 
a given toolbar is in the toolarea.

If you insist on changing the margins on all toolbars, all sides, disregarding 
their location and disregarding whether the effect is enabled or not, then this 
is a change orthogonal (and of much broader scope) to this patch and must go to 
a different commit.

> breezetoolsareamanager.cpp:133
> +window->setContentsMargins(0,1,0,0);
> +}
> +});

Did you check that this chunk of code (changing the contents margins of the 
QMainWindow) has any effect ?  I had my doubt so I changed the margin to 100 
instead of 1, added a printout to make sure that this line of code is called, 
then ran dolphin and 
1/ the code is indeed called
2/ it changes nothing, on dolphin, okular, ...
if you can confirm, then just drop it.

(in fact, I don't understand the login in the first place: why would you need 
to add one pixel on top of everything in the QMainWindow ?)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: IlyaBizyaev, davidre, davidedmundson, hpereiradacosta, 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

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 animationMap;
> +QList 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

D28087: Fix Defaults not being set properly in Breeze window decoration settings for 'Draw a circle around close button'

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.


  Many thanks !

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D28087

To: paulm, #breeze, hpereiradacosta
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Generating the toolarea colors from the windows colors and making sure the 
WindowText color still works; is also a solution. But it requires to also 
change the decoration to use this generated color instead of that of a palette, 
and an option to go with it. Also: you would still need an option to disable 
all this, especially when window borders are not "none", or when people want to 
use their palette decoration color.

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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  For what its worth: what you would need to fix this properly, is a new 
QIcon::Mode for "in active tool area" that you would map the the right colors 
when generating a given pixmap from the svg inside kiconloader. 
  Only then would you be able to cache for a given icon its properly colored 
pixmap for being inside the tool area; in a regular button; active; disabled, 
etc ... without the need to resort to different QPalette as done now (namely 
one palette for active toolarea and one palette for all the rest).

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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> hpereiradacosta wrote in breezestyle.cpp:4382
> I'll dig in KIconLoader code a bit over the week-end to see what really 
> happens when calling setCustomPalette and resetPalette, to get a sense of how 
> resource consuming this is ...

So,
Digging into the code it seems that this might not be as big of a problem as 
anticipated. 
My fear was that changing the qpalette in kiconloader (setCustomPalette or 
resetPalette) would invalidate the internal cache each time. (which would be 
disastrous, effectively removing all the caching ability). I need to double 
check, but it seems it is not the case: the actual palette is used in the cache 
key. So you end up with a twice larger cache (which is ok) but no cache reset. 
The only thing settingCustomPalette and resetPalette do is change the palette 
key for next cache access. 
So, if confirmed the only remaining thing would be to try avoid too many 
palette creation, by using the palette intrinsic sharing.

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

D27892: [RFC] Don't draw shadows on quick tiled or maximized edges

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D27892#623436 , @davidre wrote:
  
  > In D27892#623425 , @ngraham 
wrote:
  >
  > > +1 for the concept and resulting appearance. But, should this maybe be 
done in KWin instead? That way all window decoration themes would get this 
fix/change, not just Breeze.
  >
  >
  > I don't know about the best place but  not drawing a border if quick tiled 
or maximized is also done in breeze.
  >  Also @davidedmundson said to do it on the decorations  level.
  
  
  Well, the thing is, all the gymnastic you do here for cutting out borders and 
corners is already done inside kdecoration2, to split the passed image based on 
the passed padding. So keeping the code here is very redundant with code that 
already existes elsewhere. It should be much easier (and more generic) to do 
this upstream. adding @zzag for further comments on this.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27892

To: davidre, #breeze, #vdg, zzag, hpereiradacosta
Cc: davidedmundson, ngraham, anthonyfieroni, 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> hpereiradacosta wrote in breezestyle.cpp:4382
> I am sorry to say this is a serious show stopper. 
> One cannot reset/update the kiconloader palette at every repaint event for 
> every single toolbutton in toolbars especially when there are animations (not 
> only for the active inactive switch but also simply mouse-over).
> 
> If there is no way to make this more efficient, then as far as I am concerned 
> this is a no-go.
> 
> Maybe one could try to cache the icons ourselves ? (and trigger cache clear 
> when appropriate ?)

I'll dig in KIconLoader code a bit over the week-end to see what really happens 
when calling setCustomPalette and resetPalette, to get a sense of how resource 
consuming this is ...

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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> cblack wrote in breezestyle.cpp:4382
> The issue is that the QStyle only has the global icon palette to mutate for 
> the tools area.  If the custom palette were to be left intact, that would 
> affect widgets it's not supposed to. There is no other code flow that won't 
> result in KIconLoader using a custom palette when it shouldn't.
> 
> The reason this is necessary in the first place is that KIconLoader doesn't 
> honor the widget palettes set elsewhere in the QStyle. This code is simply 
> working with what it's being given, and the issues that lead to this solution 
> are deep-rooted in Qt. You simply cannot do anything else without likely 
> breaking Qt's current APIs that are in control of this.

I am sorry to say this is a serious show stopper. 
One cannot reset/update the kiconloader palette at every repaint event for 
every single toolbutton in toolbars especially when there are animations (not 
only for the active inactive switch but also simply mouse-over).

If there is no way to make this more efficient, then as far as I am concerned 
this is a no-go.

Maybe one could try to cache the icons ourselves ? (and trigger cache clear 
when appropriate ?)

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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> breezestyle.cpp:4897
> +
> +auto palette = toolbar->palette();
> +palette.setColor( QPalette::Window, 
> _toolsAreaManager->background(widget) );

This should move in the if condition below. Creating a new palette (even by 
copy) is expensive. 
When you use the palette.color( QPalette::Window) later in line 4909, you 
should use the color directly instead, namely 
_toolsAreaManager->background(widget) (which you can evaluate here already 
before entering the if loop. This should be cheaper.

> breezestyle.cpp:4933
> +QRectF copy(widget->geometry());
> +copy.setTop(widget->childrenRect().top());
> +

another case where it is probably more correct to use option->rect directly.

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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  I noticed there seems to be something wrong in the color logic when
  
  - active window decoration color is the same as the main window decoration
  - inactive window decoration is different
  
  (essentially the opposite as current breeze theme)
  In that case, one would expect the inactive tool area to be colored and the 
active one not to be. It seems that neither are colored except for the menu bar.
  See: 
  F8169741: Screenshot_20200310_214627.png 

  corresponding to an *inactive* window in this modified color scheme (just for 
illustration purpose)

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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> cblack wrote in breeze.h:104
> This is part of the visual changes of the tools area design—extra spacing as 
> to prevent ugly border on border action.

Why would this be more "ugly" as when there was no toolarea ? can you post a 
screeshot of the said ugliness ?

> breezetoolsareamanager.cpp:15
> +ToolsAreaManager::~ToolsAreaManager() {
> +for (auto entry : animationMap) {
> +delete entry.foregroundColorAnimation;

This is not needed. Since all animations are created as children of the 
ToolsAreaManager, they should be deleted automatically when this one is 
destroyed. 
This way you can simply remove the class destructor altogether and rely on the 
default one (from QObject).

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

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(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&) || (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 animationMap;
> +QList 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> breezehelper.cpp:1613
> +
> +auto castedWidget = const_cast(widget);
> +

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(toolbar)))

> hpereiradacosta wrote in breezestyle.cpp:4579
> Are you sure about the logic here ? geometry is relative to the parent 
> corrdinate system while childrenRect is relative to current widget. 
> Also, I would as expected that opt->rect would have been enough in any case 
> without the need to resort to the widget accessors

Here at least, using opt->rect directly (no need for the copy), fixes the 
multiple toolbar issue, with no regression elsewhere.
No regression on the menubar empty area either.

> breezestyle.cpp:4890
> +{
> +auto toolbar = const_cast(qobject_cast QToolBar*>(widget));
> +

as far as I can tell you dont need the cast to QToolbar. Just use the widget.
(and in any case if you use the toolbar you must make sure that the case 
succeeded.)

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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D27669#625338 , @ngraham wrote:
  
  > FWIW, I'm okay making this configurable, as I do admit that it could be a 
very polarizing change. We do intend to change the default Breeze color scheme 
itself to use a light gray for the titlebar, but yes, I understand that people 
who prefer very dark or bold titlebar colors in an otherwise light color scheme 
might not like the change, and they should be able to disable this without 
ditching Breeze entirely.
  >
  > However it's also worth mentioning that there's been a persistent request 
for exactly that appearance, as some people really seem to like a 
strongly-colored top area and I anticipate that they'll enjoy using this with a 
color scheme like Honeycomb.
  
  
  I doubt it. did you actually try ? 
  The active window decoration and the highlight color are the same in honey 
comb. Meaning that you have essentially no more mouse-over effect or focus 
effects in toolbars and menubars (see:

INLINE COMMENTS

> breezestyle.cpp:4579
> +
> +QRectF copy(widget->geometry());
> +copy.setTop(widget->childrenRect().top());

Are you sure about the logic here ? geometry is relative to the parent 
corrdinate system while childrenRect is relative to current widget. 
Also, I would as expected that opt->rect would have been enough in any case 
without the need to resort to the widget accessors

> breezestyle.cpp:4910
> +
> +QRectF copy(widget->geometry());
> +copy.setTop(widget->childrenRect().top() - 10);

Same remark as above.

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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> breezestyle.cpp:4382
> +QPixmap pixmap = toolButtonOption->icon.pixmap( iconSize, 
> iconMode, iconState );
> +if (_helper->isInToolsArea(widget)) {
> +KIconLoader::global()->setCustomPalette(widget->palette());

updating the palette in every paint event (and calling resetPalette()) will be 
very innefficient. You need to track whether the palette actually needs change 
before calling these.

> breezestyle.cpp:4894
> +toolbar->setPalette(toolbar->parentWidget()->palette());
> +return true;
> +}

Same remark. This is very innefficient. 
You need to track whether the palette actually need change.
Ideally it should be enough to set the right palette in polish, and then to 
update on toolbar position change (can be achieved by catching moveEvent with 
an event filter on the toolbar)

> breezestyle.cpp:4902
> +
> +toolbar->setPalette(palette);
> +

same remark

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

the this-> here are all unnecessary.

> cblack wrote in breezetoolsareamanager.cpp:108
> This is intentional—if a window's titlebar and content colours are the same, 
> the window is drawn as a homogeneous window instead of a window with a tools 
> area.

I was not refering to the rendering of the separator, but rather to the fact 
that the widgets margin change between active and inactive. This should not 
happen (disregarding the color scheme). 
All in all, whether you need to draw the separator or not you need to add the 
margin always (or follow David's suggestion)
Unless I misunderstood the actual issue reported by Nathan of course.

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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Some more comments about the code.

INLINE COMMENTS

> breeze.h:104
>  // toolbars
> -ToolBar_FrameWidth = 2,
> +ToolBar_FrameWidth = 6,
>  ToolBar_HandleExtent = 10,

This change seems unrelated to introducing a tool area. 
I would move it to a separate commit and review request. In fact there are 
already complains about there being too much space wasted in breeze. These 
extra 4 pixels don't go in the right direction (and again: are not needed for 
this patch as far as I can tell)

> breezehelper.cpp:1703
> +for (auto widget : widgets) {
> +if (this->isInToolsArea(widget) == true) {
> +return true;

this-> is not necessary.

> breezehelper.cpp:1740
> +for (auto widget : widgets) {
> +if (this->isInToolsArea(widget)) {
> +return widget;

this-> is not necessary

> breezehelper.h:31
>  
> +#include 
>  #include 

Not needed as far as I can tell (none of your changes in this file require 
QToolbar)

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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D27669#625270 , @IlyaBizyaev 
wrote:
  
  > For color schemes that have contrasting titlebar and content colors, this 
patch makes application headers look giant...
  >  F8168755: photo_2020-03-10_15-09-54.jpg 

  >  It does look good with the proposed default color palette, but it makes 
many others, including current Breeze, look like an ugly CSD parody :/
  
  
  yes. It also breaks menubar/toolbar highlight and mouse-over when the 
corresponding colors are similar to the active window color. 
  (oxygen color scheme for instance). I was about to comment on this next. I 
think the only way for this to work is for breeze to completely ignore the 
decoration colors, and calculate its own, based on QPalette::Window and 
WindowText colors. 
  The same way as oxygen style was doing to ensure proper blend of the 
decoration and main window. 
  Now you can also add an option to force respecting the decoration colors, 
(oxygen style has such an option), but then it is the responsibility of the use 
who checks this to fix her/his colorscheme accordingly.
  
  Concerning window border, I think that there is no choice but to disable this 
entirely if the window borders are not "none" or "no side". 
  And in fact, having a user option to disable this anyway in the config might 
be needed. This is a quite intrusive change, and many (me included) might want 
to live without.

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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Some more comments, mostly code related. 
  Thanks for addressing all the comments so far !

INLINE COMMENTS

> breezehelper.cpp:33
> +#include 
> +#include 
>  

This include is not needed any more as far as  I can tell

> breezehelper.cpp:1723
> +
> +painter->setPen(QPen(toolsAreaBorderColor(widget), 1));
> +painter->setRenderHints(QPainter::Antialiasing, false);

remove the unnecessary QPen

> breezestyle.cpp:59
>  #include 
> +#include 
>  #include 

Not needed

> breezestyle.cpp:174
>  , _tabBarData( new BreezePrivate::TabBarData( this ) )
> +, _toolsAreaManager ( new ToolsAreaManager( this, _helper ) )
>  #if BREEZE_HAVE_KSTYLE

Compiler complains about variable being initialized in incorrect order. Member 
initialization must be in the same order as the declaration. (here toolsarea 
befor tabbardata)
In general try compile with a recent gcc (I have gcc 9.2.1) and fix all 
warnings.

> breezestyle.cpp:367
> +} else if ( qobject_cast (widget) || 
> qobject_cast (widget) ) {
> +widget->setAttribute(Qt::WA_StyledBackground);
>  }

Just for my understanding: why is it necessary ? 
Here at least, nothing changes if I take this out.

> breezestyle.cpp:879
>  
> +bool Style::drawWidgetPrimitive( const QStyleOption* option, QPainter* 
> painter, const QWidget* widget ) const {
> +if (qobject_cast(widget) || qobject_cast QDialog*> (widget)) {

"option" is unused (and compiler complains about it)
Either add Q_UNUSED(option) just put "QStyleOption*," (anonymous variable)

> breezestyle.cpp:882
> +if (!_helper->toolsAreaHasContents(widget) && 
> _helper->shouldDrawToolsArea(widget)) {
> +painter->setPen(QPen(_helper->toolsAreaBorderColor(widget), 
> 1));
> +painter->setRenderHints(QPainter::Antialiasing, false);

Just "setPen( _helper-> ...) 
you don't need the temporary QPen if it has a width 1.

> cblack wrote in breezetoolsareamanager.cpp:108
> I believe this is due to how it determines "should draw tools area" 
> conflicting with your colourscheme.
> When active, the window and the titlebar colours are different, thus causing 
> the tools are to be drawn. When inactive, the window and the titlebar colours 
> are the same, causing the tools area to not be drawn.

That's still needs fixing though. You cannot have things floating around 
depending on which color scheme is used. right ? 
There are many color schemes of all types in the wild. The widget style must 
work for all.

> breezetoolsareamanager.h:9
> +namespace Breeze {
> +typedef struct ToolsAreaAnimation {
> +QPointer foregroundColorAnimation;

Please remove the typedef. Not needed

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: 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

D27669: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  The toolbar background seems to be broken when there are multiple toolbars in 
the same row.
  See, e.g. ktorrent:
  F8168244: Screenshot_20200309_213717.png 

  Does not happen for applications that have a single toolbar (most 
applications these days)
  Or multiple toolbars but one per row.
  Also happens with kdevelop, or konqueror.
  Then also there are problems with colors (for all color schemes that have 
strong contrast between window decoration and window). But should be discussed 
once the above is fixed.

INLINE COMMENTS

> breezestyle.h:505
> +//* signal manager for the tools area
> +ToolsAreaManager* _toolsAreaManager;
> +

Should initialize there to null ptr (like the other members). This ensures that 
you don't have a dangling pointer if for some reason someone deletes the 
initialization with new in the styles constructor.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: 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

D27938: 'Classic' and 'Redmond' button icon styles, configurable via Breeze window decoration settings

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In my opinion this should really go into a different decoration. Starting to 
have "styles inside styles" inside a single decoration is a UI mess. 
  If you want to turn breeze into a theme engine and not a theme, then so be 
it, but one must go the full way. Not just essentially duplicating the code to 
render a different set of visually orthogonal buttons into the engine. The new 
buttons must have a different entry in the kcm. Not be selectable by an 
internal option.  See how decoration engines like aurorae implements different 
entries in decoration page as the way it should be.
  How is a new user supposed to discover that what she/he sees in the KCM is 
not the only set of icons she/he can chose ?
  Also on the code side, the current patch makes the code maintainability much 
more complicated.
  
  Now I am not the maintainer any more. So feel free to ignore my opinion here. 
  But I cannot +1 this, sorry. (although the idea is nice, design is nice, etc.)
  
  Hugo

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27938

To: paulm, hpereiradacosta, #breeze, ndavis
Cc: ngraham, plasma-devel, manueljlin, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, konkinartem, ian, jguidon, Ghost6, jraleigh, zachus, MrPepe, 
fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, trickyricky26, 
ragreen, crozbo, ndavis, ZrenBot, firef, skadinna, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, ahiemstra, 
mbohlender, mart

D27669: WIP: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> hpereiradacosta wrote in breezehelper.cpp:1726
> Then shouldn't you divide device pixel ratio ? 
> Also, someone else should double check. To me that does not make sense. 
> Everything else (*everything*) scales with device pixel ratio. Why should 
> this line in particular not scale ? What makes it different from regular 
> separators ?

Rereading. First ignore the comment above, I misunderstood your answer. 
What you meant is that if you don't multiply by the dpiratio, then the line 
thickness does not scale. 
Did some testing on my side: 
if I change the scale factor via display kcm, in fact none of the style lines 
scale (for instance all the frames remain one pixel, such as around textviews 
or text entries).
if I use "QT_SCALE_FACTOR =2 dolphin;" for instance, then everything scales 
properly (and I bet the separator line would scale two, also with without 
multiplying by the dpi ratio).
Can you double check ? 
I still think this multiplication should go, consistently with everywhere else 
in the code

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: 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

D27669: WIP: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> breezetoolsareamanager.cpp:146
> +if (window == widget->window()->windowHandle()) {
> +hasWidget = true;
> +}

you can break here once one widget is found. no need to loop over the remaining 
widgets.
alternatively you can use std::any_of or (better) std::none_of

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: 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

D27669: WIP: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> breeze.h:191
> +AnimationPressed = 0x8,
> +AnimationWindowFocused = 0x10,
>  };

Sorry for the many postings. 
As far as I can tell this guy is used nowhere. Please remove.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: 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

D27669: WIP: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> breezetoolsareamanager.h:1
> +#pragma once
> +

Please use garding defines rather than "pragma once" for consistency with the 
rest of the code.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: 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

D27669: WIP: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a subscriber: davidedmundson.
hpereiradacosta added a comment.


  also adding @davidedmundson in the loop in particular for opinion on the 
devicepixel ratio business, and in case he can reproduce the crash.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: 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

D27669: WIP: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> cblack wrote in breezehelper.cpp:1726
> In testing, I found that this would remain 1 physical pixel regardless of 
> scale factor.

Then shouldn't you divide device pixel ratio ? 
Also, someone else should double check. To me that does not make sense. 
Everything else (*everything*) scales with device pixel ratio. Why should this 
line in particular not scale ? What makes it different from regular separators ?

> breezestyle.cpp:886
> +painter->drawLine(
> +widget->rect().left() - 100,
> +widget->rect().top(),

What are these + and -100 numbers ? They sound arbitrary. At the minimum there 
should be a comment, and an enum to define what this number is.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: 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

D27669: WIP: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> breezehelper.cpp:1726
> +
> +painter->setPen(QPen(outline, 
> 1*widget->screen()->devicePixelRatio()));
> +painter->setBrush(Qt::NoBrush);

No. Should be QPen( outline, 1) or just "outline"
DevicePixelRatio is handled by paint engine.

> breezestyle.cpp:882
> +
> +painter->setPen(QPen(outline, 
> 1*widget->screen()->devicePixelRatio()));
> +painter->setBrush(Qt::NoBrush);

Same remark

> breezetoolsareamanager.cpp:97
> +
> +void ToolsAreaManager::registerWidget(QWidget *widget)
> +{

As far as I have followed the code you need an "unregisterWidget" for when 
widgets are unpolished and to remove windows from the map. 
You also need a connection to the destroyed signal to cleanup the map when a 
widget is deleted, otherwise you will keep accumulating an ever growing map of 
dangling pointers.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: 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

D27669: WIP: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D27669#619991 , @cblack wrote:
  
  > In D27669#619230 , 
@hpereiradacosta wrote:
  >
  > > right now it crashes all applications (here at least) when you unlock 
toolbars, move them to a different position, then move them back to top. 
  > >  Tested with dolphin, okular, etc. All crash.
  >
  >
  > Cannot reproduce.
  
  
  Can someone else try ? (Nathan ? Noah ?) I do not have the latest Qt version 
installed so maybe it is just on my system. 
  I can also try produce a crash report. 
  (in more details: open dolphin, unlock toolbars from right click menu in 
toolbar area. Move toolbar to the left side of the window and leave it there. 
Then move it back to the top, where it originally was. Bam. Crash)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: 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

D27669: WIP: [kstyle] Tools area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Overall, this is good stuff I think (although we should also discuss whether 
this should be part of breeze or rather a next-gen theme).
  Still:
  right now it crashes all applications (here at least) when you unlock 
toolbars, move them to a different position, then move them back to top. 
  Tested with dolphin, okular, etc. All crash.
  
  I also have other comments (code wise and design wise), but will only have 
time to post them over the week-end.
  
  Hugo

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27669

To: cblack, #plasma, #breeze, #vdg
Cc: 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

D26823: Port connections to new syntax

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26823#599100 , @davidedmundson 
wrote:
  
  > > yeah ok. Just noted this. indeed.
  >
  > Sorry, I assumed you had seen the thread.
  >  Hope you're ok with it?
  
  
  Sure thing ! The whole thing is more modern and easier to maintain for 
everybody. Glad to see Qt4 going !

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26823

To: davidedmundson, #plasma, zzag
Cc: hpereiradacosta, zzag, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27000: [kstyle] Drop Helper::connection()

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.


  thanks !

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D27000

To: zzag, hpereiradacosta
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26978: [kstyle] Use QX11Info::isCompositingManagerRunning()

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.


  Makes sense, thanks !
  
  In principle, the removal of Helper::connection, although is a change 
unrelated to the commit purpose. Maybe put it in a different commit ? 
  (the story behind if I remember right is that QX11Info::connection have not 
always been available in the past. hence the method)

REPOSITORY
  R31 Breeze

BRANCH
  use-qx11info-is-compositing-manager-active

REVISION DETAIL
  https://phabricator.kde.org/D26978

To: zzag, #plasma, hpereiradacosta
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26823: Port connections to new syntax

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26823#599003 , @zzag wrote:
  
  > In D26823#598999 , 
@hpereiradacosta wrote:
  >
  > > This breaks the KDE4 compilation of breeze style (of course !)
  >
  >
  > Qt 4 build was dropped.
  >
  > https://mail.kde.org/pipermail/plasma-devel/2020-January/108585.html
  
  
  yeah ok. Just noted this. indeed. 
  Up to you I guess.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26823

To: davidedmundson, #plasma, zzag
Cc: hpereiradacosta, zzag, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26823: Port connections to new syntax

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  To compile with kde4: 
  cmake -DUSE_KDE4=1
  
  I know that some distributions (Fedora31 which I am using) do ship the kde4 
version of the breeze style.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26823

To: davidedmundson, #plasma, zzag
Cc: hpereiradacosta, zzag, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26823: Port connections to new syntax

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  This breaks the KDE4 compilation of breeze style (of course !)
  So if this patch is to stay, then one should officially drop the kde4 support 
(or make a branch for it).
  In that case, much more code can go (a lot of QT Version ifdefs, some CMake 
magic, etc.) 
  Otherwise, the kstyle part of this patch at least, should be reverted.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26823

To: davidedmundson, #plasma, zzag
Cc: hpereiradacosta, zzag, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26783: Center only during drawing, not the hit rects

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Thanks for the fix.
  On a different topics, I have found a couple more places where the separators 
look weird. See: 
  F7895454: Screenshot_20200121_102848.png 

  or F7895458: Screenshot_20200121_102934.png 

  In the second screenshot it is barely visible, but that makes it look even 
more like a bug.
  In my opinion we should revisit the heuristics for showing this separators, 
limiting it to actual sunken, non-transparent scroll areas, as opposed to 
standalone scrollbars.
  opinions ? (about the two screenshots above ? About the solution ?)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26783

To: mart, #vdg, hpereiradacosta, ndavis
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  > Can the top of the scrollbar go below the column headers instead of next to 
them?
  > 
  > Like this:
  >  F7886305: Screenshot_20200116_083629.png 

  
  I don't think it is doable inside Qt no, due to how widgets are nested ( 
header in viewport, next to scrollbars, in scrollarea)
  Also, for the last column this would pose serious problems on centering 
between column content and column header (for right align column in particular)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  As a side note: I have been using this code for a couple of days now, and 
while I think the separator looks well for vertical scrollbars, when you have 
both vertical and horizontal, ... this will need some getting used to. 
  See: 
  F7886206: Screenshot_20200116_083629.png 

  This is with a thin handle, but having the thick one does not really improve. 
The view looks like a frame inside a frame, first one being off-centered. 
  I have no clue how this can be improved

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26655#595314 , @mart wrote:
  
  > In D26655#595102 , @ndavis wrote:
  >
  > > I noticed that when hovering on the scrollbar border, while the view area 
is not focused, the scrollbar handle is light gray (Breeze Dark, dark gray for 
Breeze) instead of blue. Moving the mouse just slightly more to the right turns 
the scrollbar handle blue.
  >
  >
  > That's the difference between thesubControlRect of the handle (that has 
been moved by one pixel to the right for the line) and the rectangle of the 
entire scrollbar.
  >
  > to change this and have the handle always hitting, the subcontrolrects 
should stay the same and just paint the handle moved by one pixeltough the 
subcontrol rects not moving.
  >  either is good, probably the latter approach looks a bit dirtier in the 
code but if you think is better from a design pov, it can be done
  
  
  Yeah. I think one has to go this second way, to avoid the 1 pixel color 
flicker noted by Noah: scrollbar hover without handle hover. 
  Right now the scrollbar hit area includes the separator, while the handle 
does not. 
  Agreed that the code will look slightly less intuitive but so be it.
  
  Hugo

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.


  Confirmed that the centering is fixed. Now the arrow seem to have moved 
vertically (for vertical scrollbars) by half a pixel, which is probably a 
consequence of the 20->21, and make them look somewhat thicker due to 
antialiasing. It is a minor detail, so feel free to ignore.
  Other than that, no more comments !

REPOSITORY
  R31 Breeze

BRANCH
  arcpatch-D26655

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  yeah thanks !
  Now, it turns out that the arrows are still ill-aligned (with respect to the 
handle). Maybe they use the full width for rendering ?

REPOSITORY
  R31 Breeze

BRANCH
  arcpatch-D26655

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Alignment again: so you increased the width of the groove rather than 
increasing the width of the scrollbar. This indeed fixes the alignment of the 
handle (and groove), but now the scrollbar arrows are off centered. (for the 
same reason), with both the scrollbar groove and with respect to the line. I 
would revert to the old handle width and rather increase the scrollbar width by 
1 pixel. Then adjust the positioning. Does that make sense ? Increasing the 
width of the arrows by one pixel might break pixel alignment and create 
inconsistencies with other places were arrows are rendered ... 
  Now I realize that the thin scrollbar will still be off centered (and always 
was so far) while it is well centered with your patch, since it is only 3 
pixels while the thick one is 6 pixels ... This was not visible without the 
separator line. But it is going to dissapear with the other patch anyway. 
(otherwise one would probably need to increase its width to 4 pixels)

REPOSITORY
  R31 Breeze

BRANCH
  arcpatch-D26655

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> breezestyle.cpp:6570
> +
> +_helper->renderScrollBarBorder( painter, separatorRect, 
> _helper->alphaColor( option->palette.color( QPalette::WindowText ), 0.1 ));
> +

Another thing:
Color role should be QPalette::Text rather than WindowText, since the vast 
majority of the scrollbars are rendered on views (color roles then being Base 
and Text)

REPOSITORY
  R31 Breeze

BRANCH
  mart/phab/scrollbarseparator2

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Hi again Marco,
  since you mentionned centering in plasmoids in the other commits, in fact 
here also the centering is wrong, with the added separator. Probably the offset 
is the one pixel you take for the separator. Can you double check ? (this 
appears true for QWidget based scrollbars, not QQC). Likely you need to 
increase the scrollbar width by 1, and tune the positionning of the 
groove+handle.

REPOSITORY
  R31 Breeze

BRANCH
  mart/phab/scrollbarseparator2

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  >> my complain here is that this argument was either not made or discarded, 
when the first switch to thin scrollbar was done.
  >>  This is my main concern about this change: the going back and forth using 
adhoc arguments each time to justify the change, often contradicting each 
other.  It means essentially that we either don't know what we are doing, or 
dont think our changes enough. This is bad imo
  > 
  > If I had been around back then or noticed the patch, I would have made this 
argument.
  
  For the record, you were around for the original commit that made the 
scrollbar thiner (https://phabricator.kde.org/D9792)

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26655#594280 , @mart wrote:
  
  > In D26655#594257 , 
@hpereiradacosta wrote:
  >
  > > Is this really intended ? 
  > >  I would at least keep the color blending, making the color stronger only 
when the scrollbar is actually hovered.
  >
  >
  > ouch, well spotted, i will fix that :)
  
  
  Great :)
  And rebase to latest master, so that the patch appears cleanly :)

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  I would start by
  
  - rebase this patch to apply cleanly
  - split it in two (one for the separator line and one for the think handle 
removal), because these are really two features, adressing two different issues:
  
  1: the floating bar
  2: the two small handle that few people dislike.
  
  Adding an option does not really solve anything (especially if it is not the 
default) in terms of the back and forth. 
  If everyone is ok with the back and forth, you should go with it. 
  I can always hack my own breeze clone so that I am happy with it. 
  Just warning how some users could feel about it.

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Removed previous comment because after testing the patch it turns out that 
this is not too strong of an issue. 
  However I also noticed that you not only removed the think handle but also 
the color blending. This make the handle not only larger but also stronger in 
terms of colors, especially in the case where the parent list has focus (you 
get a strong blue handle). 
  Is this really intended ? 
  I would at least keep the color blending, making the color stronger only when 
the scrollbar is actually hovered.

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Also, concerning the separator line: how does it look with hovered 
scrollbars, when the handle grove is also rendered. 
  Isn't there some redundancy between the handle groove and the separator ? 
(essentially a frame inside a frame)
  should the handle groove be removed entirely ?

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26655#594240 , @ngraham wrote:
  
  > Regarding the thickened scrollbars, I think it makes sense for a few 
reasons:
  >
  > - Acknowledging user feedback: we've had a bunch of complaints about the 
thin scrollbars.
  
  
  With all due respect: 
  This is irrelevant unless you do an actual poll. 
  You will have user feedback that want the thin scrollbar back. (I will).
  
  > - Usability: even though the click area was the same size for the thin 
inactive scrollbars, it didn't //look// as large. This can make people 
subconsciously position their cursors more precisely than they need to
  
  my complain here is that this argument was either not made or discarded, when 
the first switch to thin scrollbar was done. 
  This is my main concern about this change: the going back and forth using 
adhoc arguments each time to justify the change, often contradicting each 
other.  It means essentially that we either don't know what we are doing, or 
dont think our changes enough. This is bad imo
  
  > - If we draw a separator line but keep the thin scrollbar, then the track 
looks much too wide because the thin inactive scroll handle looks lost in the 
wide track. But we can't make the track narrower since it has to accommodate 
the scroll handle's thicker expanded width too. Much simpler and more visually 
pleasing to just make it always thicker and not change its size on hover.
  
  Matter of taste. It think I would be happy with thin handle and separator 
line.
  
  > - IMO the overall result just looks good. :)
  
  IMO so does the above (thin handle and separator line). With the advantage 
that it does not feel like a step backward.
  
  > F7883097: Screenshot_20200114_091952.png 

  > 
  > F7883099: Scrollers.png 
  > 
  > F7883100: Scroller 2.png 

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26655#594173 , @ahiemstra 
wrote:
  
  > > In QQC there is an issue of overlap between item selection (and header 
titles) and scrollbar. Isn't that made even worth with the addition of the thin 
separator ?
  >
  > That problem is caused by overlapping scrollbars, D26530 
 makes them no longer overlapping thus 
fixing that problem.
  
  
  Clear enough yes. Thanks !

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Hi Notmart,
  you need to rebase (you have some unrelated changes in the diff, which revert 
latest change from Noah)
  
  - Concerning the separator, it looks great.
  - Concerning reverting the thin bar, I think this is really unfortunate. I 
expect you will have as many complaints after the revert for people to have it 
back, as now of people who want the thick one. I would be among the former. 
Going back and forth in these design issues really gives wrong impression IMHO 
on our design capabilities, and I think this would be one of VDG's task to 
avoid this at all cost.
  
  Now, the thin bar might not work very well with the new separator (and both 
pursue opposite goals: one to make the scrollbar more visible and the other 
less visible). 
  But then again, it just means that we, (including our designers and 
usuability experts), keep going back and forth on which direction to follow 
(here regarding whether we should make the scrollbars more prominent or less 
prominent). This does not give a good impressing to the outside world IMHO.
  
  - In QQC there is an issue of overlap between item selection (and header 
titles) and scrollbar. Isn't that made even worth with the addition of the thin 
separator ?

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: hpereiradacosta, mthw, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26639: Make checkboxes/radiobuttons use Window Background in windows and View Backround in lists

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.


  In D26639#593537 , @ndavis wrote:
  
  > In D26572#593511 , 
@hpereiradacosta wrote:
  >
  > > > Otherwise, it should be Window Background. Doing it that way would 
preserve the original look in most cases.
  > >
  > > This would lead to some regression (I think) for unchecked checkboxes in 
lists. (because of window background being used).
  >
  >
  > This doesn't seem to have any visual regressions like that.
  
  
  That works yes (since you test for parent item view). 
  Also yes, feel free to remove the now unused isItemSelected.
  Thanks for the patch !

REPOSITORY
  R31 Breeze

BRANCH
  checkbox-radiobutton (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26639

To: ndavis, #vdg, #breeze, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26572: Always render checkbox/radiobutton background

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26572#593496 , @ndavis wrote:
  
  > What I meant is that I did not change the color of the checkbox background 
in this patch. I only made it so that the background would always be rendered.
  
  
  Clear enough. Indeed you did not change the colors, and it led to an unwanted 
visual change. Sorry for not having caught that up during review. (in fact it 
now leads to an inconsistency between checkboxes in menus and checkboxes in 
windows).
  
  > It seems like it might be a good idea to detect whether or not the button 
is being rendered in a view area and then set it to View Background.
  
  yes
  
  > Otherwise, it should be Window Background. Doing it that way would preserve 
the original look in most cases.
  
  This would lead to some regression (I think) for unchecked checkboxes in 
lists. (because of window background being used).

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26572

To: ndavis, #vdg, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26572: Always render checkbox/radiobutton background

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26572#593442 , @ndavis wrote:
  
  > In D26572#593379 , 
@hpereiradacosta wrote:
  >
  > > in more detail: imagine a color scheme where window background is black, 
window text is white, view background is white view text is black. 
  > >  you will get a white square on a white background for unchecked checkbox 
... 
  > >  I would really revert this part of the change
  >
  >
  > To be clear, that change hasn't been made.
  
  
  I'm confused: it has happened, no ? If I apply the patch I now get a white 
background (with normal breeze color scheme), behind checkboxes and radio 
button, always, which was not there before.  This is , I think because of the 
change at line 3889.
  
  > You mean a white square on a black background?
  
  No: if view color is white this would be white square  (window text) on white 
background (view background).
  
  > That could look odd if buttons aren't also normally white in that 
colorscheme, which might indicate that Button Background would be more 
semantically correct. By the way, the checkboxes already used View Background 
when selected unless they were drawn on top of a menu.
  
  No: in general background was not rendered. the View background was used for 
a special case of selection that happens only in views (if I remember the code 
right). With the standard breeze color scheme, a checkbox/radio button rendered 
in a window or a menu would never have a white background (from the view) drawn 
behind. 
  Unlike when applying the current patch.
  
  > In that case, they used Window Background when selected

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26572

To: ndavis, #vdg, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26572: Always render checkbox/radiobutton background

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  in more detail: imagine a color scheme where window background is black, 
window text is white, view background is white view text is black. 
  you will get a white square on a white background for unchecked checkbox ...

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26572

To: ndavis, #vdg, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26572: Always render checkbox/radiobutton background

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  I would strongly object to that: 
  first that is a visual change unrelated to the issue this patch adress
  second this is semantically wrong, and this will break on some color theme. 
  Things should be kept consistent. If you change the background role, you must 
also change the foreground role, and then you might really get funny results.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26572

To: ndavis, #vdg, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26572: Always render checkbox/radiobutton background

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.


  ship it. No strong feeling.

REPOSITORY
  R31 Breeze

BRANCH
  checkbox-radiobutton-background (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26572

To: ndavis, #vdg, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26572: Always render checkbox/radiobutton background

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  I would prefer not systematically render the background, because it might 
break existing applications, like rendering a widget on top of a painted image. 
Also this goes against Qt's rendering model which does not require rendering 
widget background, relying on that of the parent widget or window itself. I 
would instead either do it in qqc, or detect if we are rendering a qqc control 
and in this case only render the background systematically. There are several 
places in the code that do that already. Look of "#if BREEZE_HAVE_QTQUICK" 
blocks.
  
  Hugo

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26572

To: ndavis, #vdg, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26225: Change frameRadius to use pen widths, add newPenWidthFrameRadius, add PenWidth::NoPen

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.


  In D26225#583803 , @ndavis wrote:
  
  > - Change newPenWidthFrameRadius to frameRadiusNewPenWidth
  >
  >   This sounds slightly better to me and allows it to show up next to 
frameRadius when autocompleting.
  
  
  What about "frameRadiusForPenWidth"  ? 
  I think the "New" is misleading. 
  If you agree, please change and then commit. The rest sounds good.

REPOSITORY
  R31 Breeze

BRANCH
  replace-hardcoded-2 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26225

To: ndavis, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26267: Replace deprecated QWeakPointer::data() with .toStrongRef().data()

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26267#584400 , @ndavis wrote:
  
  > In D26267#584396 , 
@hpereiradacosta wrote:
  >
  > > -1:
  > >  if QWeakPointer::data() is obsoleted, (while needed in the code), I 
would object to adding toStrongRef, in between, since as pointed out by Antony, 
calling data immediately after brings no further security, and just results in 
some overhead (Weak to shared pointer). It means that either
  > >
  > > - weakPointer::data should not be obsoleted
  > > - the kdecoration api should be changed (to return shared pointers, or 
provide direct access to the data).
  >
  >
  > So this patch is not the correct thing to do at all, even if I make the 
changes @anthonyfieroni suggested. I doubt getting Qt to undeprecate 
QWeakPointer::data() is an option, so it seems like changing KDecoration would 
be the correct thing to do. Does that seem correct to you?
  
  
  That's my understanding yes. As long as access to the true data is needed 
(here for signal/slot connections) by the customers of kdecoration, then 
kdecoration must provide this access, without roundtrips to shared pointers. 
  Others please correct me if I am wrong.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26267

To: ndavis, #breeze, #plasma
Cc: hpereiradacosta, anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26267: Replace deprecated QWeakPointer::data() with .toStrongRef().data()

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  -1:
  if QWeakPointer::data() is obsoleted, (while needed in the code), I would 
object to adding toStrongRef, in between, since as pointed out by Antony, 
calling data immediately after brings no further security, and just results in 
some overhead (Weak to shared pointer). It means that either
  
  - weakPointer::data should not be obsoleted
  - the kdecoration api should be changed (to return shared pointers, or 
provide direct access to the data).

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26267

To: ndavis, #breeze, #plasma
Cc: hpereiradacosta, anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26225: Add newFrameRadius, change frameRadius to use pen widths, add PenWidth::NoPen

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> breezehelper.h:318
> +//* frame radius with new pen width
> +constexpr qreal newFrameRadius ( const qreal oldRadius, const int 
> penWidth ) const
> +{ return qMax( oldRadius - (0.5 * penWidth), 0.0 ); }

Would need a new function name than "newFrameRadius", that makes it clear when 
this should be used on not the other.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26225

To: ndavis, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26217: Add standard pen widths and replace hardcoded numbers

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.


  looks good. See comment about the symbol pen width change and then ship it.
  
  In D26217#583358 , @ngraham wrote:
  
  > ...And after these patches land, let's do the doxygen-friendly comment 
formatting all at once in another patch.
  
  
  Most comments should already be doxygen friendly in breeze, except that //* 
and /** are used instead of ///

INLINE COMMENTS

> breeze.h:180
> + */
> +// The standard pen stroke width for symbols.
> +static constexpr qreal Symbol = 1.01;

For the record, //* (and /**) should also be caught by Doxygen (and hopefully 
KDevelop). It is used more or less systematically in oxygen, so please use this 
instead of ///

> breezehelper.cpp:1355
>  pen.setJoinStyle( Qt::MiterJoin );
> -pen.setWidthF( 1.1*qMax(1.0, 18.0/rect.width() ) );
> +pen.setWidthF( PenWidth::Symbol*qMax(1.0, 18.0/rect.width() ) );
>  painter->setPen( pen );

Here the penwidth is actually changed (from 1.1 to 1.01) this could affect the 
appearance of the actual buttons. Are you happy with the new appearance ? 
Also, should doublecheck that this is consistent with button rendering in the 
decoration.

In fact for the sake of changing only one thing at a time, I would set 
penwidth::Symbol to 1.1

REPOSITORY
  R31 Breeze

BRANCH
  replace-hardcoded (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26217

To: ndavis, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26094: Add shadow rendering helper functions

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.


  Looks good and sensible. Very nice consolidation. Thanks !

REPOSITORY
  R31 Breeze

BRANCH
  replace-hardcoded (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26094

To: ndavis, #breeze, hpereiradacosta, #plasma
Cc: ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D19890: Reduce the indicator arrow size for press-and-hold menus in QToolButtons

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  As a side remark: did you check how it looks when one selects "text under 
icon" or "text beside icon" for toolbar button ?

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D19890

To: hallas, #vdg, #breeze, ngraham, abetts, ndavis
Cc: ndavis, abetts, hpereiradacosta, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, ahiemstra, mart

D26001: Fix rubberband selection outline position

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.


  Fix is legit. Thanks ! 
  I would avoid changing the alpha of the color though because: it makes no 
difference, and it is unrelated to the issue. Should be a different commit 
(which you can do without review if you really think it is important)
  
  Fix it then ship it !

REPOSITORY
  R31 Breeze

BRANCH
  fix-rubberband (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26001

To: ndavis, #vdg, #breeze, #plasma, hpereiradacosta, broulik
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, cblack, konkinartem, 
ian, jguidon, hannahk, Ghost6, jraleigh, MrPepe, fbampaloukas, squeakypancakes, 
alexde, IohannesPetros, GB_2, trickyricky26, ragreen, mglb, crozbo, ndavis, 
ZrenBot, firef, ngraham, alexeymin, skadinna, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, ahiemstra, 
mbohlender, mart

D19890: Reduce the indicator arrow size for press-and-hold menus in QToolButtons

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D19890#577204 , @ndavis wrote:
  
  > Ok, so there is an overlap problem, but it's quite rare. It happens when an 
icon uses 100% of the available space in the bottom right corner (or left with 
RTL, I think).
  >  Here I changed the stop icon in KDevelop to the icon for Codelite: 
F7817068: Screenshot_20191213_171219.png .
  >
  > I could get around this in a number of ways that all have their downsides:
  >  1/ Move the arrow to the very bottom right and make sure it's small enough 
that no clipping happens
  >
  > - This looks bad and makes it harder to see the arrow 2/ Reduce the default 
icon size for flat buttons
  > - This is more consistent with non-flat buttons, but I like the bigger 
icons. They're more readable too.
  
  
  
  
  > 3/ Increase the padding around flat buttons
  > 
  > - I like the current padding 4/ Ignore the problem
  > - Will look fine with most icon themes, but bad in some cases such as the 
one described above.
  
  Confused about 2: I thought icon size was the same for flat and non flat 
toolbuttons (see oxygen-demo5, buttons tab)
  About 3: really I think that would be bad
  I too think that either 4 (ignoring the problem) or 5: don't change the 
current code, and dont revert the revert, is the way to go.
  for 4: only a few icons would be affected, and only for toolbuttons with long 
popup delay (the only case for which this arrow is drawn). 
  These are already rather rare, and considered bad UI anyway if I remember 
right.
  
  Please pretty please, do not (re) add this arrow (whether tiny or large) to 
instant-popup toolbuttons ...
  
  Hugo

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D19890

To: hallas, #vdg, #breeze, ngraham, abetts, ndavis
Cc: ndavis, abetts, hpereiradacosta, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, ahiemstra, mart

D25691: [KDecoration] Use QIcon::paint

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.


  Thanks !

REPOSITORY
  R113 Oxygen Theme

REVISION DETAIL
  https://phabricator.kde.org/D25691

To: broulik, #plasma, hpereiradacosta
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25593: [kdecoration] Use QVariantAnimation instead of QPropertyAnimation

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.


  Thanks !

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25593

To: broulik, #vdg, hpereiradacosta, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25543: Split Style & Helper into files by widget type

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Also, this change will make it pretty difficult to use git blame and git 
bisect in the future to track possible regressions. (to my knowledge at least). 
Might as well start a new repository.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25543

To: ndavis, #plasma, #breeze
Cc: hpereiradacosta, ngraham, IlyaBizyaev, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D25543: Split Style & Helper into files by widget type

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Also (and somewhat independently from this patch): with Qt6/KF6 being around 
the corner, I somewhat wonder about the utility of rewritting and hacking 
breeze at that time. Would this make sense to actually leave breeze (which is a 
rather well-working theme, with very little bug reports) unchanged, and start 
working anew on a new widget style targeting KF6 ? Would that not be more 
exciting, bring more people in, trigger new ideas, give more freedom ? And be a 
more ambitious task all in all ?  
  One could of course start from breeze, and apply all the new ideas about how 
the code should be organized, how highlight should look, and checkboxes, 
without disturbance for something which has lived largely unchanged (and with 
not so many complains) for all the kf5 lifetime ...
  
  My two cents.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25543

To: ndavis, #plasma, #breeze
Cc: hpereiradacosta, ngraham, IlyaBizyaev, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D25543: Split Style & Helper into files by widget type

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  You are missing copyright information and license in all the newly created 
files. 
  On the review side: it is impossible to actually review, right ? 
  As for the conceptual side: I fear this is addressing a non existing issue, 
and giving a wrong impression about how one should hack on a widget style. It 
is wrong to think that you can hack on a widget style widget by widget without 
consideration about how they should appear one with respect to the others, how 
they should align one with respect to the other, and how the other widgets are 
implemented. You do need to know the whole code and interplay before starting 
to hack anyway. The splitting does not change this. In the end it might just 
result in a lot of duplicated code.
  
  Anyway. I wont prevent you for pushing this. Who does the job decides. This 
probably marks the end of my contributions to breeze though, but they have not 
been that many lately anyway.
  
  Hugo

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25543

To: ndavis, #plasma, #breeze
Cc: hpereiradacosta, ngraham, IlyaBizyaev, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D24008: Make renderDialGroove() area match the maximum renderDialContents() area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  on the other hand, after checking that the dials keep the old appearance when 
"wrapping" is turned on, and since dials are rather seldom used anyway, I have 
no strong feeling against the change (still prefer the old look though)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D24008

To: ndavis, #vdg, #breeze, ngraham
Cc: hpereiradacosta, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24008: Make renderDialGroove() area match the maximum renderDialContents() area

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Problem with the new design if you ask me is that it does not convey the 
information that you can roll around past the maximum as in a circle anymore.
  All other widget styles (except now breeze), use a circle metaphor for a dial 
... Personally I think the new look just looks ... broken. It looks like a bent 
scrollbar, (or a bent slider), which a dial is not ...

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D24008

To: ndavis, #vdg, #breeze, ngraham
Cc: hpereiradacosta, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D23296: Simplify rendering of raised toolbuttons with menu

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D23296#515696 , @ndavis wrote:
  
  > Hmm. I was thinking about using the button background of the dropdown menu 
for something like this mockup: F7265921: Screenshot_20190820_203213.png 

  
  
  Fair enough. In that case you do need the separate rendering, and thus the 
current simplification makes no sense. I can abandon this revision if you want.
  
  Hugo

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D23296

To: hpereiradacosta, #breeze, ndavis
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23296: Simplify rendering of raised toolbuttons with menu

[Hugo Pereira Da Costa]

hpereiradacosta created this revision.
hpereiradacosta added reviewers: Breeze, ndavis.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
hpereiradacosta requested review of this revision.

REVISION SUMMARY
  Following on https://phabricator.kde.org/D23169, it turns out one can 
simplify the toolbuttons with menu rendering strongly by rendering the whole 
frame at once, and removing the code for detecting the presence of the menu 
alltogether. This makes for cleaner code with no visual difference with respect 
to how it should look (and no 'double-shadow').

TEST PLAN
  F7264651: Screenshot_20190820_174107.png 

  This is how toolbuttons with menu appear with this patch. (no change with 
respect to without)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D23296

AFFECTED FILES
  kstyle/breezestyle.cpp

To: hpereiradacosta, #breeze, ndavis
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23169: Fix width and separator of ToolButtonComplexControl outline w/ dropdown menu

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Thanks !

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D23169

To: ndavis, #vdg, #breeze, ngraham, hpereiradacosta
Cc: hpereiradacosta, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D23169: Fix width and separator of ToolButtonComplexControl outline w/ dropdown menu

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Zoom showing the issue mentionned above with the "current" patch (or master 
branch of breeze)
  
  F7250494: Screenshot_20190816_175237.png 

  
  How it should look: 
  F7250505: Screenshot_20190816_175628.png 


REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D23169

To: ndavis, #vdg, #breeze, ngraham, hpereiradacosta
Cc: hpereiradacosta, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D23169: Fix width and separator of ToolButtonComplexControl outline w/ dropdown menu

[Hugo Pereira Da Costa]

hpereiradacosta reopened this revision.
hpereiradacosta added a comment.
This revision is now accepted and ready to land.


  F7250479: patch.diff 

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D23169

To: ndavis, #vdg, #breeze, ngraham
Cc: hpereiradacosta, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D23169: Fix width and separator of ToolButtonComplexControl outline w/ dropdown menu

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Hi Noah
  Thanks for the patch, however, it is not the right fix to the issue. If you 
use a light color scheme (like the default breeze), you will see that the 
shadow below the part of the button that corresponds to the arrow is darker 
than below the rest of the button. This is because the frame is actually 
rendered twice.
  
  Now, the bug you try to fix is real, and as I was 100% sure that it was not 
there in the past, I used git bisect to track it down to this commit:
  
  32d8b02880a237e6de415861500a018a5cd09781 

  
  The corresponding diff contains 
  @@ -5988,7 +5988,6 @@ namespace Breeze
  
// frame
if( toolButtonOption->subControls & SC_ToolButton )
{
  
  - copy.rect = buttonRect; if( inTabBar ) drawTabBarPanelButtonToolPrimitive( 
, painter, widget ); else drawPrimitive( PE_PanelButtonTool, , 
painter, widget); }
  
  Which is what causes the issue. 
  Could you revert this commit, and push instead the proper fix that I will 
post in another comment ? 
  Thanks !

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D23169

To: ndavis, #vdg, #breeze, ngraham
Cc: hpereiradacosta, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D22851: [SplitterProxy] Don't manually mapToGlobal

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Thanks for the fix ! 
  Oxygen needs the same patch. Will you also push it there ?
  
  Hugo

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D22851

To: broulik, #plasma, hpereiradacosta, davidedmundson
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22138: Remove 1 pixel margin around side panels, use QPalette::Base for background

[Hugo Pereira Da Costa]

This revision was automatically updated to reflect the committed changes.
Closed by commit R31:d6b2a3a36a1c: Remove unneeded 1 pixel margin around side 
panels (authored by hpereiradacosta).

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22138?vs=61228=61236

REVISION DETAIL
  https://phabricator.kde.org/D22138

AFFECTED FILES
  kstyle/breezehelper.cpp
  kstyle/breezestyle.cpp
  kstyle/breezestyle.h

To: hpereiradacosta, mart, #vdg, ndavis, filipf
Cc: ngraham, ndavis, filipf, mart, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D22138: Remove 1 pixel margin around side panels, use QPalette::Base for background

[Hugo Pereira Da Costa]

hpereiradacosta updated this revision to Diff 61228.
hpereiradacosta added a comment.


  Simplified the patch: special case in ::pixelMetrics is in fact not needed, 
because it is overridden in ::frameContentsRect anyway

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22138?vs=61227=61228

REVISION DETAIL
  https://phabricator.kde.org/D22138

AFFECTED FILES
  kstyle/breezehelper.cpp
  kstyle/breezestyle.cpp
  kstyle/breezestyle.h

To: hpereiradacosta, mart, #vdg, ndavis, filipf
Cc: ngraham, ndavis, filipf, mart, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D22138: Remove 1 pixel margin around side panels, use QPalette::Base for background

[Hugo Pereira Da Costa]

hpereiradacosta updated this revision to Diff 61227.
hpereiradacosta added a comment.


  New patch, adding proper margin to avoid overlap with viewport

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22138?vs=60773=61227

REVISION DETAIL
  https://phabricator.kde.org/D22138

AFFECTED FILES
  kstyle/breezehelper.cpp
  kstyle/breezestyle.cpp
  kstyle/breezestyle.h

To: hpereiradacosta, mart, #vdg, ndavis, filipf
Cc: ngraham, ndavis, filipf, mart, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D22138: Remove 1 pixel margin around side panels, use QPalette::Base for background

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D22138#491285 , @mart wrote:
  
  > In D22138#491173 , 
@hpereiradacosta wrote:
  >
  > > Indeed. This is a problem. As soon as treeviews are animated Qt makes a 
pixmap copy of the treeview viewport. This does not include the blue line on 
the side, which is painted in the frame, below the (transparent) viewport. Will 
investigate further.
  >
  >
  > would still adding a pixel on the right work around the issue?
  
  
  Yes, left or right depending of RTL. I have a working patch (I think). Will 
upload in a minute.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D22138

To: hpereiradacosta, mart, #vdg, ndavis, filipf
Cc: ngraham, ndavis, filipf, mart, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol