Allow OverlaySheet clients to omit the built-in close button

Authored by ngraham on Mar 10 2018, 12:08 AM.



Most of the time we want an OverlaySheet to draw a close button so its clients don't have to. But sometimes clients want to draw their own close buttons, for example to use the OverlaySheet as a dialog box with a Cancel button (which we do in Discover). In these circumstances, we don't need OverlaySheet to draw its own close button; this patch makes that possible.

Test Plan

Tested in Discover after applying the patch and adding showCloseButton: false to DiscoverWindow.qml's proceedDialog:

Diff Detail

R169 Kirigami
overlay-sheet-without-close-button (branched from master)
No Linters Available
No Unit Test Coverage
ngraham created this revision.Mar 10 2018, 12:08 AM
Restricted Application added a project: Kirigami. · View Herald TranscriptMar 10 2018, 12:08 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Mar 10 2018, 12:08 AM
ngraham retitled this revision from Allow OverlaySheets to omit built-in close buttons to Allow OverlaySheet clients to omit the built-in close button.Mar 10 2018, 12:09 AM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham updated this revision to Diff 29130.Mar 10 2018, 12:30 AM

Fix whitespace

apol added a comment.Mar 10 2018, 1:48 AM

I'd say it's okay to have the close button and a cancel, every dialog does.

"Close button + cancel" is inherently redundant; a historical relic of the fact that dialog boxes have traditionally been separate windows, and windows have titlebars with fixed button arrangements (that said, in macOS, dialog boxes now have no window decorations). Having two visible methods of doing the exact same thing is pointless and confusing, and IMHO there's no reason to go out of our way to replicate this error now that we're using a convergent UI toolkit that has inline sheets.

mart requested changes to this revision.Mar 12 2018, 3:32 PM

I'm ok with it, tough i would change a bit the logic, as described


property alias showCloseButton: closeIcon.visible


with the alias then we again have only:
visible: !Settings.isMobile

in this case, the default behavior of showCloseButton will be !Settings.isMobile, and then the developer could always override the behavior, breaking the binding to fore it always true or always false (so the property will work even on mobile)

This revision now requires changes to proceed.Mar 12 2018, 3:32 PM
ngraham updated this revision to Diff 29359.Mar 12 2018, 10:38 PM

Make the property a simple alias

ngraham marked 2 inline comments as done.Mar 12 2018, 10:38 PM
mart accepted this revision.Mar 19 2018, 4:13 PM
This revision is now accepted and ready to land.Mar 19 2018, 4:13 PM
ngraham closed this revision.Mar 19 2018, 4:23 PM