! Please note that this is a snapshot of our old Bugzilla server, which is read only since May 29, 2020. Please go to gitlab.xfce.org for our new server !
[PATCH] Too much space reserved for panel when external VGA output unplugged
Status:
RESOLVED: FIXED

Comments

Description John Lindgren editbugs 2014-08-01 05:14:33 CEST
Created attachment 5574 
Use correct screen size for _NET_WM_STRUT_PARTIAL hint

I normally connect an external VGA monitor to my laptop in the following setup:

Screen 0: minimum 8 x 8, current 2646 x 1024, maximum 32767 x 32767
LVDS1 connected 1366x768+0+0
VGA1 connected 1280x1024+1366+0

Unplugging the VGA monitor gives the following setup:

Screen 0: minimum 8 x 8, current 2646 x 1024, maximum 32767 x 32767
LVDS1 connected 1366x768+0+0
VGA1 disconnected 1280x1024+1366+0

Note that only the laptop display is connected, but the X11 screen is still larger enough to encompass both monitors.  With the patch from #11058, the xfce4-panel is correctly displayed at the bottom of the laptop display, but xfwm4 reserves far too much space for the panel, so that maximized windows cover only the top two-thirds of the screen.

xprop on the panel shows the following:

_NET_WM_STRUT_PARTIAL(CARDINAL) = 0, 0, 0, 279, 0, 0, 0, 0, 0, 0, 0, 1365

So the panel is requesting a 279-pixel strut at the bottom of the X11 screen.  This is correct: 1024 - 279 = 745 = 768 - 23, so the 23-pixel panel is positioned at the bottom of the 768-pixel laptop display.  However, xfwm4 incorrectly interprets the strut as relative to the monitor coordinates (actually, the bounding rectangle of all monitors), not the X11 screen size.

My patch fixes the problem by calling gdk_screen_get_width/height() directly rather than using screen_info->width/height.
Comment 1 Olivier Fourdan editbugs 2015-02-23 09:38:31 CET
screen_info->{width,height} is the same as gdk_screen_{widht,height}, both take their values from Xorg.

The patch is not valid because it does not address the root cause of the issue.

Could you give the exact steps to reproduce the issue w/out the patch for bug 11058 and bug 11059 ?

I cannot reproduce that problem.
Comment 2 Olivier Fourdan editbugs 2015-02-23 10:15:52 CET
The root cause of this issue is that xfwm4 computes the size of the entire screen by itself and does not rely on the reported screen width/height.

That was added in src/screen.c myScreenComputeSize() on purpose to address bug 5795.

But it seems in your case you have the monitor reported as disconnected, gtk/gdk doesn't have an API for this (GNOME had one iirc), so we'd have to peek into XRandr directly, which defeats the benefit of relying on gdk for anything xrandr related.
Comment 3 Olivier Fourdan editbugs 2015-02-23 14:12:28 CET
I'll push a fix later tonight for this.
Comment 4 John Lindgren editbugs 2015-02-23 14:22:39 CET
(In reply to Olivier Fourdan from comment #2)
> The root cause of this issue is that xfwm4 computes the size of the entire
> screen by itself and does not rely on the reported screen width/height.
> 
> That was added in src/screen.c myScreenComputeSize() on purpose to address
> bug 5795.

I would not propose any change to myScreenComputeSize().  For window positioning, the bounding rectangle (not the logical screen size) needs to be used, otherwise you would end up with windows on the disconnected monitor, exactly as bug #5795 says.  However, the documentation for _NET_WM_STRUT_PARTIAL [1] specifically says "end values are the height or width of the logical screen", so interpreting _NET_WM_STRUT_PARTIAL relative to the bounding rectangle is wrong.

> But it seems in your case you have the monitor reported as disconnected,
> gtk/gdk doesn't have an API for this (GNOME had one iirc), so we'd have to
> peek into XRandr directly, which defeats the benefit of relying on gdk for
> anything xrandr related.

Why would you need to use XRandr directly?  gdk_screen_get_width/height() gives the logical screen size.

1. http://standards.freedesktop.org/wm-spec/1.3/ar01s05.html
Comment 5 John Lindgren editbugs 2015-02-23 14:24:13 CET
(In reply to Olivier Fourdan from comment #1)
> Could you give the exact steps to reproduce the issue w/out the patch for
> bug 11058 and bug 11059 ?
> 
> I cannot reproduce that problem.

It would not be reproducible without the patch from #11058.
Comment 6 Olivier Fourdan editbugs 2015-02-23 14:32:46 CET
Right, but then I don't think the patch for bug 11058 is correct either, as struts are relative to screen size, not monitor so I reckon the current code is correct.

http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#idm140130317566832

"Note that the strut is relative to the screen edge, and not the edge of the xinerama monitor. "
Comment 7 John Lindgren editbugs 2015-02-23 14:37:22 CET
The patch on #11058 doesn't change how the "strut" is calculated.  That is still done based on the logical screen size (line 1565 in panel-window.c):

  else if (window->struts_edge == STRUTS_EDGE_BOTTOM)
    {
      /* the window is snapped on the bottom screen edge */
      struts[STRUT_BOTTOM] = gdk_screen_get_height (window->screen) - alloc->y;
      struts[STRUT_BOTTOM_START_X] = alloc->x;
      struts[STRUT_BOTTOM_END_X] = alloc->x + alloc->width - 1;
    }

The patch on #11058 only changes where the panel is actually located, which should be at the bottom of the bounding rectangle, not the bottom of the logical screen.
Comment 8 Olivier Fourdan editbugs 2015-02-24 22:41:11 CET
Should be fixed with git commit 68365da
Comment 9 John Lindgren editbugs 2015-02-25 00:57:20 CET
68365da didn't help.  The logical screen size is larger than what myScreenComputeSize() computes, not smaller, so taking the minimum doesn't make any difference.

Was there a problem with the original patch?  I think it's best to use the logical screen size to calculate the struts, without regard for the actual monitor size(s), and to leave myScreenComputeSize() as it was.
Comment 10 Olivier Fourdan editbugs 2015-02-26 15:41:05 CET
(In reply to John Lindgren from comment #9)
> 68365da didn't help.  The logical screen size is larger than what
> myScreenComputeSize() computes, not smaller, so taking the minimum doesn't
> make any difference.
> 
> Was there a problem with the original patch?  I think it's best to use the
> logical screen size to calculate the struts, without regard for the actual
> monitor size(s), and to leave myScreenComputeSize() as it was.

I don't understand what you mean by "it's best to use the logical screen size to calculate the struts" because gdk_screen_get_width() and gdk_screen_get_height() report their values from the X screen size, which is the same as what was done before the fix for bug 5795.

So if we are to believe gdk_screen_get_width() and gdk_screen_get_height() then we go back to what was before the fix for bug 5795.
Comment 11 John Lindgren editbugs 2015-02-26 19:15:34 CET
Well, you seem to want to use either the logical screen size everywhere (as before bug 5795), or the actual monitor geometry everywhere (as in the current code).  I don't think either of those will work correctly.  For most purposes, we want to use the monitor geometry to make sure that windows are really onscreen.  Only for the specific purpose of converting the _NET_WM_STRUT_PARTIAL hint to absolute coordinates, we need to use the logical screen size, because that's how the hint is defined.  That's why my patch only uses gdk_screen_get_width/height() calls for that one calculation.  I intentionally didn't change myScreenComputeSize(), since that would affect other calculations as well and undo the fix for bug 5795.

Does that make any more sense?
Comment 12 Olivier Fourdan editbugs 2015-02-26 21:14:31 CET
(In reply to John Lindgren from comment #11)
> Well, you seem to want to use either the logical screen size everywhere (as
> before bug 5795), or the actual monitor geometry everywhere (as in the
> current code).  I don't think either of those will work correctly.  For most
> purposes, we want to use the monitor geometry to make sure that windows are
> really onscreen.  Only for the specific purpose of converting the
> _NET_WM_STRUT_PARTIAL hint to absolute coordinates, we need to use the
> logical screen size, because that's how the hint is defined.  That's why my
> patch only uses gdk_screen_get_width/height() calls for that one
> calculation.  I intentionally didn't change myScreenComputeSize(), since
> that would affect other calculations as well and undo the fix for bug 5795.
> 
> Does that make any more sense?

Err, nope.

There are two main things, the screen size, which is the size of the root window which includes all individual monitors, and the individual monitor sizes.

Struts and partial struts are by definition relative to *screen* size, not monitor size, as stated in the EWMH spec (that's what I referred in comment 4).

gdk_screen_get_width/height() return the *screen* size, not the monitor size. It's actually the same as what myScreenComputeSize() does, except that myScreenComputeSize() recomputes the screen size by adding up each monitor size (to work around bug 5795).

Your bug occurs because the VGA monitor still shows in the monitor list but is disconnected, and myScreenComputeSize() does not check if monitors are actually connected or on (that would require an additional API not present in gtk+, that's what I explained in comment 2).

myScreenComputeSize() is called on every XRandr events so that each layout change, monitor added/removed, resolution chamges will trigger myScreenComputeSize() and the screen size recomputed.

So the fix I added is to check if the values returned by gdk_screen_get_width/height() are actually smaller than the ones obtained by adding up the monitors' sizes.

The values are the same as the ones from gdk_screen_get_width/height() which is exactly what your patch does (except that your patch calls these every time whereas myScreenComputeSize() store these in a structure so we don't have to call gdk each and every time.
Comment 13 Olivier Fourdan editbugs 2015-02-26 21:42:17 CET
Ok, I've read again both bug 11058 and bug 11059 and I think you're trying to work around bugs in either the panel or in the drivers.

Back to basics, struts and partial struts are relative to screen/rootwin size. So if the screen/rootwin size is correct, there there cannot be a bug.

I suspect your patch in bug 11058 does indeed place the panel at the right position but the panel still computes the strut size based on the wrong screen size, which explains why the struts are too large and therefore cause this bug.

Can you post the values of "xprop _NET_WM_STRUT_PARTIAL" taken on the panel before and after unplugging the monitor?

The 4th value, ie bottom strut should be the same because the panel is the same height. If not, then there is a bug in the panel.
Comment 14 John Lindgren editbugs 2015-02-27 00:46:53 CET
(In reply to Olivier Fourdan from comment #13)
> Ok, I've read again both bug 11058 and bug 11059 and I think you're trying
> to work around bugs in either the panel or in the drivers.

I said this already on bug 11058:

"If you think Xorg or the driver should be ensuring that the logical screen size remains consistent with the connected monitors, then I'm willing to report a bug to the relevant component (but I wouldn't know which)."

> Back to basics, struts and partial struts are relative to screen/rootwin
> size. So if the screen/rootwin size is correct, there there cannot be a bug.
> 
> I suspect your patch in bug 11058 does indeed place the panel at the right
> position but the panel still computes the strut size based on the wrong
> screen size, which explains why the struts are too large and therefore cause
> this bug.

See https://bugzilla.xfce.org/show_bug.cgi?id=11058#c15.

> Can you post the values of "xprop _NET_WM_STRUT_PARTIAL" taken on the panel
> before and after unplugging the monitor?
> 
> The 4th value, ie bottom strut should be the same because the panel is the
> same height. If not, then there is a bug in the panel.

They are the same before and after.  In each case the strut is 279, which is logical screen height (1024) minus laptop display height (768) plus panel height (23).

_NET_WM_STRUT_PARTIAL(CARDINAL) = 0, 0, 0, 279, 0, 0, 0, 0, 0, 0, 0, 1365
_NET_WM_STRUT_PARTIAL(CARDINAL) = 0, 0, 0, 279, 0, 0, 0, 0, 0, 0, 0, 1365

However, after running xrandr to reset the logical screen size to the same as the laptop display, then the strut is 23, which is just the height of the panel.

_NET_WM_STRUT_PARTIAL(CARDINAL) = 0, 0, 0, 23, 0, 0, 0, 0, 0, 0, 0, 1365
Comment 15 John Lindgren editbugs 2015-02-27 00:51:57 CET
For what it's worth, I just tried a couple different window managers to see what they would do in this case (with my patch applied to xfce4-panel).  OpenBox behaves in the way I think it should (interprets the strut relative to the "incorrect" X11 screen size).  Metacity dies with a segfault ...
Comment 16 John Lindgren editbugs 2015-02-27 02:16:52 CET
(In reply to Olivier Fourdan from comment #12)
> Your bug occurs because the VGA monitor still shows in the monitor list but
> is disconnected, and myScreenComputeSize() does not check if monitors are
> actually connected or on (that would require an additional API not present
> in gtk+, that's what I explained in comment 2).

Actually this is not true: gdk_screen_get_n_monitors() does check whether monitors are connected, and returns only the number of connected monitors.  I tested this to be the case at least with GTK+ 2.24.26.

> So the fix I added is to check if the values returned by
> gdk_screen_get_width/height() are actually smaller than the ones obtained by
> adding up the monitors' sizes.

But this is where you have it backwards.  Actually the values returned by adding up the monitor sizes end up being smaller (more "correct", if you like).  It's the values returned by gdk_screen_get_width/height() that do not change, because the logical screen size is not updated when the VGA monitor is disconnected.

Now are we on the same page?
Comment 17 Olivier Fourdan editbugs 2015-02-27 11:02:46 CET
(In reply to John Lindgren from comment #16)
> Now are we on the same page?

We agree on the symptoms and root cause, but not on the solution.

The problem comes from the discrepancy between the screen size (let's call it logical screen size as you did, although it's nothing logical to me) and the actual size of all outputs combined.

The panel was using the logical screen size to position itself and set the struts accordingly (bug 11058) - Struts are relative to the screen size, so that works, the location matched the logical size so the struts where correct.

You posted a patch to relocate the panel at the output boundaries (bug 11058) but left the computation of the struts based on the logical screen size so the struts reported are now much larger than before (because of that discrepancy).

xfwm4 does not use the logical screen size because of bug 5795 but always use the output boundaries (which are always correct unless proven otherwise). And this is where the problem arise, applying struts that were computed on a larger area wastes spaces.

I think we agree on all of the above.

How do we fix this?

1. Leave everything as it is, the panel is misplaced in some specific cases but the struts and location are consistent - This is the status-quo.

2. Modify the panel in a consistent way, ie place the panel according to output boundaries and compute the struts based on the same output boundaries, that means fixing the patch in bug 11058 and leave xfwm4 as it is.

3. Modify the panel as you did in bug 11058, but then this needs a change in xfwm4 as well but your patch in bug 11059 is far from being complete, it needs changing everywhere struts are involved, not just in one place.

I'll work on solution #3 as I expect other panels to be as broken as xfce4-panel (even though I still reckon xfwm4 is not broken).
Comment 18 John Lindgren editbugs 2015-02-27 14:18:46 CET
(In reply to Olivier Fourdan from comment #17)
> The problem comes from the discrepancy between the screen size (let's call
> it logical screen size as you did, although it's nothing logical to me) and
> the actual size of all outputs combined.

Sorry if my word choice was confusing; "nominal" probably would have been clearer.

> 3. Modify the panel as you did in bug 11058, but then this needs a change in
> xfwm4 as well but your patch in bug 11059 is far from being complete, it
> needs changing everywhere struts are involved, not just in one place.
> 
> I'll work on solution #3 as I expect other panels to be as broken as
> xfce4-panel (even though I still reckon xfwm4 is not broken).

This sounds like the best solution to me.  Thank you!
Comment 19 Olivier Fourdan editbugs 2015-02-28 10:08:52 CET
Fix for this pushed to git.
Comment 20 John Lindgren editbugs 2015-02-28 15:25:16 CET
I tested the Git version and it is working great!
Comment 21 John Lindgren editbugs 2015-03-02 06:25:57 CET
(In reply to John Lindgren from comment #20)
> I tested the Git version and it is working great!

Spoke too soon:
https://bugzilla.gnome.org/show_bug.cgi?id=745406
Comment 22 Olivier Fourdan editbugs 2015-03-11 09:25:03 CET
(In reply to John Lindgren from comment #21)
> Spoke too soon:
> https://bugzilla.gnome.org/show_bug.cgi?id=745406

Can you check with http://git.xfce.org/xfce/xfwm4/commit/?h=xfce-4.12&id=b861c3 ?

It seems it works for me.
Comment 23 John Lindgren editbugs 2015-03-14 18:49:08 CET
Works for me, thanks!

Bug #11059

Reported by:
John Lindgren
Reported on: 2014-08-01
Last modified on: 2015-03-14

People

Assignee:
Olivier Fourdan
CC List:
0 users

Version

Version:
4.10.1

Attachments

Additional information