[PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[Tom Gillespie]

[-- Attachment #1: Type: text/plain, Size: 2115 bytes --]

Hi,
    This patch fixes a bug that occurs when running Emacs in a window
manager that does not set _NET_WM_STATE. The patch is simplest fix
that restores expected behavior for window managers that do not set
_NET_WM_STATE, but I have no idea whether there are other issues that
may be occurring.

I'm guessing that there are also likely issues with other logic in the
MapNotify case that uses not_hidden for window managers that does not
set _NET_WM_STATE.

Thus, the more complex solution, which I am not knowledgeable enough
to implement correctly, is to fix x_get_current_wm_state so that it
works on window managers that do not set _NET_WM_STATE.

I am unfamiliar with this section of the code, so my explanation may
be off and a sanity check would be appreciated. Best,
Tom


A description of the bug for the record.

The bug manifests as frames rendering as solid background after
changing desktops on window managers that do not set _NET_WM_STATE.

When changing desktops and returning to a desktop with Emacs frames,
any frame that is not focused will render the whole frame as only the
background color for the current theme.

I have run a git bisect and found that the first bad commit is
483c5e953c12a95382bef4a3b6769a680c32fe86 Fix two GTK3 event handling
issues.

I think that the bug is occurring because 483c5e953c1 changed calls to
SET_FRAME_VISIBLE and friends from being unconditional to being
conditional on not_hidden, which returns incorrect information when
_NET_WM_STATE is not set (I think).

I encountered the bug on fluxbox but the behavior should happen for
any window manager that does not set _NET_WM_STATE.

It should be possible to reproduce the bug using the following steps.
Run a window manager that does not set _NET_WM_STATE, e.g. fluxbox.
In a terminal run the following and when the newly built Emacs
launches alt-tab to the terminal window, and then leave and return to
the desktop with the Emacs frame.
#+begin_src bash
pushd ~/git/emacs
git checkout 483c5e953c12a95382bef4a3b6769a680c32fe86
git clean -dfx && ./autogen.sh && ./configure && make && \
src/emacs -q
#+end_src

[-- Attachment #2: 0001-Fix-rendering-issues-on-window-managers-without-_NET.patch --]
[-- Type: text/x-patch, Size: 2024 bytes --]

From a12f4a133aa4697494cdb04fbaaef2471fe72269 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Fri, 14 May 2021 18:47:48 -0700
Subject: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

* src/xterm.c (handle_one_xevent): MapNotify handle cases where window
manager is missing _NET_WM_STATE. Partially reverts a change
introduced in 483c5e953c12a95382bef4a3b6769a680c32fe86 which made
previously unconditional calls to SET_FRAME_VISIBLE, and
SET_FRAME_ICONIFIED conditional on the not_hidden variable. On window
managers missing _NET_WM_STATE it seems that not_hidden = false does
not imply visible = false because x_get_current_wm_state will return
false when _NET_WM_STATE and a frame is visible.
---
 src/xterm.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/src/xterm.c b/src/xterm.c
index bdf0804f89..67c6eaa719 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -8440,22 +8440,19 @@ handle_one_xevent (struct x_display_info *dpyinfo,
 		x_set_z_group (f, Qbelow, Qnil);
 	    }
 
-	  if (not_hidden)
-	    {
-	      SET_FRAME_VISIBLE (f, 1);
-	      SET_FRAME_ICONIFIED (f, false);
+	    SET_FRAME_VISIBLE (f, 1);
+	    SET_FRAME_ICONIFIED (f, false);
 #if defined USE_GTK && defined HAVE_GTK3
-	      /* If GTK3 wants to impose some old size here (Bug#24526),
-		 tell it that the current size is what we want.  */
-	      if (f->was_invisible)
-		{
-		  xg_frame_set_char_size
-		    (f, FRAME_PIXEL_WIDTH (f), FRAME_PIXEL_HEIGHT (f));
-		  f->was_invisible = false;
-		}
-#endif
-	      f->output_data.x->has_been_visible = true;
+	    /* If GTK3 wants to impose some old size here (Bug#24526),
+		tell it that the current size is what we want.  */
+	    if (f->was_invisible)
+	    {
+		xg_frame_set_char_size
+		(f, FRAME_PIXEL_WIDTH (f), FRAME_PIXEL_HEIGHT (f));
+		f->was_invisible = false;
 	    }
+#endif
+	    f->output_data.x->has_been_visible = true;
 
           if (not_hidden && iconified)
             {
-- 
2.26.3

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[Omar Polo]

Tom Gillespie <tgbugs@gmail.com> writes:

> Hi,
>     This patch fixes a bug that occurs when running Emacs in a window
> manager that does not set _NET_WM_STATE. The patch is simplest fix
> that restores expected behavior for window managers that do not set
> _NET_WM_STATE, but I have no idea whether there are other issues that
> may be occurring.
>
> I'm guessing that there are also likely issues with other logic in the
> MapNotify case that uses not_hidden for window managers that does not
> set _NET_WM_STATE.
>
> Thus, the more complex solution, which I am not knowledgeable enough
> to implement correctly, is to fix x_get_current_wm_state so that it
> works on window managers that do not set _NET_WM_STATE.
>
> I am unfamiliar with this section of the code, so my explanation may
> be off and a sanity check would be appreciated. Best,
> Tom
>
>
> A description of the bug for the record.
>
> The bug manifests as frames rendering as solid background after
> changing desktops on window managers that do not set _NET_WM_STATE.
>
> When changing desktops and returning to a desktop with Emacs frames,
> any frame that is not focused will render the whole frame as only the
> background color for the current theme.
>
> I have run a git bisect and found that the first bad commit is
> 483c5e953c12a95382bef4a3b6769a680c32fe86 Fix two GTK3 event handling
> issues.
>
> I think that the bug is occurring because 483c5e953c1 changed calls to
> SET_FRAME_VISIBLE and friends from being unconditional to being
> conditional on not_hidden, which returns incorrect information when
> _NET_WM_STATE is not set (I think).
>
> I encountered the bug on fluxbox but the behavior should happen for
> any window manager that does not set _NET_WM_STATE.

I can confirm that the bug was present also with cwm and that your patch
fixes it, thanks!!

> It should be possible to reproduce the bug using the following steps.
> Run a window manager that does not set _NET_WM_STATE, e.g. fluxbox.
> In a terminal run the following and when the newly built Emacs
> launches alt-tab to the terminal window, and then leave and return to
> the desktop with the Emacs frame.
> #+begin_src bash
> pushd ~/git/emacs
> git checkout 483c5e953c12a95382bef4a3b6769a680c32fe86
> git clean -dfx && ./autogen.sh && ./configure && make && \
> src/emacs -q
> #+end_src

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[martin rudalics]

 > I can confirm that the bug was present also with cwm and that your patch
 > fixes it, thanks!!

Omar, does it work again for you with latest master?

martin

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[Omar Polo]

martin rudalics <rudalics@gmx.at> writes:

>> I can confirm that the bug was present also with cwm and that your patch
>> fixes it, thanks!!
>
> Omar, does it work again for you with latest master?
>
> martin

Sorry for the delay,

Yes, I've compiled from the master (as of now commit bf8b8cc6), tested
with emacs -Q and it works on CWM.

Thanks!

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[martin rudalics]

 > Yes, I've compiled from the master (as of now commit bf8b8cc6), tested
 > with emacs -Q and it works on CWM.

Thanks for the information.  CWM is _not_ a tiling WM, right?

martin

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[Omar Polo]

martin rudalics <rudalics@gmx.at> writes:

>> Yes, I've compiled from the master (as of now commit bf8b8cc6), tested
>> with emacs -Q and it works on CWM.
>
> Thanks for the information.  CWM is _not_ a tiling WM, right?
>
> martin

No, it's a floating window manager

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[Tom Gillespie]

Reporting back on an observation which may or may not be related to
these changes. Now when I alt-tab, or disable/enable the menu bar
focus switches away from the minibuffer if it was active and switches
to the last active maxi buffer. I haven't had a chance to bisect so I
don't know what introduced the issue, it might be
46e4704e2abca4d264a43965f92eab7608211ee6 Miscellaneous corrections to
src/minibuf.c as well (I'm at
b5e6dba05fcb4fa1c716dc759f8c4b2561bacaa7). Best,
Tom

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[martin rudalics]

 > Reporting back on an observation which may or may not be related to
 > these changes. Now when I alt-tab, or disable/enable the menu bar
 > focus switches away from the minibuffer if it was active and switches
 > to the last active maxi buffer. I haven't had a chance to bisect so I
 > don't know what introduced the issue, it might be
 > 46e4704e2abca4d264a43965f92eab7608211ee6 Miscellaneous corrections to
 > src/minibuf.c as well (I'm at
 > b5e6dba05fcb4fa1c716dc759f8c4b2561bacaa7). Best,

Does this happen with emacs -Q?  When I do here C-h f, alt-tab away from
Emacs, alt-tab back to Emacs, I'm still in the minibuffer.  Same with
C-h f and deactivating the menu bar from the Options menu.  So please be
more specific about what you did.

Thanks, martin

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[Basil L. Contovounesios]

martin rudalics <rudalics@gmx.at> writes:

>> Reporting back on an observation which may or may not be related to
>> these changes. Now when I alt-tab, or disable/enable the menu bar
>> focus switches away from the minibuffer if it was active and switches
>> to the last active maxi buffer. I haven't had a chance to bisect so I
>> don't know what introduced the issue, it might be
>> 46e4704e2abca4d264a43965f92eab7608211ee6 Miscellaneous corrections to
>> src/minibuf.c as well (I'm at
>> b5e6dba05fcb4fa1c716dc759f8c4b2561bacaa7). Best,
>
> Does this happen with emacs -Q?  When I do here C-h f, alt-tab away from
> Emacs, alt-tab back to Emacs, I'm still in the minibuffer.  Same with
> C-h f and deactivating the menu bar from the Options menu.  So please be
> more specific about what you did.

I see the same as Tom with Xmonad.  In an empty workspace:

0. emacs -Q
1. C-x 5 2
2. M-x
   [Minibuffer is current.]
3. Alt-tab
   [*scratch* is current.]
4. Alt-tab
   [*scratch* is still current.]

In Emacs 27, the last step leaves the minibuffer current.

-- 
Basil

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[martin rudalics]

 > I see the same as Tom with Xmonad.  In an empty workspace:
 >
 > 0. emacs -Q
 > 1. C-x 5 2
 > 2. M-x
 >     [Minibuffer is current.]
 > 3. Alt-tab
 >     [*scratch* is current.]
 > 4. Alt-tab
 >     [*scratch* is still current.]
 >
 > In Emacs 27, the last step leaves the minibuffer current.

With C-x 5 2 I see that here as well.  Do you have a scenario where the
menu bar is involved, too?

martin

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[Basil L. Contovounesios]

martin rudalics <rudalics@gmx.at> writes:

>> I see the same as Tom with Xmonad.  In an empty workspace:
>>
>> 0. emacs -Q
>> 1. C-x 5 2
>> 2. M-x
>>     [Minibuffer is current.]
>> 3. Alt-tab
>>     [*scratch* is current.]
>> 4. Alt-tab
>>     [*scratch* is still current.]
>>
>> In Emacs 27, the last step leaves the minibuffer current.
>
> With C-x 5 2 I see that here as well.  Do you have a scenario where the
> menu bar is involved, too?

Sorry, I don't normally use the menu bar, and I can't guess how Tom is
using it in relation to the minibuffer.  I tried the following:

0. emacs -Q
1. M-x global-set-key RET C-c m menu-bar-mode RET
2. M-x C-c m C-c m

But the minibuffer remains current throughout.

-- 
Basil

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[martin rudalics]

 >      This patch fixes a bug that occurs when running Emacs in a window
 > manager that does not set _NET_WM_STATE. The patch is simplest fix
 > that restores expected behavior for window managers that do not set
 > _NET_WM_STATE, but I have no idea whether there are other issues that
 > may be occurring.
 >
 > I'm guessing that there are also likely issues with other logic in the
 > MapNotify case that uses not_hidden for window managers that does not
 > set _NET_WM_STATE.
 >
 > Thus, the more complex solution, which I am not knowledgeable enough
 > to implement correctly, is to fix x_get_current_wm_state so that it
 > works on window managers that do not set _NET_WM_STATE.
 >
 > I am unfamiliar with this section of the code, so my explanation may
 > be off and a sanity check would be appreciated. Best,

Thanks.  This explains why our MapNotify code was not able to handle
visibility correctly with some WMs.  Actually, this also means that we
should get rid of x_get_current_wm_state because it apparently fails in
too many cases.

Unfortunately, reverting parts of 483c5e953c1 as in your patch will only
reintroduce the bug 483c5e953c1 was trying to fix.  I meanwhile checked
in a different solution based on handling VisibilityNotify.  Please try
with latest master and tell us whether the problem still exists.

Thank you, martin

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[Tom Gillespie]

Hi Martin,
    Reporting that the solution based on VisibilityNotify resolves my
issue. Many thanks!
Tom

Re: [PATCH] Fix rendering issues on window managers without _NET_WM_STATE

[martin rudalics]

 >      Reporting that the solution based on VisibilityNotify resolves my
 > issue. Many thanks!

And thanks for identifying the missing setting of _NET_WM_STATE as the
culprit.  IIUC this means that quite a number of WMs (fluxbox, Xmonad,
Emacs' own EXWM, cwm) are not treated well by our calls of
x_get_current_wm_state and x_handle_net_wm_state in handle_one_xevent
and we probably should do away with that.

Adam, given the above, can you think of fixing Bug#9893 in a way that is
not based on _NET_WM_STATE?

martin