enhanced bell configuration #483

Manually merged
dnkl merged 1 commits from r_c_f/foot:bell-enhancement into master 6 months ago
r_c_f commented 6 months ago

Add a separate section for bell configuration, with a bell-specific
command option and a setting to allow that command to run without regard
to keyboard focus (for those of us who enjoy being beeped at at all
times, for example). The actions are also no longer mutually exclusive;
this is primarily anticipating urgency support which cannot be
replicated outside the process (in server mode anyway) and would thus be
complementary to any notification or arbitrary command.

Add a separate section for bell configuration, with a bell-specific command option and a setting to allow that command to run without regard to keyboard focus (for those of us who enjoy being beeped at at all times, for example). The actions are also no longer mutually exclusive; this is primarily anticipating urgency support which cannot be replicated outside the process (in server mode anyway) and would thus be complementary to any notification or arbitrary command.
r_c_f added 1 commit 6 months ago
5d71ccc174 enhanced bell configuration
dnkl requested changes 6 months ago
dnkl left a comment

Wow! I'm really liking this! I've been thinking about adding support for a generic notify "command" now for a while. This PR does that, and more. Nice!

I have a couple of minor comments below, but code in general looks great.

We'd also need changelog entries (at least one for the new section/options, and one for the old option being deprecated). And feel free to add yourself to the "contributors" list while you're at it ;)

config.c Outdated
return false;
}
}
dnkl commented 6 months ago
Poster
Owner

This needs to be kept for a full 1.x release, with deprecation warnings. See e.g. 386796cec6/config.c (L1075-L1088)

This needs to be kept for a full 1.x release, with deprecation warnings. See e.g. https://codeberg.org/dnkl/foot/src/commit/386796cec69089b0acb70c5f10f6b049a30aad52/config.c#L1075-L1088
dnkl marked this conversation as resolved
# SECTION: bell
*urgent*
When set to _true_, the margins will be painted in red
dnkl commented 6 months ago
Poster
Owner

For consistency, use yes|no rather than true|false (and that obviously applies to all the occurrences below as well).

For consistency, use `yes|no` rather than `true|false` (and that obviously applies to all the occurrences below as well).
dnkl marked this conversation as resolved
terminal.c Outdated
term->render.urgency = true;
term_damage_margins(term);
break;
if (term->conf->bell.command.argv && (!term->kbd_focus || term->conf->bell.command_focused)) {
dnkl commented 6 months ago
Poster
Owner

nit: for consistency, use an explicit comparison with NULL.

nit: for consistency, use an explicit comparison with `NULL`.
dnkl marked this conversation as resolved
dnkl added the
enhancement
label 6 months ago
dnkl added this to the 1.8.0 milestone 6 months ago
r_c_f added 3 commits 6 months ago
dnkl requested changes 6 months ago
dnkl left a comment

Looking mostly good now. Just a couple of minor issues, and I'll have to actually test this branch, but then I think we're good to go.

config.c Outdated
else if (strcmp(value, "none") == 0)
conf->bell_action = BELL_ACTION_NONE;
conf->bell.notify = true;
/* we do nothing by default, so none may be ignored */
dnkl commented 6 months ago
Poster
Owner

While uncommon, it is possible to have (in foot.ini) e.g.

bell=notify
bell=set-urgency
bell=none

(it's not entirely pointless - perhaps someone is testing the different options).

The above would result in both bell.urgent and bell.notify being set to true. To be completely safe, reset the bell options before setting the new value (but only when the new value is valid...)

While uncommon, it _is_ possible to have (in foot.ini) e.g. ```conf bell=notify bell=set-urgency bell=none ``` (it's not entirely pointless - perhaps someone is testing the different options). The above would result in both `bell.urgent` and `bell.notify` being set to `true`. To be completely safe, reset the bell options before setting the new value (but _only_ when the new value is valid...)
dnkl marked this conversation as resolved
foot.ini Outdated
# urgent=false
# notify=false
# command=
# command_focused=false
dnkl commented 6 months ago
Poster
Owner

no instead of false here.

`no` instead of `false` here.
dnkl marked this conversation as resolved
r_c_f added 1 commit 6 months ago
dnkl approved these changes 6 months ago
dnkl left a comment

LGTM!

r_c_f force-pushed bell-enhancement from 5df981d23f to 9b9f08a492 6 months ago
dnkl merged commit 63572e4223 into master manually 6 months ago
Owner

Thanks!

Thanks!

Reviewers

dnkl approved these changes 6 months ago
The pull request has been manually merged as 63572e4223.
Sign in to join this conversation.
Loading…
There is no content yet.