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 <https://phabricator.kde.org/F8185479>
> Without: F8185480: image.png <https://phabricator.kde.org/F8185480>

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