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

Authored by hpereiradacosta on Jun 28 2019, 12:29 PM.

Details

Summary

Remove unneeded 1 pixel margin around side panels, namely QAbstractScrollArea with property _kde_side_panel_view set to true.
In order to be able to still draw a vertical line on the side of the list, a one pixel margin is kept, on this side only, using SE_FrameContent subelementRect
The viewport background is kept to QPalette::Base, as for regular lists.
The logic in polishScrollArea has been adjusted accordingly

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Restricted Application added a project: Plasma. · View Herald TranscriptJun 28 2019, 12:29 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
hpereiradacosta requested review of this revision.Jun 28 2019, 12:29 PM
ngraham added subscribers: filipf, ndavis, ngraham.

Thanks @hpereiradacosta! This looks fantastic. Adding @ndavis and @filipf for comment since they've been working on this project too from other angles.

Thanks @hpereiradacosta! This looks fantastic. Adding @ndavis and @filipf for comment since they've been working on this project too from other angles.

No need for thanks @ngraham, I am not doing this for you and this is irrelevant in terms of review.

Umm OK, sorry if I've said something wrong.

ndavis resigned from this revision.Jul 4 2019, 5:23 PM

Since this apparently doesn't need review, I'll resign.

Very nice! In conjunction with Marco's patch (D22083), I now see the following for Dolphin's settings window:

I notice that your screenshot depicts the sidebar with no top, bottom, or left margins, which is the indended appearance. Is that the result of some other required patch I haven't applied yet, or did I do something wrong?

Since this apparently doesn't need review,

This is not what I said. What I said is "thanks, it looks fantastic" is not a proper review and hence irrelevant. What I expect for a proper review is

  • does the patch achieve the behavior matching its title, and is it the intended behavior
  • does the code look legit (compiles, properly formatted, etc.)
  • is there a better way to achieve the same
  • does the patch break other things, and how can it be improved so that it doesn't

If you can provide such a review I'd be happy to modify the patch accordingly.

I'll resign.

hpereiradacosta added a comment.EditedJul 4 2019, 6:46 PM

Very nice! In conjunction with Marco's patch (D22083), I now see the following for Dolphin's settings window:

I notice that your screenshot depicts the sidebar with no top, bottom, or left margins, which is the indended appearance. Is that the result of some other required patch I haven't applied yet, or did I do something wrong?

Sorry. My bad. Yes, my own copy included other changes beyond my patch and Marco's, resulting in the zero margins. The screenshot you post is the correct one. Thanks.
The remaining margin issue, handled at the layout level, either in breeze or in kpagedialog, should be a separate patch.

The purpose of this one is to fix the margin issues which cannot be handled at the layout level.

ndavis added a comment.Jul 4 2019, 7:42 PM

Since this apparently doesn't need review,

This is not what I said. What I said is "thanks, it looks fantastic" is not a proper review and hence irrelevant. What I expect for a proper review is

  • does the patch achieve the behavior matching its title, and is it the intended behavior
  • does the code look legit (compiles, properly formatted, etc.)
  • is there a better way to achieve the same
  • does the patch break other things, and how can it be improved so that it doesn't If you can provide such a review I'd be happy to modify the patch accordingly.

I'll resign.

In that case, I will review this.

ndavis accepted this revision.Jul 5 2019, 3:58 AM

As far as I can tell, there aren't any problems with this patch. Some apps like Spectacle and the system tray settings don't follow the new style, but they don't use Qt Widgets for the sidebar.

This revision is now accepted and ready to land.Jul 5 2019, 3:58 AM
ndavis requested changes to this revision.Jul 5 2019, 4:05 AM

Actually, I did find one problem that I missed from before:

This revision now requires changes to proceed.Jul 5 2019, 4:05 AM

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.

mart added a comment.Jul 5 2019, 11:03 AM

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?

In D22138#491285, @mart 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.

New patch, adding proper margin to avoid overlap with viewport

hpereiradacosta edited the summary of this revision. (Show Details)Jul 5 2019, 3:35 PM

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

ndavis accepted this revision.Jul 5 2019, 4:49 PM
This revision is now accepted and ready to land.Jul 5 2019, 4:49 PM
ndavis added a comment.Jul 5 2019, 4:51 PM

Works for me. Tested RTL with kate --reverse.

This revision was automatically updated to reflect the committed changes.