bug#63511: [Patch] Fix for hidden window bug

[Christian Schmidt]

On 8/24/23 02:12, Po Lu wrote:
> Stefan Kangas <stefankangas@gmail.com> writes:
>
> > Christian Schmidt <cs@aibiot.de> writes:
> >
> > > At least on enlightenment, when switching virtual desktop away from the
> > > on containing emacs and back, emacs does not render screen updates.
> > > This patch fixes the cause.
> > > Analysis for the bug tracker, copied from commit message:
> > >
> > > If no state flags are set in the _NET_WM_STATE property,
> > > the returned X property type will be "0". This situation
> > > occurs when "_NET_WM_STATE_HIDDEN" was the only property
> > > set for the emacs window, e.g. in enlightenment due to
> > > change of a virtual desktop.
>
> Does Enlightenment _remove_ the property, or merely set its type to
> something other than ATOM?

prop->type == (Atom) 0 IMHO refers to an empty list, which is valid.

> > Po, could you take a look at this patch please.
>
> That's a bug in Enlightenment, as the type of _NET_WM_STATE should
> always be set to an array of atoms irrespective of its contents.

I would be surprised if this was a bug in E, given emacs is the only 
program I know of that exhibits this issue. I have discussed this issue 
extensively with raster, the main author of E, and we have come to the 
same conclusion: when _NET_WM_STATE_HIDDEN  the is the only active/set 
property, and then is removed/unset (sorry if my terminology is wrong, 
my days of X programming have been 20+ years ago, and these are corners 
I have never touched before), there is no code path in emacs that can 
handle this case. Also, the request does not hit the window manager, but 
it is X that replies. As such, the bug would lie within X11.

What happens is that the array of atoms is empty, and thus the list is 
(Atom) 0. Currently this is interpreted as "keep the current state", and 
not to re-evaluate the (now empty) list of atoms to realize that 
_NET_WM_STATE_HIDDEN is not set.

The scenario in which this occurs is a switch of a virtual desktop in E. 
E will only hide affected windows, not iconify them, which seems 
correct. E uses compositing and updates even windows on other virtual 
desktops in the pager when (and if) they redraw while hidden.

> If the window manager outright removes the property, clients have no
> means to ascertain if the window manager has relinquished control over
> the window, or it is still iconified by some other means (such as the
> traditional WM_STATE).

If that would be an issue, given there is no "unhidden" property that 
could be set, how should the WM handle this? E sets _NET_WM_STATE_HIDDEN 
when the window is hidden, and removes it when no longer hidden.

> I would prefer a check against that other property when that is the
> case, rather than overriding the results of the loop below.

In summary: Two scenarios currently can cause SET_FRAME_ICONIFIED (f, 
true): _NET_WM_STATE_HIDDEN and wm_state in Iconic_State. A removal of a 
final property _NET_WM_STATE_HIDDEN does not trigger SET_FRAME_ICONIFIED 
(f, false), because of what appears to me like a misinterpretation of an 
empty Atom list.

However, as long as any other property, e.g. sticky, would be set, 
removing _NET_WM_STATE_HIDDEN would behave like the patched version.

I would agree that the check could be reasonable, though I can't really 
imagine a situation in which iconified+hidden is set, yet only hidden 
would be removed. In fact, the implementation notes give iconified as 
the typical use case of hidden, so these would go hand in hand.

This patch only adds correct handling of an empty _NET_WM_STATE Atom 
list, and does not otherwise alter behavior as described above. The 
unconditional initialization of actual_size seems ok to me as the 
alternative would be to have it set twice, once in the else tree, and 
secondly in another new else tree. I'd say both from code size and 
runtime performance this should be the better option.

Best regards,
Christian