bakkeby/patches

Swallowing the wrong window

paniash opened this issue · 13 comments

Hi!

I'm currently using this patch of swallow in my dwm build. (Patch dated: 2020/07/07)

When invoking my pdf viewer through the terminal, at times, the adjacent terminal window gets swallowed. Not sure if this is fixed in the latest patch on the website or not.

screen.mp4

I guess the main question is which terminal is it?

I guess the main question is which terminal is it?

As I mentioned earlier, the adjacent terminal next to the one which is supposed to be swallowed, gets swallowed. Unfortunately, github seems to recognize my screencast as a corrupted file.

Screencast: https://drive.google.com/file/d/1myiOWS9GHb5kVpCcpL5ENZ_nKbmlZsC-/view?usp=sharing

Anyway, I've now patched my build with your implementation of swallow. Can't tell if I won't get the same issue but so far so good.

How is this patch different from the one on the suckless webpage?

No I mean what terminal application is it?

You will have this effect if you use gnome-terminal for example because gnome-terminal is not actually a program, but a back end service.

So when I start gnome-terminal what actually starts is gnome-terminal-server, and every gnome-terminal window I open belongs to this single process.

If you start xclock on the terminal then when dwm sees this window it will work out what the process the xclock window belongs to. Then it will work out if any of the parent processes of the xclock process belongs to a terminal.

Because gnome-terminal windows all belong to the same process I believe that it would just use the very first terminal window it finds.

I think situations like this might be alleviated by checking the window that currently has focus first, but that's of course no guarantee.

If I start st or Alacritty for example, then these all start as individual processes so the above is not a problem.

No I mean what terminal application is it?

Ah, I misunderstood your question. I'm running zathura inside st.

If it is st then that is pretty weird as they should have different process IDs. Maybe it has something to do with how the st processes are spawned, not sure.

My implementation uses the same logic when it comes to traversing the process tree to find the parent process, so I'd assume you'll find similar issues.

The main difference has to do with how the "swallowing" happens. In my patch the new window and the terminal window are just two different clients and they just swap places.

In the patch on the suckless website the two windows are separate clients, but the window of the application client and the window of the terminal client swaps places. While the behaviour seems to be the same the size hints of the new window will directly affect that of the terminal, so when you exit the program the terminal may not size correctly. If you swallow another window again the size hints of the terminal can affect that window, etc.

Would be interesting to know why it works like this in your case.

Do you perhaps use the newterm patch to spawn new st windows?

Do you perhaps use the newterm patch to spawn new st windows?

Yes! I assume that's the reason why this issue pops up?

I would believe so, presuming that the original st will be the parent process of the new st. I guess you can confirm that with ps -ef.

I think we can fix that though by trying to make the new st detached from the parent.

This might be a start.

https://stackoverflow.com/questions/9037831/how-to-use-fork-to-daemonize-a-child-process-independent-of-its-parent

and

http://netzmafia.de/skripten/unix/linux-daemon-howto.html

@bakkeby Thanks! Perhaps a patch to the newterm diff to be issued to make it compatible with swallow?

Yes that was the idea.

What I came up with was this:

void
newterm(const Arg* a)
{
    int res;
    switch (fork()) {
    case -1:
        die("fork failed: %s\n", strerror(errno));
        break;
    case 0:
        switch (fork()) {
        case -1:
            die("fork failed: %s\n", strerror(errno));
            break;
        case 0:
            res = chdir(getcwd_by_pid(pid));
            execlp("st", "./st", NULL);
            break;
        default:
            exit(0);
        }
    default:
        wait(NULL);
    }
}

It uses a technique commonly used by daemons which is to fork a process that forks a second process, then the first process exits leaving the second process as an orphan process.

Seems to work fine as far as I can tell.

@bakkeby Thanks! Seems to be working fine on my end. Good so far. :-)

I put up a full patch here for reference:
https://github.com/bakkeby/patches/blob/master/st/st-newterm-orphan-20210712-4536f46.diff

and added a post on reddit in case someone has any critical feedback. I'll see about getting it uploaded to the patches page on the suckless website depending on that.