GNOME Bugzilla – Bug 746108
make modal dialog buttons follow the same gapless design that gtk+ does
Last modified: 2015-08-05 12:46:53 UTC
Probably a post 3.16 tweak, but it would be nice to have the same design for modal dialogs buttons that we sport in gtk+
Created attachment 308384 [details] [review] modalDialog: Don't make it fill the parent We don't actually want the modal dialog to fill the full screen. This was being triggered by some relayout if some o the children were expanding.
Created attachment 308385 [details] [review] modalDialog: Match gtk+ buttons style Follow the design we have in gtk+ for buttons dialogs, which are at the bottom and they expand full width, having the same amount of space for each one. Also, since this removes any space for non-button widgets in the button area, move the spinner present in the auth prompt dialog next to the password entry.
This is fantastic.
Created attachment 308389 [details] [review] gnome-shell-sass patch
Review of attachment 308384 [details] [review]: This change does affect more than just the visible dialogLayout - are you sure the eventBlocker still works as intended after this?
Review of attachment 308385 [details] [review]: ::: data/theme/gnome-shell.css @@ +57,3 @@ + +.dialog-button-box { + height: 44px; } Why a fixed height? At the least, it should scale with the text IMO, but can't you achieve just the same with setting appropriate padding on the buttons? ::: js/ui/components/keyring.js @@ +59,3 @@ y_align: St.Align.START }); + this._addSpinner (this._messageBox); addSpinner(), not _addSpinner() Also: can't we find a better place to put it? In the polkit dialog, the new position is an improvement IMHO. Here it's just ... weird. ::: js/ui/modalDialog.js @@ +111,3 @@ + this.buttonLayout = new St.Widget ({ layout_manager: new Clutter.BoxLayout ({ homogeneous:true }), + style_class: "dialog-button-box" }); Any particular reason for dropping the 'modal' prefix? @@ +175,3 @@ + y_expand: true, + x_align: St.Align.MIDDLE, + y_align: St.Align.MIDDLE, Note that MIDDLE is already the default for StBin:x-align/StBin:y-align @@ +195,3 @@ }, + addSpinner: function(container) { This API is awkward - this.addThing(arg) means "add arg to this" everywhere, so turning this into "create something and add it to arg" is confusing. Something like createSpinner() would be a slightly better, though returning an object that is still mostly managed by the creator is also odd. I'd just move this into the keyring and polkit dialogs - if you are really worried about code sharing (though there are already other places where we show/hide a spinner), we could add Animation.Spinner(size) and show()/hide() methods (with optional fade) to AnimatedIcon ...
Created attachment 308569 [details] [review] modalDialog: Match gtk+ buttons style Follow the design we have in gtk+ for buttons dialogs, which are at the bottom and they expand full width, having the same amount of space for each one. Also, since this removes any space for non-button widgets in the button area, move the spinner present in the auth prompt dialog next to the password entry.
Created attachment 308570 [details] [review] modalDialog: Match gtk+ buttons style Follow the design we have in gtk+ for buttons dialogs, which are at the bottom and they expand full width, having the same amount of space for each one. Also, since this removes any space for non-button widgets in the button area, move the spinner present in the auth prompt dialog next to the password entry.
Created attachment 308571 [details] [review] theme: Match gtk+ modal dialog buttons style Only drawbackis we need a little workaround in _drawing for an issue with box-shadow. See bug 752934 for context.
Review of attachment 308570 [details] [review]: ::: js/ui/components/keyring.js @@ +83,3 @@ + this._workSpinner = new Animation.AnimatedIcon(spinnerIcon, WORK_SPINNER_ICON_SIZE); + this._workSpinner.actor.opacity = 0; + this._workSpinner.actor.show(); Clutter actors are visible by default @@ +135,3 @@ this._passwordEntry.clutter_text.connect('activate', Lang.bind(this, this._onPasswordActivate)); + this._createSpinner(); It's a bit odd to first create the password entry inline (7 lines) and then use a separate method to set up the spinner (3 lines) - just inline the code. ::: js/ui/components/polkitAgent.js @@ +143,3 @@ this._passwordBox.add(this._passwordEntry, { expand: true }); + this._addSpinner(); Same as in keyring @@ +188,3 @@ + this._workSpinner = new Animation.AnimatedIcon(spinnerIcon, WORK_SPINNER_ICON_SIZE); + this._workSpinner.actor.opacity = 0; + this._workSpinner.actor.show(); Dito ::: js/ui/modalDialog.js @@ +76,3 @@ this.dialogLayout = new St.BoxLayout({ style_class: 'modal-dialog', + x_align: Clutter.ActorAlign.CENTER, + y_align: Clutter.ActorAlign.CENTER, Do we want that snippet on gnome-3-16 as well?
Review of attachment 308571 [details] [review]: Looks good to me, but might want to run this by Jakub before pushing
The stylesheet side of things looks good. I can't seem to be able to apply the patch right now to try live as things have shifted under in master it seems.
(In reply to Jakub Steiner from comment #12) > I can't seem to be able to apply the patch right now The conflict is just the gnome-shell-sass submodule, which you can ignore. Alternatively, I pushed it to origin/wip/csoriano/modal-dialog-button-style.
Thumbs up form me.
Created attachment 308739 [details] [review] modalDialog: Match gtk+ buttons style Follow the design we have in gtk+ for buttons dialogs, which are at the bottom and they expand full width, having the same amount of space for each one. Also, since this removes any space for non-button widgets in the button area, move the spinner present in the auth prompt dialog next to the password entry.
Agree on everything. > Do we want that snippet on gnome-3-16 as well? I would rather not touch something that is already working fine for a stable release. However, as fas as my understanding goes, this actually shouldn't work. But no clue why it worked before I changed the button box to be a StWidget with a ClutterBoxLayout, if before with a St.BoxLayout (which uses ClutterBoxLayout internally), and setting to expand was working as expected. I used St.Widget with a ClutterBoxLayout solely for the support of homogeneous allocation.
Comment on attachment 308571 [details] [review] theme: Match gtk+ modal dialog buttons style Attachment 308571 [details] pushed as e12b124 - theme: Match gtk+ modal dialog buttons style
Attachment 308739 [details] pushed as 0722c06 - modalDialog: Match gtk+ buttons style