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 706208 - Add a confirmation dialog for display changes
Add a confirmation dialog for display changes
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on: 705670
Blocks:
 
 
Reported: 2013-08-17 15:45 UTC by Giovanni Campagna
Modified: 2013-08-20 10:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add a confirmation dialog for display changes (10.08 KB, patch)
2013-08-17 15:45 UTC, Giovanni Campagna
none Details | Review
Screenshot (1.22 MB, image/png)
2013-08-17 15:48 UTC, Giovanni Campagna
  Details
Add a confirmation dialog for display changes (10.57 KB, patch)
2013-08-19 11:09 UTC, Giovanni Campagna
none Details | Review
Add a confirmation dialog for display changes (10.05 KB, patch)
2013-08-19 11:12 UTC, Giovanni Campagna
none Details | Review
Add a confirmation dialog for display changes (10.02 KB, patch)
2013-08-19 22:38 UTC, Giovanni Campagna
reviewed Details | Review
New screenshot (591.92 KB, image/png)
2013-08-19 22:39 UTC, Giovanni Campagna
  Details
Add a confirmation dialog for display changes (10.32 KB, patch)
2013-08-20 08:27 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2013-08-17 15:45:39 UTC
Related, and dependent on, bug 705670, but that one is too
long, better keep this separate.
Comment 1 Giovanni Campagna 2013-08-17 15:45:42 UTC
Created attachment 252050 [details] [review]
Add a confirmation dialog for display changes

Unfortunately, display configuration can and does fail, due
to unspecified HW constraints, drivers bugs, unsupported exotic
configurations or just bad luck.
So when the user makes a change in the control center, show
a dialog asking him if it looks OK, and revert back after 20 seconds
otherwise.
Comment 2 Giovanni Campagna 2013-08-17 15:48:39 UTC
Created attachment 252051 [details]
Screenshot

This is how it looks like - it's a simple toolkit conversion of the Gtk dialog from gnome-settings-daemon.

The left button is truncated, I know.
We can either make the dialog larger (now it's sized like all other modal dialogs), or use a shorter label, eg. "Restore previous"
Comment 3 Frederic Peters 2013-08-19 10:55:06 UTC
Do note translations will exist, with different label sizes. Could the dialog be sized according to the label sizes, instead of a manual adjustment targeting English?
Comment 4 Giovanni Campagna 2013-08-19 11:09:36 UTC
Created attachment 252201 [details] [review]
Add a confirmation dialog for display changes

Unfortunately, display configuration can and does fail, due
to unspecified HW constraints, drivers bugs, unsupported exotic
configurations or just bad luck.
So when the user makes a change in the control center, show
a dialog asking him if it looks OK, and revert back after 20 seconds
otherwise.

For the sake of discussion, this is the version that enlarges
to accomodate for longer buttons.
Comment 5 Giovanni Campagna 2013-08-19 11:12:36 UTC
Created attachment 252202 [details] [review]
Add a confirmation dialog for display changes

Unfortunately, display configuration can and does fail, due
to unspecified HW constraints, drivers bugs, unsupported exotic
configurations or just bad luck.
So when the user makes a change in the control center, show
a dialog asking him if it looks OK, and revert back after 20 seconds
otherwise.

And this is the version that uses labels like Windows 7
(http://h10025.www1.hp.com/ewfrf-JAVA/Doc/images/839/c01891593.gif)
Comment 6 Allan Day 2013-08-19 22:32:11 UTC
In general I like the Windows wording: it is less verbose, and avoids technical terms like "configuration". To keep it GNOMEy, we should have the negative "Undo" action first though. So something like:

Do you want to keep these display settings?

Settings changes will revert in %s seconds

[ Revert Settings ] [ Keep Changes ]
Comment 7 Giovanni Campagna 2013-08-19 22:38:56 UTC
Created attachment 252311 [details] [review]
Add a confirmation dialog for display changes

Unfortunately, display configuration can and does fail, due
to unspecified HW constraints, drivers bugs, unsupported exotic
configurations or just bad luck.
So when the user makes a change in the control center, show
a dialog asking him if it looks OK, and revert back after 20 seconds
otherwise.
Comment 8 Giovanni Campagna 2013-08-19 22:39:29 UTC
Created attachment 252312 [details]
New screenshot
Comment 9 drago01 2013-08-20 05:41:26 UTC
Review of attachment 252311 [details] [review]:

Looks good, just some minor stuff (see below).

::: js/ui/windowManager.js
@@ +56,3 @@
+                         y_align: St.Align.START });
+
+        this._countDown = 20;

Use a constant?

@@ +67,3 @@
+                         y_align: St.Align.START });
+
+        this._cancelButton = this.addButton({ label: _("Revert Settings"),

Add a translator comment that warns about the length.

@@ +71,3 @@
+                                              key: Clutter.Escape },
+                                            { expand: true, x_fill: false, x_align: St.Align.START });
+        this._okButton = this.addButton({ label:  _("Keep Changes"),

Add a translator comment that warns about the length.

@@ +76,3 @@
+                                        { expand: false, x_fill: false, x_align: St.Align.END });
+
+        this._timeoutId = Mainloop.timeout_add(1000, Lang.bind(this, this._tick));

Use a constant?

@@ +1146,3 @@
+
+    _confirmDisplayChange: function() {
+        (new DisplayChangeDialog(this._shellwm)).open();

Use let ... = new .. (); etc looks easier to read then this.
Comment 10 Alexandre Franke 2013-08-20 07:42:12 UTC
(In reply to comment #9)
> Add a translator comment that warns about the length.

Is it really impossible to adjust to the length of the string?
Comment 11 Giovanni Campagna 2013-08-20 08:27:00 UTC
Created attachment 252344 [details] [review]
Add a confirmation dialog for display changes

Unfortunately, display configuration can and does fail, due
to unspecified HW constraints, drivers bugs, unsupported exotic
configurations or just bad luck.
So when the user makes a change in the control center, show
a dialog asking him if it looks OK, and revert back after 20 seconds
otherwise.
Comment 12 drago01 2013-08-20 08:39:47 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > Add a translator comment that warns about the length.
> 
> Is it really impossible to adjust to the length of the string?

No, see comment 4 ... but the designers want a fixed width for those dialogs.
Comment 13 drago01 2013-08-20 08:40:51 UTC
Review of attachment 252344 [details] [review]:

LG.
Comment 14 Giovanni Campagna 2013-08-20 10:03:39 UTC
Attachment 252344 [details] pushed as 02224bb - Add a confirmation dialog for display changes