introduce concept of header and footer for kpageview
ClosedPublic

Authored by mart on Jun 24 2019, 4:50 PM.

Details

Summary

put the dialog buttons in the footer for kpagedialog
this is the first step for the new setting dialog windows.
note that the look is still not that, but the changes will have to
come probably from Breeze

Test Plan

tested some application f=config windows

Diff Detail

Repository
R236 KWidgetsAddons
Branch
mart/pageviewfooter
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13999
Build 14017: arc lint + arc unit
mart created this revision.Jun 24 2019, 4:50 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 24 2019, 4:50 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
mart requested review of this revision.Jun 24 2019, 4:50 PM
mart added a comment.Jun 24 2019, 4:51 PM

white background and part of margins removal will have to probably be in breeze style

ngraham accepted this revision as: VDG.Jun 25 2019, 4:01 PM
ngraham added a subscriber: ngraham.

This seems to work quite well. For the remaining padding on the left, top, and bottom, does that come from Breeze, or would it need to be removed here? Or does it come from here and we should work around it in Breeze?

ngraham added a subscriber: ndavis.Jun 25 2019, 4:13 PM
cfeck added a subscriber: cfeck.Jun 25 2019, 4:32 PM
cfeck added inline comments.
src/kpageview.cpp
472

This check is duplicated.

490

Please remove this empty line.

src/kpageview.h
163

Does this transfer ownership of the widget? If yes, is ownership transferred back for any previously set widget? Or is it even automatically deleted? It needs to be clarified in the docs.

src/kpageview_p.h
81

No space before <

On the breeze side, asside from the layout margins/spacing issues, there is at least one hard-coded pixel in the frame rendering that must be removed. In fact I have this committed in my local breeze version already. The corresponding patch is rather simple and attached. Feel free to add this or something similar to any other modification you plan to do.

mart added a comment.Jun 28 2019, 8:57 AM

On the breeze side, asside from the layout margins/spacing issues, there is at least one hard-coded pixel in the frame rendering that must be removed. In fact I have this committed in my local breeze version already. The corresponding patch is rather simple and attached. Feel free to add this or something similar to any other modification you plan to do.

Thank you very much Hugo.. that pixel was driving me mad ;)
Do you want to push it yourself or I just include it in the breeze modifications I'll do for the sidebar style?

hpereiradacosta added a comment.EditedJun 28 2019, 9:25 AM

ather simple and attached. Feel free to add this or something similar to any other modification you plan to do.

Thank you very much Hugo.. that pixel was driving me mad ;)
Do you want to push it yourself or I just include it in the breeze modifications I'll do for the sidebar style?

Well, it turns out that things are more complicated than anticipated. (it always is, right ?)
As long as the sidebar is kept "transparent", the patch works. Now if you put back the sidebar background to its original white, it does not. Reason is that the viewport now coincides completely with the frame, and its background overlaps the left side vertical line. Result: the side vertical line is only visible as long as you keep the viewport transparent.
So all in all: the patch goes in the right direction for what you intend to do ultimately but needs refinement.
In the current code, the margins are (1,1,1,1). With my patch they are (0,0,0,0), and ultimately what you want is (0,0,1,0) or (1,0,0,0), depending on RTL.
I'll try to investigate some more to fix this overlap issue, and then will submit a PR

mart added a comment.Jul 1 2019, 10:35 AM

ather simple and attached. Feel free to add this or something similar to any other modification you plan to do.

Thank you very much Hugo.. that pixel was driving me mad ;)
Do you want to push it yourself or I just include it in the breeze modifications I'll do for the sidebar style?

Well, it turns out that things are more complicated than anticipated. (it always is, right ?)
As long as the sidebar is kept "transparent", the patch works. Now if you put back the sidebar background to its original white, it does not. Reason is that the viewport now coincides completely with the frame, and its background overlaps the left side vertical line. Result: the side vertical line is only visible as long as you keep the viewport transparent.
So all in all: the patch goes in the right direction for what you intend to do ultimately but needs refinement.
In the current code, the margins are (1,1,1,1). With my patch they are (0,0,0,0), and ultimately what you want is (0,0,1,0) or (1,0,0,0), depending on RTL.
I'll try to investigate some more to fix this overlap issue, and then will submit a PR

thank you very much
(yes the goal is to make it white again once the rest is done)

Hi Marco,
see https://phabricator.kde.org/D22138
(I also included the white background in there, because it turned out a bit tricky).

mart updated this revision to Diff 61206.Jul 5 2019, 11:14 AM
  • introduce concept of header and footer for kpageview
  • adress code style comments
This revision was not accepted when it landed; it landed in state Needs Review.Jul 5 2019, 11:15 AM
This revision was automatically updated to reflect the committed changes.
mart marked 3 inline comments as done.
mart added a comment.Jul 5 2019, 11:16 AM

code issues are fixed.
Now for removing the further margin around the whole window:
should that be done here or in breeze?
if done here, it may make it look bad on all other widget styles?

mart reopened this revision.Jul 5 2019, 11:16 AM
In D22083#491299, @mart wrote:

code issues are fixed.
Now for removing the further margin around the whole window:
should that be done here or in breeze?
if done here, it may make it look bad on all other widget styles?

Good question. Either way is fine with me.
You have more flexibility if you fix it in the widget than in the style (for instance if you change the margins of the layout in the style, this will propagate to sublayouts, and you might have a hard time to change the sublayouts back), but then you might need to test on the widget style name to not break other themes.
When changing on the style side, you need a check on the widget type.
Also, the best place to change on the style side is probably in ::pixelMetrics (PM_LayoutLeftMargin and the likes), rather than hacking contentsMargins directly in ::polish
In either case this probably need some experimenting ...

mart added a comment.Jul 9 2019, 11:04 AM

can this part go forward?
I think ui-wise looks fine even if the other parts won't make it for release? (still hope they will tough :)

Yep, I think so.

I'd like to get the extra margins removed as well, but this is a good incremental step, and given how D22138 already landed, I think it's better to have this in.

mart updated this revision to Diff 61844.Jul 16 2019, 9:49 AM
  • introduce concept of header and footer for kpageview
  • adress code style comments
  • use rowspan and colspan to add margins in the right places
mart added a comment.Jul 16 2019, 9:52 AM

this makes it look almost pixel perfect

tough introduces a behavior change: moves the responsibility of adding margins from the outer layout to the internal one, making some users look weird. here kcmshell not adapted

ngraham accepted this revision.Jul 16 2019, 6:45 PM

Lovely. Looks perfect in Dolphin, Kate, Konsole, Okular, Gwenview, and Kile.

Ark and Spectacle also need to be adapted, like kcmshell. Anything not using KPageView properly will need some adjustment, I guess. It's probably good that we're doing this because it provides a good opportunity to fix them and unify things.

This revision is now accepted and ready to land.Jul 16 2019, 6:45 PM
mart added a comment.Jul 17 2019, 7:26 AM

Lovely. Looks perfect in Dolphin, Kate, Konsole, Okular, Gwenview, and Kile.

Ark and Spectacle also need to be adapted, like kcmshell. Anything not using KPageView properly will need some adjustment, I guess. It's probably good that we're doing this because it provides a good opportunity to fix them and unify things.

one thing that concerns me is that things that need to be adapted now get a double margin.
tough i guess if we remove margins completely from the dialogs it doesn't look that good anymore...

Hi Marco,
Patch works like a charm in standard config dialogs.
As a somewhat off topic remark though, it does not work in kcmultidialog (in framework kcmutils), even though it derives from kpageddialog.
This shows up for instance in the "window manager settings" you can get from the decoration menu (Alt+F3), or simply when running oxygen-setting or breeze-settings.
Looking into kcmultidialog, it seems there is some internal handling of margins in there, for a reason unknown to me. Maybe you want to investigate there too (in a different patch)

Hugo

This revision was automatically updated to reflect the committed changes.
mart added a comment.Jul 25 2019, 4:50 PM

Looking into kcmultidialog, it seems there is some internal handling of margins in there, for a reason unknown to me. Maybe you want to investigate there too (in a different patch)

Hugo

It should look better now