window: sync up command line option and code behavior.

[Ionic]

The description for --no-force-fullscreen explicitly states that it
disables forcing fullscreen mode on windows that have no decorations
and are screen-sized.

Up until now, the code directly contradicted that by running the part
only if either the decorations where enabled or no CSD being used.

That causes multiple problems.

First, modern video players, which can certainly not be regarded as
"legacy applications", that try to show the video window as big as
possible and hence set the extents to exactly the screen size, including
using static gravity (so that borders are NOT removed from the window
size automatically) but still request window decorations are wrongly forced into
fullscreen mode by this hack. Changing the gravity would work around
that problem because the window size is effectively lowered, but we
can't expect applications to work around bugs in marco.

Secondly, the check for window decorations (or not) gets completely
cancelled out by the OR'd check for no CSD. According to the original
commit message, this hasn't been the intention, but rather to NOT force
fullscreen mode on windows that have no decorations and are CSD. It was
meant well, but the logic behind the change failed to do what it was
supposed to do.

Hence, fix that by checking if an application explicitly disabled window
decorations AND is not CSD.

This syncs up the help description and code behavior and fixes the CSD
part, but note that, naturally, it also changes the general behavior.
Specifically, this might break cases in which applications were supposed
to be forced into fullscreen mode even though they didn't explicitly
disable window decorations. I'd argue that such behavior would rather be
a bug and not a feature, though.

[vkareh]

This is confusing to me. Your description talks about the description for --no-force-fullscreen, but the code in question is specifically if (meta_prefs_get_force_fullscreen()...), so we're trying to address the opposite behavior?

Also, that block seems to want to address bugs with legacy apps that force window size to the same as the monitor without structs, but your comment is regarding modern apps with CSD.

Finally, your code just removes the case of windows being decorated:
decorated || !csd is logically equivalent to decorated || (!decorated && !csd)
Removing the case when windows are decorated, combined the fact that this is for the case where --no-force-fullscreen is FALSE, it seems that something piece of information is missing somewhere.

So, is there a specific bug that we can reproduce? What app, called in what way, with what workspace configuration, can we use to reproduce this issue?

[Ionic]

This is confusing to me.

I know, all this stuff is a bit complex and took me a while to understand as well.

Your description talks about the description for --no-force-fullscreen, but the code in question is specifically if (meta_prefs_get_force_fullscreen()...), so we're trying to address the opposite behavior?

Essentially I am fixing the non-disabled version, i.e., default case. Yes, I'm talking about the --no-force-fullscreen option that is essentially inverted in the code.

The documentation explicitly states:

Do not create fullscreen windows without decorations.

This directly contradicts the code: it will force fullscreen mode if --no-force-fullscreen was not provided and if the window has decorations (or is not CSD).

Also, that block seems to want to address bugs with legacy apps that force window size to the same as the monitor without structs, but your comment is regarding modern apps with CSD.

I'm referring to the comment directly above this code, which reads:

      /* Workaround braindead legacy apps that don't know how to
       * fullscreen themselves properly - don't get fooled by
       * fullscreen themselves properly - don't get fooled by
       * windows which are client decorated; that's not the same
       * windows which are client decorated; that's not the same
       * as fullscreen, even if there are no struts making the
       * as fullscreen, even if there are no struts making the
       * workarea smaller than the monitor.
       * workarea smaller than the monitor.
       */

Finally, your code just removes the case of windows being decorated:
decorated || !csd is logically equivalent to decorated || (!decorated && !csd)

It would be, and that would be wrong, if it was written like that, but take a closer look at it again. :)

So, is there a specific bug that we can reproduce? What app, called in what way, with what workspace configuration, can we use to reproduce this issue?

Reproducing it is pretty easy:
1.) use a recent version of mpv (I'm currently on commit SHA2 cdd6eb0994; any later commit should likewise work to reproduce the bug, but the latest 0.32.0 release will not reproduce it yet)
2.) on a FHD display, open a FHD video file. alternatively, for any other resolution, open a video file with the same resolution
3.) notice how the window is fullscreened from the beginning due to the buggy "legacy application fullscreen detection" code, although the player never intended for the window to be fullscreened - instead, it should have started fully decorated (and, usually, scaled down because of upper/lower panels restricting the max window size)

It makes sense for marco to detect situations in which applications explicitly disable the window decorations, change the extents to the screen resolution and have the WM automatically fullscreen the window, but in that case, it definitely shouldn't happen.

Some other piece of information I have slightly omitted: Quoting from 13741f5:

We try to exempt CSD windows from being forced fullscreen if they are
undecorated and the size of the screen; however, we also catch almost
all windows that do need to be forced fullscreen in this check, since
they also have decorations turned off.

So, unless my interpretation is way off, the original intention was to exempt CSD windows (which trivially are undecorated), but also catch any other undecorated window.

Looking at the code, however, the original intention wasn't carried out correctly. CSD windows were (correctly) exempted, as described, but caught decorated non-CSD windows instead.

This came from 1b63865, which already did try to exempt CSD by checking if the window is decorated and actually introduced this bug. The previously mentioned commit tried to fix it up, and did that, but only really fixed it for CSD windows, leaving the breakage for non-CSD windows in place.

I hope that this clears up a few things.

[Ionic]

Oh, one more thing...

Also, that block seems to want to address bugs with legacy apps that force window size to the same as the monitor without structs, but your comment is regarding modern apps with CSD.

"Legacy apps" in that (comment) context doesn't refer to "legacy, as in non-CSD" vs. "modern, as in CSD" windows, but even "more legacy", broken applications, like Adobe Reader, which seems to have behaved in very weird ways. Thankfully, Adobe Reader has been dead for quite some time now, but there are probably still some applications around that behave badly. Java-based stuff for instance has a reputation for very... creative X11 behavior, to say the least.

A modern video player, like mpv, can certainly not be regarded as such a legacy, braindead application, which was what I alluded to. :)

[lukefromdc]

I don't think I have ANY of the apps in question installed, but the code change is extremely simple (one conditional becomes and instead of or and we use the negation on both of the first two checks. Builds and runs fine, but I have no idea how to really test it with no java apps, no Adobe reader etc. MPV's behavior on a 4K video matching a 4K screem was totally unchanged.