bakkeby / patches

Collection of patches for dwm, st and dmenu
285 stars 30 forks source link

swallow + fullscreen bug #85

Open 2084x opened 1 month ago

2084x commented 1 month ago

I'm experiencing this using dwm with only the swallow and togglefullscreen patches applied.

when a window is unswallowed the fullscreen state does not carry over from the swallowed window. I assume this makes dwm think you are in the fullscreen state that the window had when it was initially swallowed.

you can observe this by doing the following:

  1. open terminal and swallow it
  2. set fullscreen
  3. unswallow
  4. fullscreen is now automatically unset and window is floating(?)

or

  1. open terminal and set fullscreen
  2. swallow it
  3. unset fullscreen
  4. unswallow
  5. border has now dissapeared for all windows and won't come back until the terminal that was swallowed is closed.

ideally when the window becomes unswallowed it should retain the fullscreen state that the swallowed window had so that windows don't randomly float or have their borders disappear.

bakkeby commented 1 month ago

Fullscreen is a sort of special state and what is the ideal handling of those in relation to window swallowing may depend on the perspective of the user.

In the swallow patch there is no special handling of the fullscreen state and I would agree that the current behaviour (of leaving a floating window that spans the entire monitor) is not great and that this should be changed to a behaviour that is more intuitive for daily use.

If the fullscreen state is retained on swallow / unswallow then that makes the behaviour very easy to explain and to grasp, but I'd question how practical such a solution would be.

Let's say that you open a (floating) terminal and you open an image using nsxiv - and you tell nsxiv to open in fullscreen.

$ nsxiv -f <image.png>

In the context of window swallowing you could either expect:

The first option retains the floating state and the second doesn't. This is something that came up recently in https://github.com/bakkeby/dwm-flexipatch/issues/430 (in relation to the original swallow patch) and one option is to not allow window swallowing if the new window opens in fullscreen.

The other two scenarios that you mentioned:

  1. open terminal and swallow it
  2. set fullscreen
  3. unswallow
  4. fullscreen is now automatically unset and window is floating(?)

In this case I am wondering if there is a legitimate case where quitting a fullscreen window and ending up in a fullscreen terminal is practical.

I had a situation a couple of months back where I was using a command to generate images and opening them to see the results. Sometimes the generated image would be very large and I would fullscreen the image viewer to see the image in full, then exit after having inspected the results.

This would leave a fullscreen terminal (or a floating window the size of the screen) which was quite disruptive to the workflow. Confusing actually. In that scenario it was much more practical for me that the terminal window would be restored in the original position (tiled or floating) rather than adhering to the fullscreen state of the other window.

  1. open terminal and set fullscreen
  2. swallow it
  3. unset fullscreen
  4. unswallow
  5. border has now dissapeared for all windows and won't come back until the terminal that was swallowed is closed.

Problematic. I am wondering if a fullscreen terminal is an edge case or if many people use this workflow.

My first impression is that if the terminal window is fullscreen at the time of swallowing then it would be intuitive that the new window is also fullscreen. On the other hand you could have a window that has a fixed size that doesn't support being fullscreened, could it be that it is more practical that the fullscreen terminal disappears and the new window appears as normal (tiled or floating)?

If we go with the first option (inheriting fullscreen) and you then immediately exit I am thinking that restoring the terminal in fullscreen is more intuitive.

But if you exit fullscreen beforehand then it would make more sense for the terminal to be restored without fullscreen.

It is also worth mentioning that if the fullscreen state is simply inherited on swallow / unswallow, and you do not use the toggle fullscreen patch, then you can get into situations where the terminal window is fullscreen and you can't exit fullscreen.

Just some of my thoughts on the topic.

bakkeby commented 1 month ago

Expected behaviour with swterminheritfs = 1 (default).

Terminal before Swallowing window >> State at unswallow Terminal after
Tiled Tiled >> Tiled Tiled
Tiled Tiled >> Floating (x y w h) Floating (x y w h)
Tiled Tiled >> Fullscreen (tiled) Fullscreen (tiled)
Tiled Tiled >> Fullscreen (floating) Fullscreen (floating)
Floating (x y w h) Floating (x y w h) >> Tiled Tiled
Floating (x y w h) Floating (x y w h) >> Floating (a b c d) Floating (a b c d)
Floating (x y w h) Floating (x y w h) >> Fullscreen (tiled) Fullscreen (tiled)
Floating (x y w h) Floating (x y w h) >> Fullscreen (floating) Fullscreen (floating)
Fullscreen (tiled) Fullscreen (tiled) >> Tiled Tiled
Fullscreen (tiled) Fullscreen (tiled) >> Floating (x y w h) Floating (x y w h)
Fullscreen (tiled) Fullscreen (tiled) >> Fullscreen (tiled) Fullscreen (tiled)
Fullscreen (tiled) Fullscreen (tiled) >> Fullscreen (floating) Fullscreen (floating)
Fullscreen (floating) Fullscreen (floating) >> Tiled Tiled
Fullscreen (floating) Fullscreen (floating) >> Floating (x y w h) Floating (x y w h)
Fullscreen (floating) Fullscreen (floating) >> Fullscreen (tiled) Fullscreen (tiled)
Fullscreen (floating) Fullscreen (floating) >> Fullscreen (floating) Fullscreen (floating)

Expected behaviour with swterminheritfs = 0.

Terminal before Swallowing window >> State at unswallow Terminal after
Tiled Tiled >> Tiled Tiled
Tiled Tiled >> Floating (x y w h) Floating (x y w h)
Tiled Tiled >> Fullscreen (tiled) Tiled
Tiled Tiled >> Fullscreen (floating) Floating (x y w h)
Floating (x y w h) Floating (x y w h) >> Tiled Tiled
Floating (x y w h) Floating (x y w h) >> Floating (a b c d) Floating (a b c d)
Floating (x y w h) Floating (x y w h) >> Fullscreen (tiled) Tiled
Floating (x y w h) Floating (x y w h) >> Fullscreen (floating) Floating (x y w h)
Fullscreen (tiled) Fullscreen (tiled) >> Tiled Tiled
Fullscreen (tiled) Fullscreen (tiled) >> Floating (x y w h) Floating (x y w h)
Fullscreen (tiled) Fullscreen (tiled) >> Fullscreen (tiled) Fullscreen (tiled)
Fullscreen (tiled) Fullscreen (tiled) >> Fullscreen (floating) Fullscreen (floating)
Fullscreen (floating) Fullscreen (floating) >> Tiled Tiled
Fullscreen (floating) Fullscreen (floating) >> Floating (x y w h) Floating (x y w h)
Fullscreen (floating) Fullscreen (floating) >> Fullscreen (tiled) Fullscreen (tiled)
Fullscreen (floating) Fullscreen (floating) >> Fullscreen (floating) Fullscreen (floating)

Trying to compile an overview of the expected states of the terminal and the swallowing window before and after window swallowing.

The correctness of the listed expectations is up for debate.

2084x commented 1 month ago

Let's say that you open a (floating) terminal and you open an image using nsxiv - and you tell nsxiv to open in fullscreen.

since I rarely float windows and only fullscreen with keybinds rather than cli args I didn't consider this case.

the nsxiv window to open in fullscreen

if I've specified -f I would always expect the program to open in fullscreen even if that meant not retaining the floating state.

one option is to not allow window swallowing if the new window opens in fullscreen.

it seems to me like this is already how the patch works though. if I run nsxiv -f <image> then unset fullscreen, I can still see the terminal there as if it hasn't been swallowed.

In that scenario it was much more practical for me that the terminal window would be restored in the original position (tiled or floating) rather than adhering to the fullscreen state of the other window.

on second thought I do agree with you and I don't think fullscreen being unset would annoy me in this sort of scenario. that being said, my main point of issue here was the fact that the window would end up floating when you unswallowed despite being tiled when it was swallowed.

Problematic. I am wondering if a fullscreen terminal is an edge case or if many people use this workflow.

it's definitely common for me. often I'll be using lf (a tui file manager) to browse files, opening and closing images and videos which makes the terminal swallow and unswallow as I go... I might be setting and unsetting fullscreen to change focus where necessary and not keeping track of what fullscreen state I last swallowed in and all of a sudden I encounter one of the two scenarios I described.

you could have a window that has a fixed size that doesn't support being fullscreened,

is this a common thing? my workflow is mostly tui based, but even with the few gui programs I use I've never encountered it. wouldn't it just be best to specify noswallow for those specific programs or windows?

could it be that it is more practical that the fullscreen terminal disappears and the new window appears as normal (tiled or floating)?

personally I would find it incredibly annoying if I was kicked out of fullscreen every time the terminal was swallowed. refer to the lf case I gave above.

If we go with the first option (inheriting fullscreen) and you then immediately exit I am thinking that restoring the terminal in fullscreen is more intuitive. But if you exit fullscreen beforehand then it would make more sense for the terminal to be restored without fullscreen.

ideally you'd want both. is there no way for the terminal to inherit the fullscreen / tiled / floating etc state when you unswallow so that both of these could be achieved?

It is also worth mentioning that if the fullscreen state is simply inherited on swallow / unswallow, and you do not use the toggle fullscreen patch, then you can get into situations where the terminal window is fullscreen and you can't exit fullscreen.

good point. this just makes me wish that fullscreen toggling was native in dwm haha... it's only a few simple lines of code :/

Trying to compile an overview of the expected states of the terminal and the swallowing window before and after window swallowing.

pretty sure I agree with all these. maybe there could be some debate about the third case reverting from fullscreen to tiled. also what's the difference between fullscreen (tiled) and fullscreen (floating)?

bakkeby commented 1 month ago

what's the difference between fullscreen (tiled) and fullscreen (floating)?

In the table I produced above there is no difference, but I thought it was better to write them out separately in case there would be some ambiguity between a fullscreen window that was previously tiled or floating.

I am going to try to experiment with creating a swallow patch that meets all of the criteria.

bakkeby commented 1 month ago

@2084x if you want to play around, I think these minor changes covers all cases.

diff --git a/config.def.h b/config.def.h
index 5dc3f22..c27a7f9 100644
--- a/config.def.h
+++ b/config.def.h
@@ -4,6 +4,7 @@
 static const unsigned int borderpx  = 1;        /* border pixel of windows */
 static const unsigned int snap      = 32;       /* snap pixel */
 static const int swallowfloating    = 0;        /* 1 means swallow floating windows by default */
+static const int swterminheritfs    = 0;        /* 1 terminal inherits fullscreen on unswallow, 0 otherwise */
 static const int showbar            = 1;        /* 0 means no bar */
 static const int topbar             = 1;        /* 0 means bottom bar */
 static const char *fonts[]          = { "monospace:size=10" };
diff --git a/dwm.c b/dwm.c
index 7aef964..644d5f8 100644
--- a/dwm.c
+++ b/dwm.c
@@ -1335,7 +1335,6 @@ replaceclient(Client *old, Client *new)

        new->mon = mon;
        new->tags = old->tags;
-       new->isfloating = old->isfloating;

        new->next = old->next;
        new->snext = old->snext;
@@ -1357,6 +1356,16 @@ replaceclient(Client *old, Client *new)
        old->next = NULL;
        old->snext = NULL;

+       if (old->isfullscreen == new->isfullscreen) {
+               new->isfloating = old->isfloating;
+       } else if (new->isfullscreen && old->isterminal) {
+               /* allow windows starting in fullscreen to retain fullscreen */
+       } else if (swterminheritfs || !new->isterminal || !old->isfullscreen) {
+               /* inherit fullscreen state from the other client */
+               new->oldstate = old->oldstate;
+               setfullscreen(new, old->isfullscreen);
+       }
+
        XMoveWindow(dpy, old->win, WIDTH(old) * -2, old->y);

        if (ISVISIBLE(new) && !new->isfullscreen) {
@@ -1763,12 +1772,9 @@ swallow(Client *t, Client *c)
 {
        if (c->noswallow || c->isterminal)
                return 0;
-       if (!swallowfloating && c->isfloating)
+       if (!swallowfloating && ((c->isfloating && !c->isfullscreen) || c->oldstate))
                return 0;

-       if (t->isfullscreen)
-               setfullscreen(c, 1);
-
        replaceclient(t, c);
        c->ignorecfgreqpos = 1;
        c->swallowing = t;

There is a config option swterminheritfs that controls whether a non-fullscreen terminal will inherit the fullscreen state of the other window. The variable name is quite horrible of course.

2084x commented 1 month ago

I tested all the cases in the table and everything works as expected except:

Fullscreen (tiled) Fullscreen (tiled) >> Floating (x y w h) Floating (x y w h)

in which case the terminal ends up tiled rather than floating.

bakkeby commented 1 month ago

That was a very good point.

I also discovered that it was a lot more complicated when taking all four states into account:

because you can have edge cases like:

I tried a variety of solutions primarily trying to sort out the state before inheriting fullscreen.

This got quite complex considering the 16 possible combinations and the special optional behaviour I wanted to offer where the terminal window does not inherit fullscreen unswallow.

In the end I worked out that it was easiest to just trigger fullscreen and then sort out the states afterwards.

diff --git a/config.def.h b/config.def.h
index 5dc3f22..776d579 100644
--- a/config.def.h
+++ b/config.def.h
@@ -4,6 +4,7 @@
 static const unsigned int borderpx  = 1;        /* border pixel of windows */
 static const unsigned int snap      = 32;       /* snap pixel */
 static const int swallowfloating    = 0;        /* 1 means swallow floating windows by default */
+static const int swterminheritfs    = 1;        /* 1 terminal inherits fullscreen on unswallow, 0 otherwise */
 static const int showbar            = 1;        /* 0 means no bar */
 static const int topbar             = 1;        /* 0 means bottom bar */
 static const char *fonts[]          = { "monospace:size=10" };
diff --git a/dwm.c b/dwm.c
index 7aef964..4ae052f 100644
--- a/dwm.c
+++ b/dwm.c
@@ -1335,21 +1335,20 @@ replaceclient(Client *old, Client *new)

        new->mon = mon;
        new->tags = old->tags;
-       new->isfloating = old->isfloating;

        new->next = old->next;
        new->snext = old->snext;

-       if (old == mon->clients)
+       if (old == mon->clients) {
                mon->clients = new;
-       else {
+       } else {
                for (c = mon->clients; c && c->next != old; c = c->next);
                c->next = new;
        }

-       if (old == mon->stack)
+       if (old == mon->stack) {
                mon->stack = new;
-       else {
+       } else {
                for (c = mon->stack; c && c->snext != old; c = c->snext);
                c->snext = new;
        }
@@ -1357,13 +1356,34 @@ replaceclient(Client *old, Client *new)
        old->next = NULL;
        old->snext = NULL;

+       if (!swterminheritfs && new->isterminal && !new->isfullscreen && old->isfullscreen) {
+               /* Do not allow a non-fullscreen terminal to inherit the fullscreen property of
+                * a windoww when unswallowed */
+               new->x = old->oldx;
+               new->y = old->oldy;
+               new->w = old->oldw;
+               new->h = old->oldh;
+               new->isfloating = old->oldstate;
+       } else {
+               setfullscreen(new, old->isfullscreen);
+               new->x = old->x;
+               new->y = old->y;
+               new->w = old->w;
+               new->h = old->h;
+               new->oldx = old->oldx;
+               new->oldy = old->oldy;
+               new->oldw = old->oldw;
+               new->oldh = old->oldh;
+               new->oldbw = old->oldbw;
+               new->bw = old->bw;
+               new->isfloating = old->isfloating;
+               new->oldstate = old->oldstate;
+       }
+
        XMoveWindow(dpy, old->win, WIDTH(old) * -2, old->y);

The setfullscreen call will be a no-op unless there is a change in fullscreen state.

In principle, because the state is not corrected before the setfullscreen call, it is possible that when losing fullscreen the terminal window is moved somewhere else (i.e. flickers) before ending up in the intended place. This would depend on when actions are synced with the X server. I didn't spot this in my testing, but it would be relatively straightforward to introdue a workaround for that (e.g. setting the oldx, oldy, etc. for the new client before the setfullscreen call).

I'll push an updated patch for the above.

2084x commented 1 month ago

it is possible that when losing fullscreen the terminal window is moved somewhere else (i.e. flickers) before ending up in the intended place

actually I noticed this with the first diff but it's gone after the most recent commits. every scenario in the table is working as expected too!

the only issue I can find now is when using cli args to start in fullscreen. for example nsxiv does not enter fullscreen at all when using -f. or for a program with size hints like mpv, using --fs sets fullscreen for the window but the video actually opens locked to the top left corner and has it's native resolution.

bakkeby commented 1 month ago

Had a play around, looks like there are three main scenarios:

At first I thought it was merely having an else if that does nothing for the second case, but in that case we don't properly propagate the floating state and position of the window for when it exits fullscreen.

Updated proposal:

diff --git a/dwm.c b/dwm.c
index fd78193..65b5d00 100644
--- a/dwm.c
+++ b/dwm.c
@@ -1332,7 +1332,7 @@ replaceclient(Client *old, Client *new)
 {
        Client *c = NULL;
        Monitor *mon = old->mon;
-       int x, y, w, h;
+       int x = old->x, y = old->y, w = old->w, h = old->h;

        new->mon = mon;
        new->tags = old->tags;
@@ -1365,12 +1365,17 @@ replaceclient(Client *old, Client *new)
                w = old->oldw;
                h = old->oldh;
                new->isfloating = old->oldstate;
+       } else if (old->isterminal && new->isfullscreen && !old->isfullscreen) {
+               /* Allow windows starting in fullscreen to retain fullscreen */
+               new->oldx = old->x;
+               new->oldy = old->y;
+               new->oldw = old->w;
+               new->oldh = old->h;
+               new->oldbw = old->bw;
+               new->oldstate = old->isfloating;
        } else {
+               /* Inherit fullscreen property from the old client */
                setfullscreen(new, old->isfullscreen);
-               x = old->x;
-               y = old->y;
-               w = old->w;
-               h = old->h;
                new->oldx = old->oldx;
                new->oldy = old->oldy;
                new->oldw = old->oldw;

The behaviour is starting to feel quite good. Thanks for your testing efforts.

2084x commented 1 month ago

as far as I can tell, everything is working perfectly now.

thank you for all your hard work fixing this and helping me with the other swallow issue too. I greatly appreciate it!!

bakkeby commented 1 month ago

While trying to integrate this into my own build I came up with this alternative approach to:

void
replaceclient(Client *old, Client *new)
{
        Client *c = NULL;
        Monitor *mon = old->mon;
        int x, y, w, h, f, bw;

        if (old->isfullscreen) {
                x = old->oldx;
                y = old->oldy;
                w = old->oldw;
                h = old->oldh;
                f = old->oldstate;
                bw = old->oldbw;
        } else {
                x = old->x;
                y = old->y;
                w = old->w;
                h = old->h;
                f = old->isfloating;
                bw = old->bw;
        }

        new->mon = mon;
        new->tags = old->tags;

        new->next = old->next;
        new->snext = old->snext;

        if (old == mon->clients) {
                mon->clients = new;
        } else {
                for (c = mon->clients; c && c->next != old; c = c->next);
                c->next = new;
        }

        if (old == mon->stack) {
                mon->stack = new;
        } else {
                for (c = mon->stack; c && c->snext != old; c = c->snext);
                c->snext = new;
        }

        old->next = NULL;
        old->snext = NULL;

        if (!new->isfullscreen && old->isfullscreen && (swterminheritfs || !new->isterminal)) {
                /* Gain fullscreen */
                setfullscreen(new, 1);
        } else if (new->isfullscreen && !old->isfullscreen && !old->isterminal) {
                /* Lose fullscreen */
                setfullscreen(new, 0);
        }

        if (new->isfullscreen) {
                new->oldbw = bw;
                new->oldstate = f;
                new->oldx = x;
                new->oldy = y;
                new->oldw = w;
                new->oldh = h;
        } else {
                new->bw = bw;
                new->isfloating = f;

                if (ISVISIBLE(new) && new->isfloating) {
                        resize(new, x, y, w, h, 0);
                }
        }

        XMoveWindow(dpy, old->win, WIDTH(old) * -2, old->y);
}

It has a slightly higher line count, but I think that it is easier to reason about.

2084x commented 1 month ago

no issues with that version either. I'll use whichever you think is best.

bakkeby commented 1 month ago

I think I'll probably go with the latter. Just going to do some more sanity testing first, but logically the approach is a lot more sound and a lot less that could go wrong.