search

cdown / clipmenu

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

Reload config on SIGHUP #222

Open N-R-K opened 2 months ago

N-R-K commented 2 months ago

Seems to work okay from some cursory tests. But since the code wasn't written with a reload-able config in mind, there might be bugs lurking.

Keeping this a draft now, until a more careful and thorough review.

cdown commented 2 months ago

Unless there's substiantial demand I'd be inclined to avoid config reloading, or at least do it without the minefield of signal involvement.

N-R-K commented 2 months ago

This was more of a "other deamons do it and seems like easy to add support for" rather than "i need this feature" thing. If someone really needed to have a new config, just restarting clipmenud works fine and retains previous clips. So yeah, this is probably not worthwhile.

Slightly off topic, but what exactly is max_clips_batch supposed to do? This is the only place where it's used, and it doesn't really make much sense to me:

    if ((int)cur_clips > cfg.max_clips_batch) {
        expect(cs_trim(&cs, CS_ITER_NEWEST_FIRST, (size_t)cfg.max_clips) == 0);
    }

Was it meant to be cur_clips > cfg.max_clips + cfg.max_clips_batch?

EDIT: see https://github.com/cdown/clipmenu/pull/225

N-R-K commented 2 months ago

just restarting clipmenud works fine

This made me realize, you could simply re-exec instead of re-reading config and get a fully clean state. Updated the patch. Let me know if you find this approach acceptable. Otherwise I can drop the PR, as it's not too important for me.