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 643606 - Maximizing windows not working if window is too wide
Maximizing windows not working if window is too wide
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
2.91.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 672378 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2011-03-01 17:17 UTC by Hendrik Richter
Modified: 2012-03-19 07:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screenshot (155.86 KB, image/png)
2011-05-25 16:08 UTC, William Jon McCann
  Details
window: Disallow maximization for windows that can't be maximized (3.51 KB, patch)
2012-03-08 20:43 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
window: Disallow maximization for windows that can't be maximized (4.10 KB, patch)
2012-03-12 21:09 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
Simplify the frame testing logic in callers to grab borders (8.62 KB, patch)
2012-03-13 03:18 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
window: Fix meta_window_get_workspaces when a window isn't on a workspace (1.04 KB, patch)
2012-03-13 04:41 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
window: Disallow maximization for windows that can't be maximized (2.21 KB, patch)
2012-03-13 04:41 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
window: Disallow maximization for windows that can't be maximized (2.32 KB, patch)
2012-03-17 01:35 UTC, Jasper St. Pierre (not reading bugmail)
rejected Details | Review

Description Hendrik Richter 2011-03-01 17:17:20 UTC
(I'm using the latest gnome-shell from Fedora 15 repository)

When starting Evoluton with LC_ALL=de_DE the window is too wide to fit on 1024x768. This didn't happen under GNOME 2, now with gnome-shell the fonts and everything else are larger than before, and there seems no obvious way to change is.

But the main bug is: if I move the Evolution window to the upper border in order to maximize it, the shell displays the blueish overlay indicating that the window is maximized if I drop it, but then it isn't maximized (though it receives the maximized property, i.e. I cannot resize it afterwards). This works fine for windows that aren't too wide (e.g. LC_ALL=C evolution), but not if the window is too wide. I have to manually un-maximize it and then manually change the size to maximum.
Comment 1 William Jon McCann 2011-05-25 16:08:01 UTC
Created attachment 188603 [details]
screenshot

Can confirm this.
Comment 2 William Jon McCann 2012-03-08 19:25:03 UTC
FYI. To reproduce this:

 * Change resolution to say 1024x768 using Display Settings
 * Run LC_ALL=de_DE evolution
 * Right click on titlebar and select Maximize
Comment 3 William Jon McCann 2012-03-08 19:40:32 UTC
Also confirmed with LC_ALL=de_DE palimpsest
Comment 4 Jasper St. Pierre (not reading bugmail) 2012-03-08 20:43:18 UTC
Created attachment 209284 [details] [review]
window: Disallow maximization for windows that can't be maximized
Comment 5 Jasper St. Pierre (not reading bugmail) 2012-03-08 20:43:57 UTC
Talked to Jon a bit. if the window can't fit in the screen size, there's no point in pretending it can be maximized. This is really the only solution we can do.
Comment 6 drago01 2012-03-12 08:39:24 UTC
Review of attachment 209284 [details] [review]:

Approach makes sense to me. Commit message lacks a body.

::: src/core/window.c
@@ +3673,3 @@
 
+static void
+get_possible_tile_area (MetaWindow    *window,

Don't like that name ... maybe get maximum or available tile area?

@@ +3679,3 @@
+
+  monitor = meta_screen_get_current_monitor (window->screen);
+  meta_window_get_work_area_for_monitor (window, monitor->number, tile_area);

I'd just do window->monitor->number ... you don't need "monitor" for anything else anyway.

@@ +3713,3 @@
+  /* Don't call can_maximize here -- we end up doing mostly
+   * the same calculations, so just do the only unique check
+   * here. */

"Only unique check" sounds weird ... common instead of unique?
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-03-12 08:49:57 UTC
Review of attachment 209284 [details] [review]:

::: src/core/window.c
@@ +3673,3 @@
 
+static void
+get_possible_tile_area (MetaWindow    *window,

Everything about the name is terrible - it's not just related to the tile area, either. It's just the monitor rect minus visible borders...

... Wait, why are we removing the visible borders? I think this code is incorrect and we should be removing the invisible borders...

@@ +3679,3 @@
+
+  monitor = meta_screen_get_current_monitor (window->screen);
+  meta_window_get_work_area_for_monitor (window, monitor->number, tile_area);

I copied the code from meta_window_can_tile_side_by_side. I'm not sure of the case where the window's monitor and "the current" monitor are different in practice, but I want to make sure I'm not breaking anything.

@@ +3713,3 @@
+  /* Don't call can_maximize here -- we end up doing mostly
+   * the same calculations, so just do the only unique check
+   * here. */

It's not common, though. It's the one check that's in can_maximize, but not can_tile_side_by_side.

"the only check unique to can_maximize" maybe?
Comment 8 drago01 2012-03-12 18:23:23 UTC
(In reply to comment #7)
> Review of attachment 209284 [details] [review]:

(Argh. yet another case of "doing a review does not add me to CC" -.- )

> ::: src/core/window.c
> @@ +3673,3 @@
> 
> +static void
> +get_possible_tile_area (MetaWindow    *window,
> 
> Everything about the name is terrible - it's not just related to the tile area,
> either. It's just the monitor rect minus visible borders...

*You* named it ... ;)

> ... Wait, why are we removing the visible borders? I think this code is
> incorrect and we should be removing the invisible borders...

Err yeah.

> @@ +3679,3 @@
> +
> +  monitor = meta_screen_get_current_monitor (window->screen);
> +  meta_window_get_work_area_for_monitor (window, monitor->number, tile_area);
> 
> I copied the code from meta_window_can_tile_side_by_side. I'm not sure of the
> case where the window's monitor and "the current" monitor are different in
> practice, but I want to make sure I'm not breaking anything.

We do use window->monitor->number in a lot of places so I doubt this will break anything.

> @@ +3713,3 @@
> +  /* Don't call can_maximize here -- we end up doing mostly
> +   * the same calculations, so just do the only unique check
> +   * here. */
> 
> It's not common, though. It's the one check that's in can_maximize, but not
> can_tile_side_by_side.
> 
> "the only check unique to can_maximize" maybe?

I meant "common" as in "both have it".
Comment 9 Florian Müllner 2012-03-12 20:31:36 UTC
(In reply to comment #8)
> > I copied the code from meta_window_can_tile_side_by_side. I'm not sure of the
> > case where the window's monitor and "the current" monitor are different in
> > practice, but I want to make sure I'm not breaking anything.
> 
> We do use window->monitor->number in a lot of places so I doubt this will break
> anything.

The two may indeed be different: window->monitor->number is the monitor which contains the largest part of window; meta_screen_get_current_monitor() is the monitor which contains the pointer. In most cases the two are identical (because the function is only called while dragging the window around iirc), but they may be different near the screen edges (which happen to be the areas we are most interested in for tiling).

(For deciding whether we show the tile previews, we check whether the pointer is near a monitor edge; it would be pretty weird if entering the "hot area" triggered a tile preview on another monitor, so we really want to base the "tile monitor" on the pointer position)
Comment 10 Jasper St. Pierre (not reading bugmail) 2012-03-12 21:09:39 UTC
Created attachment 209545 [details] [review]
window: Disallow maximization for windows that can't be maximized

Windows that have minimum widths larger than the screen can't be maximized,
even though we put them in a maximized state and allow users to do so:
the window just won't change size and position. Fix this by simply not giving
the option to maximize, like what happens for non-resizable windows.



I still can't find a good name for that function. As Florian points out, I
think meta_screen_get_current_monitor is the appropriate thing to do here
(though a lot of this will break when we start handling touch and multiple
pointers, but that's not my bug :)
Comment 11 Ray Strode [halfline] 2012-03-13 00:03:51 UTC
i think you need to make sure _NET_WM_ALLOWED_ACTIONS is up to date, too, otherwise libwnck will be out of sync from mutter (not that that would practically matter in most cases).  Still "git grep has_maximize_func" shows a lot of places where has_maximize_func is checked to mean "can maximize", and it shows that variable getting toggled off in some cases where maximization isn't allowed. So I think we already have a can_maximize() kind of thing in the code, it's "window->has_maximize_func".  Given that, maybe it makes sense to toggle it directly?

It looks like recalc_window_features would be an appropriate place.
Comment 12 Jasper St. Pierre (not reading bugmail) 2012-03-13 03:18:44 UTC
Created attachment 209557 [details] [review]
Simplify the frame testing logic in callers to grab borders

A lot of code did something similar to:

  MetaFrameBorders borders;

  if (window->frame)
    meta_frame_calc_borders (window->frame, &borders);
  else
    meta_frame_borders_clear (&borders);

Sometimes, the else part was omitted and we were unknowingly using
uninitalized values for OR windows. Clean this up by just testing
for a NULL frame in meta_frame_calc_borders and clearing for the
caller if so.
Comment 13 Jasper St. Pierre (not reading bugmail) 2012-03-13 04:41:52 UTC
Created attachment 209559 [details] [review]
window: Fix meta_window_get_workspaces when a window isn't on a workspace

Since we're going to be evaluating the work area at startup now, we need
to make sure that we don't iterate over workspaces before they're assigned.
The easiest way to do this is to make sure that meta_window_get_workspaces
doesn't crash.
Comment 14 Jasper St. Pierre (not reading bugmail) 2012-03-13 04:41:56 UTC
Created attachment 209560 [details] [review]
window: Disallow maximization for windows that can't be maximized

Windows that have minimum widths larger than the screen can't be maximized,
even though we put them in a maximized state and allow users to do so:
the window just won't change size and position. Fix this by simply not giving
the option to maximize, like what happens for non-resizable windows.
Comment 15 drago01 2012-03-16 23:30:46 UTC
Review of attachment 209560 [details] [review]:

Makes sense, fine to commit with the comment fixed.

::: src/core/window.c
@@ +4506,3 @@
+
+      /* If we're changing monitors, we need to update the has_maximize_func flag,
+       * as the working area has changed. */

"work area"
Comment 16 drago01 2012-03-16 23:32:35 UTC
Review of attachment 209559 [details] [review]:

Looks good.
Comment 17 Jasper St. Pierre (not reading bugmail) 2012-03-17 01:00:42 UTC
This still depends on the first "frame borders" commit -- while not necessary, it's a nice cleanup IMO.
Comment 18 Jasper St. Pierre (not reading bugmail) 2012-03-17 01:32:37 UTC
Comment on attachment 209559 [details] [review]
window: Fix meta_window_get_workspaces when a window isn't on a workspace

Attachment 209559 [details] pushed as 578b1c0 - window: Fix meta_window_get_workspaces when a window isn't on a workspace
Comment 19 Jasper St. Pierre (not reading bugmail) 2012-03-17 01:35:16 UTC
Created attachment 209973 [details] [review]
window: Disallow maximization for windows that can't be maximized

Windows that have minimum widths larger than the screen can't be maximized,
even though we put them in a maximized state and allow users to do so:
the window just won't change size and position. Fix this by simply not giving
the option to maximize, like what happens for non-resizable windows.



And if you don't want to review the frame testing logic patch because we're close to hard code freeze, here's an alternate version that doesn't depend on it.
Comment 20 drago01 2012-03-17 10:35:50 UTC
Review of attachment 209557 [details] [review]:

Looks good.
Comment 21 drago01 2012-03-17 10:36:12 UTC
Review of attachment 209973 [details] [review]:

We don't need this one.
Comment 22 Jasper St. Pierre (not reading bugmail) 2012-03-17 10:41:11 UTC
Attachment 209557 [details] pushed as 8c1b2d5 - Simplify the frame testing logic in callers to grab borders
Attachment 209560 [details] pushed as 7f64d6b - window: Disallow maximization for windows that can't be maximized
Comment 23 Ray Strode [halfline] 2012-03-19 07:19:22 UTC
*** Bug 672378 has been marked as a duplicate of this bug. ***