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 657986 - Fix keyboard in run dialog and lg (and other drive-by fixes)
Fix keyboard in run dialog and lg (and other drive-by fixes)
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: keyboard
unspecified
Other All
: Normal normal
: ---
Assigned To: Florian Müllner
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2011-09-01 18:13 UTC by Dan Winship
Modified: 2011-09-22 12:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
main: add back layoutManager.init() call that was accidentally removed (699 bytes, patch)
2011-09-01 18:13 UTC, Dan Winship
none Details | Review
layout: fix a few keyboard show/hide bugs (1.99 KB, patch)
2011-09-01 18:13 UTC, Dan Winship
accepted-commit_now Details | Review
keyboard: create and destroy this.actor (2.38 KB, patch)
2011-09-01 18:13 UTC, Dan Winship
reviewed Details | Review
layout: add chrome actors directly to uiGroup (8.78 KB, patch)
2011-09-01 18:13 UTC, Dan Winship
reviewed Details | Review
layout: separate keyboard and tray (5.43 KB, patch)
2011-09-01 18:13 UTC, Dan Winship
accepted-commit_now Details | Review
keyboard: fix in Run dialog and Looking Glass (4.87 KB, patch)
2011-09-01 18:13 UTC, Dan Winship
reviewed Details | Review
main: add back layoutManager.init() call that was accidentally removed (2.69 KB, patch)
2011-09-01 22:13 UTC, Dan Winship
reviewed Details | Review
main: add back layoutManager.init() call that was accidentally removed (2.69 KB, patch)
2011-09-02 13:23 UTC, Dan Winship
reviewed Details | Review
main: add back layoutManager.init() call that was accidentally removed (699 bytes, patch)
2011-09-06 15:41 UTC, Dan Winship
committed Details | Review
layout: fix initial struts (2.56 KB, patch)
2011-09-06 15:41 UTC, Dan Winship
committed Details | Review
layout: fix a few keyboard show/hide bugs (1.99 KB, patch)
2011-09-06 15:42 UTC, Dan Winship
committed Details | Review
keyboard: create and destroy this.actor (2.43 KB, patch)
2011-09-06 15:42 UTC, Dan Winship
committed Details | Review
shell-generic-container: document signals (3.49 KB, patch)
2011-09-06 15:42 UTC, Dan Winship
committed Details | Review
layout: add chrome actors directly to uiGroup (8.34 KB, patch)
2011-09-06 15:43 UTC, Dan Winship
committed Details | Review
layout: separate keyboard and tray (5.43 KB, patch)
2011-09-06 15:44 UTC, Dan Winship
committed Details | Review
layout: fix keyboard in modal dialogs (914 bytes, patch)
2011-09-06 15:44 UTC, Dan Winship
committed Details | Review
lookingGlass: fix to not overlap the keyboard (4.70 KB, patch)
2011-09-06 15:45 UTC, Dan Winship
committed Details | Review

Description Dan Winship 2011-09-01 18:13:42 UTC
Currently the OSK doesn't work in Alt+F2, because it pops up
underneath the lightbox, and it's annoying in lg, because it pops up
over the bottom of the lg dialog. Fix both of those.

The first three patches fix miscellaneous issues found along the way
Comment 1 Dan Winship 2011-09-01 18:13:44 UTC
Created attachment 195419 [details] [review]
main: add back layoutManager.init() call that was accidentally removed

Fixes the chrome to update when entering/leaving the overview again
Comment 2 Dan Winship 2011-09-01 18:13:46 UTC
Created attachment 195420 [details] [review]
layout: fix a few keyboard show/hide bugs
Comment 3 Dan Winship 2011-09-01 18:13:49 UTC
Created attachment 195421 [details] [review]
keyboard: create and destroy this.actor

Rather than having Main.keyboard.actor always exist, and creating and
destroying only its contents, create and destroy that actor as well.
Comment 4 Dan Winship 2011-09-01 18:13:52 UTC
Created attachment 195422 [details] [review]
layout: add chrome actors directly to uiGroup

Rather than having a single chrome layer and putting all of the chrome
into that, put the chrome actors directly into uiGroup, so that they
can be stacked independently of one another relative to other actors.

(This requires making uiGroup a ShellGenericContainer, so we can use
skip_paint to avoid painting non-visibleInFullscreen chrome when we're
in fullscreen.)
Comment 5 Dan Winship 2011-09-01 18:13:54 UTC
Created attachment 195423 [details] [review]
layout: separate keyboard and tray

The keyboard and tray need to animate together, but they sometimes
need to be in different stacking layers (eg, from the screensaver you
want access to the keyboard, but not the tray). So remove _bottomBox
and just keep trayBox and keyboardBox lined up manually.
Comment 6 Dan Winship 2011-09-01 18:13:57 UTC
Created attachment 195424 [details] [review]
keyboard: fix in Run dialog and Looking Glass

Fix the keyboard in the Run dialog by raising it above the ModalDialog
lightbox so you can use it to type.

Fix it in LookingGlass by having LG shrink itself vertically when the
keyboard is present, if that is necessary to fit.
Comment 7 Dan Winship 2011-09-01 22:13:25 UTC
Created attachment 195445 [details] [review]
main: add back layoutManager.init() call that was accidentally removed

===

fix the initial setting of the panel strut, which got broken at some
point (and was being set to 0 height when the panel first started
animating in)
Comment 8 Dan Winship 2011-09-02 13:23:15 UTC
Created attachment 195478 [details] [review]
main: add back layoutManager.init() call that was accidentally removed

===

another fix
Comment 9 Florian Müllner 2011-09-02 19:16:10 UTC
Review of attachment 195445 [details] [review]:

Code looks good, but there's not really a connection between the changes in layout.js and main.js (or the commit message subject and body for what it's worth). Please split before pushing (no further review required on my part).

::: js/ui/layout.js
@@ +757,2 @@
     _queueUpdateRegions: function() {
+        if (!this._updateRegionIdle && !this._freezeUpdateCount)

Mmmh, I thought we had some unwritten style guide discouraging the use of integers as booleans, but it's consistent with the existing code, so OK with me ...
Comment 10 Florian Müllner 2011-09-02 19:19:22 UTC
Review of attachment 195445 [details] [review]:

Code looks good, but there's not really a connection between the changes in layout.js and main.js (or the commit message subject and body for what it's worth). Please split before pushing (no further review required on my part).

::: js/ui/layout.js
@@ +757,2 @@
     _queueUpdateRegions: function() {
+        if (!this._updateRegionIdle && !this._freezeUpdateCount)

Mmmh, I thought we had some unwritten style guide discouraging the use of integers as booleans, but it's consistent with the existing code, so OK with me ...
Comment 11 Florian Müllner 2011-09-02 19:19:51 UTC
Review of attachment 195420 [details] [review]:

LGTM
Comment 12 Florian Müllner 2011-09-02 19:20:04 UTC
Review of attachment 195421 [details] [review]:

Minor style comment, otherwise fine.

::: js/ui/keyboard.js
@@ +202,3 @@
         DBus.session.acquire_name('org.gnome.Caribou.Keyboard', 0, null, null);
 
         this._timestamp = global.get_current_time();

I like having an overview of all properties in the constructor, so

  this.actor = null;

would be nice here in my opinion ...
Comment 13 Florian Müllner 2011-09-02 19:20:12 UTC
Review of attachment 195422 [details] [review]:

Code looks generally fine, but some questions:

::: js/ui/layout.js
@@ +674,3 @@
+            let actorData = this._trackedActors[i], visible;
+            if (!actorData.isToplevel)
+                continue;

Not sure I understand this - if a tracked actor is reparented we consider it removed from the chrome, but rather than untracking it we use isToplevel to ignore it here (but not in updateRegions())?

::: js/ui/lookingGlass.js
@@ +762,3 @@
         this._updateFont();
 
+        Main.uiGroup.add_actor(this.actor);

Not sure I like this - it relies on the panel being chrome and chrome being added (as toplevel child) to uiGroup; the previous code only made the first assumption and looks like it would still work just fine - is tracking an additional actor that much of a performance penalty? (Or did I miss another reason for this change?)

::: js/ui/main.js
@@ +195,3 @@
     // Set up stage hierarchy to group all UI actors under one container.
+    uiGroup = new Shell.GenericContainer({ name: 'uiGroup' });
+    uiGroup.connect('allocate',

I know this was just moved from layout.js, but is it really safe to ignore the get-preferred-{width,height} signals? Apparently alloc is allocated using g_slice_new0(), but it doesn't appear like a particular good idea to rely on that implementation detail.
Comment 14 Florian Müllner 2011-09-02 19:20:32 UTC
Review of attachment 195424 [details] [review]:

Works as advertised, I'm just not very keen on the layoutManager addition (luckily, the code should work mostly unmodified in lookingGlass.js).

Also a minor nit in the commit message: s/Run dialog/modal dialogs/ - for instance the polkit authentication dialogs are fixed by this patch as well.

::: js/ui/layout.js
@@ +169,3 @@
+        this.primaryContentArea = new Meta.Rectangle(this.primaryMonitor);
+        if (this.primaryIndex == this.bottomIndex)
+            this.primaryContentArea.height -= this.keyboardBox.height;

Right now, "content area" feels very arbitrary ("full monitor area including panel and messagetray, but excluding the keyboard") - it just happens to be what we need for lookingGlass, but then it makes more sense to me to do the calculation there (all properties involved are already public, yay).

::: js/ui/lookingGlass.js
@@ +927,2 @@
         let myWidth = primary.width * 0.7;
+        let myHeight = Math.min(primary.height * 0.7, contentArea.height * 0.9);

I suppose contentArea doesn't consider the keyboard's position on purpose? On the one hand, I suspect this avoids quite some annoying shifting-stuff-around when showing/hiding the keyboard, on the other hand, the keyboard takes up quite a bit of space on small screens, which could be used while the keyboard's hidden.
Comment 15 Florian Müllner 2011-09-02 19:20:42 UTC
Review of attachment 195423 [details] [review]:

LGTM
Comment 16 Florian Müllner 2011-09-02 19:29:05 UTC
Review of attachment 195478 [details] [review]:

Marking 'reviewed', as the review of the previous version still applies.

::: js/ui/layout.js
@@ +769,3 @@
+    thawUpdateRegions: function() {
+        this._freezeUpdateCount--;
+        this._queueUpdateRegions();

Woops, missed that!
Comment 17 Dan Winship 2011-09-02 19:56:14 UTC
(In reply to comment #13)
> +            if (!actorData.isToplevel)
> +                continue;
> 
> Not sure I understand this - if a tracked actor is reparented we consider it
> removed from the chrome, but rather than untracking it we use isToplevel to
> ignore it here (but not in updateRegions())?

no, isToplevel is set by both addChrome() and _actorReparented(); it just tracks whether the actor is a "toplevel" piece of chrome (eg, the message tray), or a child of some other piece of chrome (eg, the notification pop-up, which is a child of the message tray, but extends outside the message tray's allocation, and so needs to be examined separately when computing the input region).

updateVisibility() only acts on toplevel chrome because we're hiding and showing things by calling Main.uiGroup.set_skip_paint() on them, so obviously we can only call it on actors that are children of Main.uiGroup. (So this code was wrong before, by not checking that, but apparently set_skip_paint() just fails silently if you pass it a bogus actor, so we didn't notice.)

> Not sure I like this - it relies on the panel being chrome and chrome being
> added (as toplevel child) to uiGroup; the previous code only made the first
> assumption and looks like it would still work just fine - is tracking an
> additional actor that much of a performance penalty? (Or did I miss another
> reason for this change?)

Yeah, chrome is there to make things work in Shell.StageInputMode.NORMAL. My feeling is that things that only exist outside that have no need to be chrome, and so they shouldn't be. (This is particularly true if the chrome-as-override-redirect-windows stuff from bug 633620 happens.)

Although... actually we should just make lg be a child of Main.layoutManager.panelBox, since that's really what we want, visually. (He said, totally contradicting the previous paragraph.)

> I know this was just moved from layout.js, but is it really safe to ignore the
> get-preferred-{width,height} signals?

I'll add documentation saying it is. :-)

(In reply to comment #14)
> Right now, "content area" feels very arbitrary

yes...

> it just happens to be
> what we need for lookingGlass, but then it makes more sense to me to do the
> calculation there (all properties involved are already public, yay).

well, the theory behind LayoutManager is that it handles all of the large-scale layout issues, so that all the different js modules aren't constantly poking their fingers into each other quite so much... But yeah, maybe it would be better to just call this "lookingGlassBox"

> I suppose contentArea doesn't consider the keyboard's position on purpose?

Hm... well, on my screen there was still plenty of lg space even with the keyboard, so I guess I didn't think about that.
Comment 18 Florian Müllner 2011-09-02 23:15:35 UTC
(In reply to comment #17)
> no, isToplevel is set by both addChrome() and _actorReparented(); [...]

OK. Thanks for the lengthy explanation!


> (He said, totally contradicting the previous paragraph.)

:-)


> > I know this was just moved from layout.js, but is it really safe to ignore the
> > get-preferred-{width,height} signals?
> 
> I'll add documentation saying it is. :-)

Works for me.


> (In reply to comment #14)
> well, the theory behind LayoutManager is that it handles all of the large-scale
> layout issues, so that all the different js modules aren't constantly poking
> their fingers into each other quite so much... But yeah, maybe it would be
> better to just call this "lookingGlassBox"

Not sure I'd consider this particular case a "large-scale layout issue", but surely "lookingGlassBox" sounds more appropriate ...


> > I suppose contentArea doesn't consider the keyboard's position on purpose?
> 
> Hm... well, on my screen there was still plenty of lg space even with the
> keyboard, so I guess I didn't think about that.


OK. To be honest, I'm not sure it's really worth spending much thought on it - I doubt devices with small screens which require osk (aka tablets) are commonly used for development (not to mention that afaict there's currently no way to trigger lg without using a physical keyboard ...)
Comment 19 Dan Winship 2011-09-06 15:41:37 UTC
Created attachment 195794 [details] [review]
main: add back layoutManager.init() call that was accidentally removed

---

just that, not the other stuff that had crept into the patch before
Comment 20 Dan Winship 2011-09-06 15:41:56 UTC
Created attachment 195795 [details] [review]
layout: fix initial struts

---

the other stuff from the earlier init() fix patch
Comment 21 Dan Winship 2011-09-06 15:42:02 UTC
Created attachment 195796 [details] [review]
layout: fix a few keyboard show/hide bugs
Comment 22 Dan Winship 2011-09-06 15:42:34 UTC
Created attachment 195797 [details] [review]
keyboard: create and destroy this.actor

Rather than having Main.keyboard.actor always exist, and creating and
destroying only its contents, create and destroy that actor as well.
Comment 23 Dan Winship 2011-09-06 15:42:41 UTC
Created attachment 195798 [details] [review]
shell-generic-container: document signals

In particular, document that you can ignore the get-preferred-*
signals if and only if you have a fixed width/height specified by some
other means.
Comment 24 Dan Winship 2011-09-06 15:43:20 UTC
Created attachment 195799 [details] [review]
layout: add chrome actors directly to uiGroup

---

now with fewer changes the lookingGlass
Comment 25 Dan Winship 2011-09-06 15:44:25 UTC
Created attachment 195800 [details] [review]
layout: separate keyboard and tray

The keyboard and tray need to animate together, but they sometimes
need to be in different stacking layers (eg, from the screensaver you
want access to the keyboard, but not the tray). So remove _bottomBox
and just keep trayBox and keyboardBox lined up manually.
Comment 26 Dan Winship 2011-09-06 15:44:52 UTC
Created attachment 195801 [details] [review]
layout: fix keyboard in modal dialogs

---

just the modal dialog part of the earlier patch, and now says "modal"
instead of "run"
Comment 27 Dan Winship 2011-09-06 15:45:11 UTC
Created attachment 195802 [details] [review]
lookingGlass: fix to not overlap the keyboard

---

just the lg parts, without modifications to layoutManager
Comment 28 Florian Müllner 2011-09-06 18:11:13 UTC
Review of attachment 195794 [details] [review]:

Yup.
Comment 29 Florian Müllner 2011-09-06 18:11:13 UTC
Review of attachment 195795 [details] [review]:

Sure.
Comment 30 Florian Müllner 2011-09-06 18:11:16 UTC
Review of attachment 195796 [details] [review]:

Looks good.
Comment 31 Florian Müllner 2011-09-06 18:11:19 UTC
Review of attachment 195797 [details] [review]:

LGTM
Comment 32 Florian Müllner 2011-09-06 18:11:26 UTC
Review of attachment 195798 [details] [review]:

Looks good (even though there's some shouting in the ::allocate docs ;-)
Comment 33 Florian Müllner 2011-09-06 18:11:30 UTC
Review of attachment 195799 [details] [review]:

LGTM
Comment 34 Florian Müllner 2011-09-06 18:11:56 UTC
Review of attachment 195800 [details] [review]:

LGTM
Comment 35 Florian Müllner 2011-09-06 18:12:06 UTC
Review of attachment 195801 [details] [review]:

Yup
Comment 36 Florian Müllner 2011-09-06 18:12:17 UTC
Review of attachment 195802 [details] [review]:

Yeah, I like that a lot better than the previous version.
Comment 37 Dan Winship 2011-09-06 18:33:16 UTC
Attachment 195794 [details] pushed as a199c12 - main: add back layoutManager.init() call that was accidentally removed
Attachment 195795 [details] pushed as bf0bcaa - layout: fix initial struts
Attachment 195796 [details] pushed as d99f08b - layout: fix a few keyboard show/hide bugs
Attachment 195797 [details] pushed as 9752fda - keyboard: create and destroy this.actor
Attachment 195798 [details] pushed as cbb3831 - shell-generic-container: document signals
Attachment 195799 [details] pushed as e79c093 - layout: add chrome actors directly to uiGroup
Attachment 195800 [details] pushed as 627fff9 - layout: separate keyboard and tray
Attachment 195801 [details] pushed as ef57c9f - layout: fix keyboard in modal dialogs
Attachment 195802 [details] pushed as 016e384 - lookingGlass: fix to not overlap the keyboard