After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 746108 - make modal dialog buttons follow the same gapless design that gtk+ does
make modal dialog buttons follow the same gapless design that gtk+ does
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2015-03-12 19:15 UTC by Jakub Steiner
Modified: 2015-08-05 12:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
modalDialog: Don't make it fill the parent (1.18 KB, patch)
2015-07-29 11:52 UTC, Carlos Soriano
reviewed Details | Review
modalDialog: Match gtk+ buttons style (21.09 KB, patch)
2015-07-29 11:52 UTC, Carlos Soriano
none Details | Review
gnome-shell-sass patch (3.06 KB, patch)
2015-07-29 12:25 UTC, Carlos Soriano
none Details | Review
modalDialog: Match gtk+ buttons style (29.77 KB, patch)
2015-07-31 16:44 UTC, Carlos Soriano
none Details | Review
modalDialog: Match gtk+ buttons style (29.77 KB, patch)
2015-07-31 16:45 UTC, Carlos Soriano
none Details | Review
theme: Match gtk+ modal dialog buttons style (3.09 KB, patch)
2015-07-31 16:46 UTC, Carlos Soriano
committed Details | Review
modalDialog: Match gtk+ buttons style (29.56 KB, patch)
2015-08-04 14:12 UTC, Carlos Soriano
committed Details | Review

Description Jakub Steiner 2015-03-12 19:15:20 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+
Comment 1 Carlos Soriano 2015-07-29 11:52:32 UTC
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.
Comment 2 Carlos Soriano 2015-07-29 11:52:40 UTC
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.
Comment 3 Jakub Steiner 2015-07-29 12:15:30 UTC
This is fantastic.
Comment 4 Carlos Soriano 2015-07-29 12:25:54 UTC
Created attachment 308389 [details] [review]
gnome-shell-sass patch
Comment 5 Florian Müllner 2015-07-29 16:51:19 UTC
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?
Comment 6 Florian Müllner 2015-07-29 16:51:31 UTC
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 ...
Comment 7 Carlos Soriano 2015-07-31 16:44:48 UTC
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.
Comment 8 Carlos Soriano 2015-07-31 16:45:18 UTC
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.
Comment 9 Carlos Soriano 2015-07-31 16:46:46 UTC
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.
Comment 10 Florian Müllner 2015-07-31 18:02:50 UTC
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?
Comment 11 Florian Müllner 2015-07-31 18:04:11 UTC
Review of attachment 308571 [details] [review]:

Looks good to me, but might want to run this by Jakub before pushing
Comment 12 Jakub Steiner 2015-08-01 00:28:11 UTC
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.
Comment 13 Florian Müllner 2015-08-01 00:42:26 UTC
(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.
Comment 14 Jakub Steiner 2015-08-03 11:08:22 UTC
Thumbs up form me.
Comment 15 Carlos Soriano 2015-08-04 14:12:24 UTC
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.
Comment 16 Carlos Soriano 2015-08-04 14:15:39 UTC
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 17 Florian Müllner 2015-08-05 12:41:55 UTC
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
Comment 18 Florian Müllner 2015-08-05 12:46:47 UTC
Attachment 308739 [details] pushed as 0722c06 - modalDialog: Match gtk+ buttons style