GNOME Bugzilla – Bug 657986
Fix keyboard in run dialog and lg (and other drive-by fixes)
Last modified: 2011-09-22 12:02:00 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
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
Created attachment 195420 [details] [review] layout: fix a few keyboard show/hide bugs
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.
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.)
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.
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.
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)
Created attachment 195478 [details] [review] main: add back layoutManager.init() call that was accidentally removed === another fix
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 ...
Review of attachment 195420 [details] [review]: LGTM
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 ...
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.
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.
Review of attachment 195423 [details] [review]: LGTM
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!
(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.
(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 ...)
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
Created attachment 195795 [details] [review] layout: fix initial struts --- the other stuff from the earlier init() fix patch
Created attachment 195796 [details] [review] layout: fix a few keyboard show/hide bugs
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.
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.
Created attachment 195799 [details] [review] layout: add chrome actors directly to uiGroup --- now with fewer changes the lookingGlass
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.
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"
Created attachment 195802 [details] [review] lookingGlass: fix to not overlap the keyboard --- just the lg parts, without modifications to layoutManager
Review of attachment 195794 [details] [review]: Yup.
Review of attachment 195795 [details] [review]: Sure.
Review of attachment 195796 [details] [review]: Looks good.
Review of attachment 195797 [details] [review]: LGTM
Review of attachment 195798 [details] [review]: Looks good (even though there's some shouting in the ::allocate docs ;-)
Review of attachment 195799 [details] [review]: LGTM
Review of attachment 195800 [details] [review]: LGTM
Review of attachment 195801 [details] [review]: Yup
Review of attachment 195802 [details] [review]: Yeah, I like that a lot better than the previous version.
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