Liupold / pidswallow

A swallower script using process hierarchy.
Apache License 2.0
43 stars 0 forks source link

Dev 2.0 #29

Closed Liupold closed 4 years ago

Liupold commented 4 years ago

Looking good for a 2.0 release.

To do

Feel free to add to this To do list. After it is done we will merge it. And release a 2.0 tar.

Liupold commented 4 years ago

@SeerLite This branch will be merge withing 2 Days. Let me know if something is broken / not working.

SeerLite commented 4 years ago

Hey, sorry for the wait! Most DEs are working great on my end. GNOME is giving me some issues with resizing sometimes, but it's hard to reproduce. I think I can look into that in a few days.

I've found some specific issues with Alacritty/zsh though. You see, if .zshrc is sourced before the terminal creates the window, then xdo id will grab the WID of the previously focused window (which will make pidswallow swallow the wrong window). I can easily reproduce this by moving the xdo id lines to the top of my .zshrc. I'm guessing people with a mostly empty .zshrc will have this problem frequently.

I think this has to be fixed before merging. I don't know how though. xdo id -m seemed like a good idea at first, but if another terminal is focused at when opening the new one it'll grab that one instead. We also can't just sleep inside .zshrc since 1) it's ugly 2) the terminal could take even longer than the sleep to appear.

Liupold commented 4 years ago

I think terminal fallback is essential. If someone's have a problem with this they can just remove that line from zshrc and it should work for those who have a empty zshrc / bashrc and use a Daemon based term the problem is real. I am currently out of ideas, ya it should be fixed before merge.

SeerLite commented 4 years ago

My .zshrc:

# pidswallow
if [ -n "$DISPLAY" ] && which xdo >/dev/null 2>&1; then
    pidswallow_tmp_terminal_wid="$(xdo id -m)"
    if [ ! -f "/tmp/term-pid-$pidswallow_tmp_terminal_wid" ]; then
        echo "$$" > /tmp/term-pid-"$pidswallow_tmp_terminal_wid"
        echo "$pidswallow_tmp_terminal_wid" > /tmp/term-wid-"$$"
        trap "( rm -f /tmp/term-wid-"$$" /tmp/term-pid-"$pidswallow_tmp_terminal_wid" )" EXIT HUP
    fi
    unset pidswallow_tmp_terminal_wid
fi
  1. xdo id -> xdo id -m: Wait for any window to focus (instead of ignoring)
  2. Check if the focused window is already stored in /tmp. If it is, fall back to terminal

This doesn't solve all the problems but it's a start.

SeerLite commented 4 years ago

Update:

if [ -n "$DISPLAY" ] && which xdo >/dev/null 2>&1; then
    pidswallow_tmp_term_attempt=0
    while [ "$pidswallow_tmp_term_attempt" -lt 5 ]; do
        pidswallow_tmp_terminal_wid="$(xdo id -m)"
        if [ ! -f "/tmp/term-pid-$pidswallow_tmp_terminal_wid" ] && [ "$(xdo id -N Alacritty $pidswallow_tmp_terminal_wid)" ]; then
            echo "$$" > /tmp/term-pid-"$pidswallow_tmp_terminal_wid"
            echo "$pidswallow_tmp_terminal_wid" > /tmp/term-wid-"$$"
            trap "( rm -f /tmp/term-wid-"$$" /tmp/term-pid-"$pidswallow_tmp_terminal_wid" )" EXIT HUP
            break
        else
            pidswallow_tmp_term_attempt=$(( pidswallow_tmp_term_attempt + 1 ))
            sleep 0.1
        fi
    done
    unset pidswallow_tmp_term_attempt
    unset pidswallow_tmp_terminal_wid
fi
  1. We try 5 times (0.1s interval) before giving up.
  2. && [ "$(xdo id -N Alacritty $pidswallow_tmp_terminal_wid)" ] checks if the WID we got is a terminal so we don't grab a previous window that isn't a terminal (which isn't saved as /tmp/term-). Not sure how to handle each terminal class though: either we write something like "put xdo id -N <terminal> in your zshrc but replace <terminal> with your terminal's class" or we use an environment variable before it "PIDSWALLOW_TERMINAL_CLASS=<terminal> and replace with your terminal's class". Whatever is easier for the end user.
Liupold commented 4 years ago

This is getting super complicated real fast. Let's think of something simpler.

SeerLite commented 4 years ago

I know haha. Give me some time though, I have some ideas

Liupold commented 4 years ago

I think the problem comes from putting the line before the prompt line. (I might be wrong).

Liupold commented 4 years ago

I know haha. Give me some time though, I have some ideas

No rush.

SeerLite commented 4 years ago

I think the problem comes from putting the line before the prompt line. (I might be wrong).

What do you mean?

Liupold commented 4 years ago

You set a custom prompt right? https://github.com/Liupold/dotfiles/blob/master/zsh/.config/zsh/.zshrc#L5-L6

Liupold commented 4 years ago

I think we should merge this and later fix that. :( Main branch is now lagging a lot.

SeerLite commented 4 years ago

You set a custom prompt right? https://github.com/Liupold/dotfiles/blob/master/zsh/.config/zsh/.zshrc#L5-L6

Yeah, but that's not directly the reason for this not working. Setting up the prompt is just the slowest part of .zshrc.

The problem is xdo id being called before the terminal window spawns: At least on alacritty, the shell and the terminal are started at the same time. This can cause shell startup files like .zshrc to complete before the window has even appeared to xorg. We have to account for people who don't even have anything in their .zshrc. They're gonna get an instant shell startup, completing before the terminal window appears.

I know this solution is quite complicated, but it's still a solution. Besides, it's not as slow as it looks. I'm cleaning it up a bit and pushing to a new branch so you can try it.

Liupold commented 4 years ago

We can add this directly into pidswallow (in a function) and call that from zshrc. (I don't like clutter in my zshrc)

SeerLite commented 4 years ago

I think we should merge this and later fix that. :( Main branch is now lagging a lot.

I think fixing this one is important first. Try an empty .zshrc with the original xdo id approach

SeerLite commented 4 years ago

We can add this directly into pidswallow (in a function) and call that from zshrc. (I don't like clutter in my zshrc)

That's actually what I was doing hehe (well, something similar)

Liupold commented 4 years ago

https://file.io/ayI5ISIzQyZ8

SeerLite commented 4 years ago

That's not the issue tho Give me a sec I'll try to demonstrate

SeerLite commented 4 years ago

https://0x0.st/iYgd.mp4

Liupold commented 4 years ago

Wait #41 will have problem with daemon based term? same PID?

SeerLite commented 4 years ago

Only if xdo id fails 5 times (by giving already tracked WIDs/wrong terminal class)

Liupold commented 4 years ago

http://0x0.st/iYEa.mkv I don't know what is happening.

SeerLite commented 4 years ago

Alacritty version? Edit: Mine is 0.5.0

Your .zshrc is being sourced after the alacritty window has appeared, but I don't know why?

Edit: Can you try with a .zshrc with only:

  xdo id

Opening alacritty on an empty desktop, does it give any output?

Liupold commented 4 years ago

Version 0.5.0 Checkout my Alacritty config. https://github.com/Liupold/dotfiles/blob/master/alacritty/.config/alacritty/alacritty.yml

SeerLite commented 4 years ago

We've got pretty similar configs.

Here's an issue on the alacritty repo which has to do with this btw. So yeah, it is normal for alacritty to launch the shell before/at the same time as creating the window. I wonder why you don't have this issue... Are you running on an SSD or a HDD?

Liupold commented 4 years ago

It's a alacrity specific issue(we can just recommended them to not use shell based approach for now), I am running HDD (5400 RPM) :( . Are you configuring shell in alacrity.yaml ?

SeerLite commented 4 years ago

running HDD (5400 RPM)

That has to be it! Alacritty would take longer to read .zshrc than to start the window. For instance, I'm running on an SSD (so I'm pretty sure that'd change the order in which this stuff happens)

Are you configuring shell in alacrity.yaml

Nope, chsh//etc/passwd.

It's a alacrity specific issue(we can just recommended them to not use shell based approach for now)

Actually, I'd be fine with that. I'm not sure if it's solely alacritty though, I'm gonna be testing other terminals

Liupold commented 4 years ago

It should be fixed for alacrity though. We can add this to know issue. The more stuff we add to this repo, more complected it becomes ( I don't want it to be messy). This seems like an edge case for which it will take a lot of time moreover it is not a deal breaker as the terminal approach works like a charm for alacrity. I know you want this to be perfect but it's not easy. It's okay to have know issues rather that have unknown bugs. That's the best we can do for now. Thanks a lot for your outstanding testing/reporting/fixing. report me what you find, as on my slow HDD. everything just woks. 🤣.

SeerLite commented 4 years ago

You're right, I'm getting too greedy here hehe. So far, only Alacritty has this issue. I guess that's just how things are.

Liupold commented 4 years ago

You can use that script yourself and add that snippet as a solution in know issue.

SeerLite commented 4 years ago

Nah, I think it's fine by falling back to terminal based. I'll add it to known issues.

Liupold commented 4 years ago

Hey, you can try something. (This might eliminate the usage of bspc hide). https://github.com/Liupold/dotfiles/blob/master/sxhkd/.config/sxhkd/sxhkdrc#L46-L48 Let me know what you think.

SeerLite commented 4 years ago

I mean, that's a nice workaround yeah, but I don't like it at all.

I never really close any windows with that shortcut (I'm talking about the usual windows you'd want to start from a terminal, swallowing). I prefer closing programs from within their interface: On zathura, I do :q. On mpv, it's either q or Ctrl-C. Hell, even on a terminal I usually do Ctrl-D.

Besides, that workaround makes bspwm lose track of all flags/states of the windows. The fact that the position is set correctly is just a hacky side effect.

Almost all tiling window managers have their own form of hiding (IIRC). I think PIDSWALLOW_SWALLOW_COMMAND has a lot of uses, especially on tiling WMs. I even think it should be listed as part of the installation steps.

Liupold commented 4 years ago

I totally agree, but I prefer this on my system (Not going to recommended this). It actually dynamically adjust the window size. (And the position). I don't have any problem with the global variable, they are now almost as fast as executing the command. Anyway are ready to merge it?

SeerLite commented 4 years ago

Oh, great!

Yeah, I think it's fine to merge! I'm not in the mood to investigate the GNOME issue I had above, it's really hard to reproduce anyway. I doubt anyone will get it, and if someone does I can look into it after it's been reported.

Liupold commented 4 years ago

Merging....