Created attachment 131463 [details] [review]
Fix the stage_input_area handling

The panel show/hide changes broke things if window restacking occurred
while the overlay was active. Now instead of having the panel set the
input area itself, main.js just watches whether or not the panel is
visible, and updates things itself (taking the modal state into
account as well).

+    inModal = true;
     global.set_stage_input_area(0, 0, global.screen_width, global.screen_height);

Can we remove this call here and just call _panelVisibilityChanged?

+ let inModal = false;

I'd use "var" here...probably means the same thing technically, but "var" to me says "global".

+        if (panel.actor.visible) {

Weird use of braces for the if clause but not the else, even though both of them are oneliners.

(In reply to comment #2)
> +    inModal = true;
>      global.set_stage_input_area(0, 0, global.screen_width,
> global.screen_height);
> 
> Can we remove this call here and just call _panelVisibilityChanged?

No, _panelVisibilityChanged sets the input area to screen_width by PANEL_HEIGHT, this one is setting it to screen_width by screen_height (for overlay/run dialog mode)

> + let inModal = false;
> 
> I'd use "var" here...probably means the same thing technically, but "var" to me
> says "global".

I used "let" because all the other globals in that file do. But all of the other globals are grouped together at the top of the file, so I should move this one to be with those. (I agree though. I wouldn't object to you fixing the whole group.)

> +        if (panel.actor.visible) {
> 
> Weird use of braces for the if clause but not the else, even though both of
> them are oneliners.

The then clause is one *statement*, but it's two *lines*. Syntactically it doesn't require the braces, but IMHO it looks clearer that way in terms of keeping track of what is and isn't inside the "then" part...

It looks like we don't currently have a consistent brace style in our code, and the GJS style guide doesn't say anything either. (Unless "the Java style" implies something.)

The patch looks good to me, presuming you'd move the declaration of inModal to the top. I think if one part of the if-else clause has braces, the other one should have them too.

I've gone ahead and applied Dan's patch since he's out for a couple of days and this is a pretty bad bug affecting the usability of GNOME Shell.

Two changes:

 - I moved inModal to the top block of global variables
 - I renamed this._group to this.actor in panel.js (the patch didn't work
   without this ... it may be dependent on some other outstanding change
   in Dan's working tree.)

(In reply to comment #5)
>  - I renamed this._group to this.actor in panel.js (the patch didn't work
>    without this ... it may be dependent on some other outstanding change
>    in Dan's working tree.)

doh. I'd done a "git commit" without "git push".