l4l / yofi

yofi is a minimalistic menu for wayland
https://crates.io/crates/yofi
MIT License
375 stars 21 forks source link

add timeout to panic notification #149

Closed BlockListed closed 1 year ago

BlockListed commented 1 year ago

This fixes #148

On panic we send a notification, but if not notification daemon is available we block on notify-send for a while, which leads to issues.

l4l commented 1 year ago

Instead of just blindly setting a timeout for child, I'd rather prefer figure out why is this happening. This behavior doesn't look rather obvious but anyway, it's better to give an option to explicitly disable the notify-send instead of silently hiding an issue.

BlockListed commented 1 year ago

Instead of just blindly setting a timeout for child, I'd rather prefer figure out why is this happening. This behavior doesn't look rather obvious but anyway, it's better to give an option to explicitly disable the notify-send instead of silently hiding an issue.

First it doesn't silently hide the issue (we log it) Second the lack of a timeout causes large issues, because the yofi ui is no longer updated after panicking, but it still grabs keyboard / mouse focus and therefor makes the wm fully unusable (eg. to force quit yofi).

I would argue that setting a timeout for notify-send is a correct solution, because the aforementioned keyboard / mouse focus locking during the panic, leads us to soft-locking the wm (only fixable by wm restart or switch to tty) based on the behaviour of another program.

l4l commented 1 year ago

I see two problems there:

The timeout for killing notify-send looks like a workaround, not a proper solution.

BlockListed commented 1 year ago

I see two problems there:

* for some reason sctk destructor doesn't free libinput stuff, though it probably should;

* notify-send shoudln't block, even without notification daemon.

The timeout for killing notify-send looks like a workaround, not a proper solution.

Well ´notify-send´ does seem to have a timeout for around 1 minute, but I think it is completely unacceptable to lock the wm for that long. Another option could be simply spawning the notify-send process and then exiting. But fixing the input locking is probably the most correct solution.

BlockListed commented 1 year ago

@l4l Quick update after more testing. Adding a breakpoint in main.rs line 264 (after the main_inner function) already results in a soft-lock of the WM. Maybe this should be pushed to a seperate PR. The RenderSurface drop function (and therefore the destroy function of the surfaces) definitely get called however.

Calling .set_keyboard_reactivity(KeyboardInteractivity::None) in the RenderSurface drop function also doesn't seem to fix anything (keyboard input to other programs is still not possible).

BlockListed commented 1 year ago

@l4l Quick update after more testing. Adding a breakpoint in main.rs line 264 (after the main_inner function) already results in a soft-lock of the WM. Maybe this should be pushed to a seperate PR. The RenderSurface drop function (and therefore the destroy function of the surfaces) definitely get called however.

Calling .set_keyboard_reactivity(KeyboardInteractivity::None) in the RenderSurface drop function also doesn't seem to fix anything (keyboard input to other programs is still not possible).

I don't really have the desire to fix this bug right now, so I put the destructor issues in #150.

I believe, that for now we should merge the shorter timeout, especially since notify-send doesn't seem to have any native timeout functionality.

l4l commented 1 year ago

Removing notification daemon doesn't reproduce, but I finally make it work while stopping the real daemon. I think I'll rewrite the notify-send or just use a library eventually but for now I'd rather prefer a simpler version with timeout binary: #151. Could you check it works for you?

BlockListed commented 1 year ago

Removing notification daemon doesn't reproduce, but I finally make it work while stopping the real daemon. I think I'll rewrite the notify-send or just use a library eventually but for now I'd rather prefer a simpler version with timeout binary: #151. Could you check it works for you?

Yes, just tested it, works fine.