search

cdown / clipmenu

Clipboard management using dmenu
MIT License
1.1k stars 90 forks source link

Die if X server is killed. #185

Closed navneetankur closed 2 years ago

cdown commented 2 years ago

Hi! This adds a dependency on xset, which isn't acceptable just for this.

Could you please explain your use case? At worst I think all we'd need is exponential backoff.

navneetankur commented 2 years ago

Could you please explain your use case?

I don't always have my window manager running. If I close my window manager, clipmenud fills the terminal with error lines. Making the terminal unusable. Also, it uses high amount of CPU due to the loop continuously running.

This adds a dependency on xset, which isn't acceptable just for this.

Okay, that makes sense.

Though I will add two things.

  1. Maybe the xset check isn't needed at all. If clipnotify doesn't give a non zero exit code when things are fine, then only the exit code of clipnotify can be checked. What does it mean when clipnotify returns a non zero exit code?
  2. If that's not enough, I think the output of clipnotify can be checked. It says something along the lines of 'can't connect to xserver'.
  3. xset check doesn't have to add a dependency. The code will work fine without xset, if we add a check before the xset check. Something like, if type xset If xset is not available, this feature(code path) can be completely ignored.
navneetankur commented 2 years ago

At worst I think all we'd need is exponential backoff.

I guess that would help my use case too. Maybe it will allow enough time to use the tty to kill the clipmenud.

cdown commented 2 years ago

I don't always have my window manager running. If I close my window manager, clipmenud fills the terminal with error lines. Making the terminal unusable.

clipmenud is dependent on X, but shouldn't be dependent on having any window manager running. Can you please show an example?

Maybe it will allow enough time to use the tty to kill the clipmenud.

Thanks. So, in general it's the user's responsibility to make sure clipmenud is launched and shut down at the appropriate time (or configure their user manager as such).

I'm ok with adding a backoff. Exponential backoff seems excessive, but adding sleep 10 with a comment is fine.

navneetankur commented 2 years ago

clipmenud is dependent on X, but shouldn't be dependent on having any window manager running.

Well, I close X too. Sorry for the confusion.

I'm ok with adding a backoff. Exponential backoff seems excessive, but adding sleep 10 with a comment is fine.

Thanks.

cdown commented 2 years ago

Oh sorry, I realise I made an error in my last round of review. What we probably actually want is:

if ! wait "$_CM_CLIPNOTIFY_PID"; then
    # X server dead?
    sleep 10
    continue
fi

Sorry to ask for another change, that's my bad. The continue is needed to stop spam from other places.

cdown commented 2 years ago

Thank you! Looks great.

navneetankur commented 2 years ago

Thanks