Biont / sway-launcher-desktop

TUI Application launcher with Desktop Entry support. Made for SwayWM, but runs anywhere
GNU General Public License v3.0
613 stars 29 forks source link

Sometimes it fails to execute the selected command due to issue finding the process ID to kil #8

Closed asahaf closed 4 years ago

asahaf commented 4 years ago

Sometimes it fails to execute the selected command due to issue finding the process ID to kill the line: https://github.com/Biont/sway-launcher-desktop/blob/f81648fe35afea6ad1633c2f761380335c004f5d/sway-launcher-desktop.sh#L183

It's random, and I don't know how to reproduce it.

thanks

Biont commented 4 years ago

Thanks for the report! I think I know what you are talking about and I am glad I know where to start looking now.

asahaf commented 4 years ago

great, thank you!

asahaf commented 4 years ago

That's the error I get sometimes:

./sway-launcher-desktop.sh: line 189: kill: (5127) - No such process

Here is says line 189, but I think it's related to line 183:

https://github.com/Biont/sway-launcher-desktop/blob/f81648fe35afea6ad1633c2f761380335c004f5d/sway-launcher-desktop.sh#L183

Biont commented 4 years ago

I wonder why that can happen. The whole problem is that pipe is still open after fzf finishes so the script cannot proceed. The solution is taken straight from here: https://github.com/junegunn/fzf/issues/880

Biont commented 4 years ago

Based on the assumption that the process is simply already destroyed for reasons I currently do not understand, we can probably safely move on here. I made a change so that it always returns successfully even if it cannot find a process. Could you please have a look @asahaf ?

I hope this fixes the problem without creating 2 new ones

asahaf commented 4 years ago

I'll give it a try, thanks

asahaf commented 4 years ago

@Biont The workaround solved the issue.

I think we can move the kill command, for the tail process, inside the same clean up trap that deletes the temp files, placing kill command before deleting the temp files.

Biont commented 4 years ago

I'm going to close here since #10 essentially handles what you suggested as well