D22083: introduce concept of header and footer for kpageview

[Marco Martin]

mart added a comment.


  In D22083#498499 , 
@hpereiradacosta wrote:
  
  > 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

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg, ngraham
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
sbergeron, michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R236:b581c5f990a6: introduce concept of header and footer for 
kpageview (authored by mart).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D22083?vs=61844=62554#toc

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22083?vs=61844=62554

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

AFFECTED FILES
  src/kpagedialog.cpp
  src/kpagedialog_p.h
  src/kpageview.cpp
  src/kpageview.h
  src/kpageview_p.h

To: mart, #plasma, #frameworks, #vdg, ngraham
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
sbergeron, michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Nathaniel Graham]

ngraham added a task: T11279: Unify settings windows' sidebar appearance.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  mart/pageviewfooter

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

To: mart, #plasma, #frameworks, #vdg, ngraham
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
sbergeron, michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  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

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  mart/pageviewfooter

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

To: mart, #plasma, #frameworks, #vdg, ngraham
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
sbergeron, michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

mart added a comment.


  In D22083#496446 , @ngraham wrote:
  
  > 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...

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  mart/pageviewfooter

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

To: mart, #plasma, #frameworks, #vdg, ngraham
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
sbergeron, michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Nathaniel Graham]

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


  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.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  mart/pageviewfooter

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

To: mart, #plasma, #frameworks, #vdg, ngraham
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
sbergeron, michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

mart added a comment.


  this makes it look almost pixel perfect
  F7031708: Screenshot_20190716_115035.png 

  
  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
  F7031712: Screenshot_20190716_115148.png 


REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
sbergeron, michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

mart updated this revision to Diff 61844.
mart added a comment.


  - introduce concept of header and footer for kpageview
  - adress code style comments
  - use rowspan and colspan to add margins in the right places

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22083?vs=61207=61844

BRANCH
  mart/pageviewfooter

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

AFFECTED FILES
  src/kpagedialog.cpp
  src/kpagedialog_p.h
  src/kpageview.cpp
  src/kpageview.h
  src/kpageview_p.h

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
sbergeron, michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

mart added a comment.


  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 :)

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Nathaniel Graham]

ngraham added a task: T11153: Unify sidebar appearance.

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  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 ...

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

mart added a comment.


  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?

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

mart reopened this revision.

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
mart marked 3 inline comments as done.
Closed by commit R236:fd5899d49d01: adress code style comments (authored by 
mart).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D22083?vs=61206=61207#toc

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22083?vs=61206=61207

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

AFFECTED FILES
  src/kpageview.cpp
  src/kpageview_p.h

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

mart updated this revision to Diff 61206.
mart added a comment.


  - introduce concept of header and footer for kpageview
  - adress code style comments

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22083?vs=60603=61206

BRANCH
  mart/pageviewfooter

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

AFFECTED FILES
  src/kpagedialog_p.h
  src/kpageview.cpp
  src/kpageview.h
  src/kpageview_p.h

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


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

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

mart added a comment.


  In D22083#487566 , 
@hpereiradacosta wrote:
  
  > ather simple and attached. Feel free to add this or something similar to 
any other modification you plan to do. F6938398: patch.diff 

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

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


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

  
  > 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,0,1) or (0,1,0,0), depending on 
RTL. 
  I'll try to investigate some more to fix this overlap issue, and then will 
submit a PR

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

mart added a comment.


  In D22083#487307 , 
@hpereiradacosta wrote:
  
  > 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. F6938398: 
patch.diff 
  
  
  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?

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  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. F6938398: 
patch.diff 

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: hpereiradacosta, cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kpageview.cpp:458
> +
> +if (d->pageHeader == header) {
> +return;

This check is duplicated.

> kpageview.cpp:476
> +}
> +
> +}

Please remove this empty line.

> kpageview.h:163
> +/**
> + * Set a widget as the header for this Page view
> + * It will replace the standard page title

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.

> kpageview_p.h:81
>  
> +QPointer  pageHeader;
> +QPointer  pageFooter;

No space before `<`

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: cfeck, ndavis, ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Nathaniel Graham]

ngraham added a subscriber: ndavis.

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: ndavis, ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Nathaniel Graham]

ngraham accepted this revision as: VDG.
ngraham added a comment.


  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?

REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

mart added a comment.


  white background and part of margins removal will have to probably be in 
breeze style
  F6931480: Screenshot_20190624_184812.png 


REPOSITORY
  R236 KWidgetsAddons

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

To: mart, #plasma, #frameworks, #vdg
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22083: introduce concept of header and footer for kpageview

[Marco Martin]

mart created this revision.
mart added reviewers: Plasma, Frameworks, VDG.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
mart requested review of this revision.

REVISION 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

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  mart/pageviewfooter

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

AFFECTED FILES
  src/kpagedialog_p.h
  src/kpageview.cpp
  src/kpageview.h
  src/kpageview_p.h

To: mart, #plasma, #frameworks, #vdg
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns