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 687127 - Run dialog is missing a close button
Run dialog is missing a close button
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.7.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 687125 689999 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-10-29 15:27 UTC by Allan Day
Modified: 2012-12-10 19:17 UTC
See Also:
GNOME target: ---
GNOME version: 3.7/3.8


Attachments
runDialog: Better match style of other modal dialogs (3.99 KB, patch)
2012-10-29 16:40 UTC, Florian Müllner
needs-work Details | Review
runDialog: Better match style of other modal dialogs (5.52 KB, patch)
2012-10-29 17:22 UTC, Florian Müllner
committed Details | Review
theme patch (1.15 KB, patch)
2012-10-30 09:46 UTC, Allan Day
committed Details | Review
runDialog: Add entry to focus chain (1.11 KB, patch)
2012-10-30 14:04 UTC, Florian Müllner
committed Details | Review
runDialog: Remove "Run" button again (2.55 KB, patch)
2012-10-30 14:04 UTC, Florian Müllner
committed Details | Review

Description Allan Day 2012-10-29 15:27:02 UTC
It is possible that someone could accidentally trigger the run dialog by pressing Alt+F2. This could be rather confusing, since the dialog doesn't have a close button - there's no visible way to make it go away.

Let's add a close button. :)

https://dl.dropbox.com/u/5031519/gnome-shell/run-dialog-new.png
Comment 1 Allan Day 2012-10-29 15:41:00 UTC
Note that this also requires that we use a standard text entry field in this dialog. The text entry field should receive keyboard focus by default.
Comment 2 Florian Müllner 2012-10-29 15:48:32 UTC
(In reply to comment #1)
> Note that this also requires that we use a standard text entry field in this
> dialog. The text entry field should receive keyboard focus by default.

That is already the case, it is just styled differently. Is that what you are referring to? You opened two bugs attaching two different mockups ...
Comment 3 Allan Day 2012-10-29 15:51:34 UTC
(In reply to comment #2) 
> That is already the case, it is just styled differently. Is that what you are
> referring to?

OK, then it just needs to use the standard styling so you can see when the text entry is focused.

> You opened two bugs attaching two different mockups ...

I updated the same mockup for both bugs (one advantage of hosting them yourself).
Comment 4 Florian Müllner 2012-10-29 16:40:53 UTC
Created attachment 227556 [details] [review]
runDialog: Better match style of other modal dialogs

Update the run dialog to
 - use a proper title
 - spot a close button
 - use the standard entry style
Comment 5 Florian Müllner 2012-10-29 16:41:09 UTC
*** Bug 687125 has been marked as a duplicate of this bug. ***
Comment 6 Jasper St. Pierre (not reading bugmail) 2012-10-29 16:50:44 UTC
Review of attachment 227556 [details] [review]:

::: data/theme/gnome-shell.css
@@ +338,2 @@
 .login-dialog-prompt-entry,
 .prompt-dialog-password-entry {

Can we have a shared style class for these?

::: js/ui/runDialog.js
@@ +239,3 @@
+        this.setButtons([{ action: Lang.bind(this, this.close),
+                           label: _("Close"),
+                           key: Clutter.Escape }]);

As discussed on IRC, it would be nice to have a "Run" button as well.
Comment 7 Florian Müllner 2012-10-29 17:22:08 UTC
Created attachment 227567 [details] [review]
runDialog: Better match style of other modal dialogs

Add a "Run" button as well, use more concise style classes
Comment 8 Jasper St. Pierre (not reading bugmail) 2012-10-29 18:01:29 UTC
Review of attachment 227567 [details] [review]:

::: js/ui/runDialog.js
@@ +297,3 @@
+            this.close();
+        else {
+            if (!this.pushModal())

I know you copy-pasted it from somewhere else, but bad style. Fix this up before pushing, please.
Comment 9 Florian Müllner 2012-10-29 18:12:59 UTC
Attachment 227567 [details] pushed as 0c807bd - runDialog: Better match style of other modal dialogs
Comment 10 Allan Day 2012-10-30 09:46:43 UTC
Created attachment 227621 [details] [review]
theme patch

I've just tested this, and there are a few tiny issues with the text styles. See the attached patch -  I've use the standard text styles for the entry field and tweaked the color and the padding of the heading.

Also, I don't know why you've added a run button here. I don't think we should be aiming to make the run dialog friendlier. The only reason we're adding the close button is to provide an escape route in case someone accidentally triggers the dialog.

Finally, it would be good to remove the icon from the error warnings we show here. The red symbol seems overkill for what is most likely a typo. I would use the same style that we have in the login screen - which is basically the same as we have now, except that it doesn't use an icon and the text is color: orange; .
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-10-30 13:39:02 UTC
(In reply to comment #10)
> Also, I don't know why you've added a run button here. I don't think we should
> be aiming to make the run dialog friendlier. The only reason we're adding the
> close button is to provide an escape route in case someone accidentally
> triggers the dialog.

As discussed on IRC, it was said that we wanted to provide a way for someone to be sure that they can run a command if they accidentally focused away from the text entry.
Comment 12 Allan Day 2012-10-30 13:49:22 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > Also, I don't know why you've added a run button here. I don't think we should
> > be aiming to make the run dialog friendlier. The only reason we're adding the
> > close button is to provide an escape route in case someone accidentally
> > triggers the dialog.
> 
> As discussed on IRC, it was said that we wanted to provide a way for someone to
> be sure that they can run a command if they accidentally focused away from the
> text entry.

Doesn't sound like a valid concern to me. It's a run command: you press return. This is analogous to a terminal or even chat: we don't have a "run" button in the terminal or a "send" button in chat notifications.
Comment 13 Florian Müllner 2012-10-30 14:04:49 UTC
Created attachment 227639 [details] [review]
runDialog: Add entry to focus chain

Currently the entry takes the intial key focus, but is not actually
part of the focus chain. Fix that, even though keynav does not work
too well for the dialog anyway, due to the entry consuming tab for
command completion.
Comment 14 Florian Müllner 2012-10-30 14:04:55 UTC
Created attachment 227640 [details] [review]
runDialog: Remove "Run" button again

While not in the mockups, it was introduced during review of commit
0c807bddaf4da5 after discussion on IRC, but the designers disagree;
remove it again.
Comment 15 Jasper St. Pierre (not reading bugmail) 2012-10-30 14:07:46 UTC
Review of attachment 227639 [details] [review]:

Uh, sure.
Comment 16 Jasper St. Pierre (not reading bugmail) 2012-10-30 14:08:54 UTC
Review of attachment 227640 [details] [review]:

Meh. It looks really strange to have an explicit close button and no "OK" action. I think I'd be happier if it was an X bubble instead, but whatever.

What we *do* know is that lots of people use the run dialog right now simply because the overview is too slow.
Comment 17 Allan Day 2012-10-30 14:21:38 UTC
(In reply to comment #14)
> Created an attachment (id=227640) [details] [review]
> runDialog: Remove "Run" button again
> 
> While not in the mockups, it was introduced during review of commit
> 0c807bddaf4da5 after discussion on IRC, but the designers disagree;
> remove it again.

Thanks Florian. Is it OK for me to push my theme patch?
Comment 18 Florian Müllner 2012-10-30 14:54:38 UTC
Attachment 227639 [details] pushed as a607174 - runDialog: Add entry to focus chain
Attachment 227640 [details] pushed as 4d51056 - runDialog: Remove "Run" button again

(In reply to comment #17)
> Thanks Florian. Is it OK for me to push my theme patch?

The patch looks fine, so I took the freedom to just push it with the other two patches.
Comment 19 Stephane Raimbault 2012-11-08 14:24:55 UTC
Allan has posted on his blog[1] about this change.

As described in post comments I'm not the only one to find it weird to have Close button to run a command. My mind will refrain me to press Enter key to avoid to trigger this Close button (comment #16 makes sense to me if you really want a close button).

BTW, I find the current dialog (GNOME 3.4) visually 'lighter'.

[1] http://afaikblog.wordpress.com/2012/11/07/fantastic-progress-in-every-detail-matters/(In reply to comment #16)
Comment 20 Cosimo Cecchi 2012-12-10 19:17:21 UTC
*** Bug 689999 has been marked as a duplicate of this bug. ***