Add support for running commands on BEL #2298

robertgzr · 2019-04-10T21:22:12Z

Adds a new configuration option under visual_bell called
bell_command that reuses the command formatting of the keybindings.

This allows for things like playing a notification sound.
See #1528 (comment)

Closes #1528

robertgzr · 2019-04-10T21:25:43Z

Opening this before I document the changes and add them to config file in case there are any comments ;)

nixpulvis

One somewhat real comment, and then a couple of naming suggestions.

src/term/mod.rs

nixpulvis · 2019-04-11T19:49:03Z

src/config/mod.rs

+    #[inline]
+    pub fn bell_command(&self) -> Option<CommandWrapper> {
+        self.bell_command.clone()
+    }


We don's have getters for the other launcher (it's simply a pub field). I don't love the idea of having a lone pub field on the struct, but at the same time, I really would like to avoid clone. Thoughts?

Using .as_ref() would probably work here to return an Option<&CommandWrapper>. No clones necessary.

I'm not quite sure how to do this? surely you don't want me to litter all the structures with lifetime annotations just because of this?

@robertgzr I know this is outdated, but FWIW this would work:

struct A(Option<u32>); impl A { fn get(&self) -> Option<&u32> { self.0.as_ref() } } fn main() { println!("{:?}", A(Some(10)).get()); println!("{:?}", A(None).get()); }

chrisduerr

Thanks for the PR!

Even though it might only be of limited use, even to people desiring an audio bell, I think a bell command is a powerful solution which enables the user to take matters into their own hands without having to outright deny the feature.

This is probably the most likely approach to this issue so I'm generally not against moving forward with it, however I do think there should be a bit of refactoring to get this implemented properly.

Dropping the bell_command in the visual_bell section is a bit of a contradiction, since it's completely unrelated to any visual bell effect. So I think than renaming the visual bell to just bell would make sense.

Here's a draft of how a new version might look like:

# Bell
#
# The bell is triggered every time the BEL escape sequence is received.
bell:
  # Visual Bell Animation
  #
  # Animation effect for flashing the screen when the visual bell is rung.
  #
  # Values for `animation`:
  #   - Ease
  #   - EaseOut
  #   - EaseOutSine
  #   - EaseOutQuad
  #   - EaseOutCubic
  #   - EaseOutQuart
  #   - EaseOutQuint
  #   - EaseOutExpo
  #   - EaseOutCirc
  #   - Linear
  animation: Testing

  # Duration of the visual bell flash. A `duration` of `0` will disable the
  # visual bell animation.
  duration: 0

  # Visual bell animation color.
  color: '0xffffff'

  # Visual Bell Command
  #
  # This program is executed whenever the visual bell is rung. Setting
  # `command` to `None` will disable it.
  #
  # Example:
  #   command:
  #     program: /bin/notify-send
  #     args: ["Hello, World!"]
  command: None

Of course we can't just remove visual_bell, so we'd have to alias that to bell with a deprecation warning similar to what's already done in other parts of the config file (see deprecation comments in src/config/mod.rs).

HeyBanditoz · 2019-06-16T19:51:00Z

Do you still intend to work on this?

Adds a new configuration option under `visual_bell` called `bell_command` that reuses the command formatting of the keybindings. This allows for things like playing a notification sound. See alacritty#1528 (comment) Closes alacritty#1528

alacritty_terminal/src/term/mod.rs

robertgzr · 2019-08-05T01:10:07Z

re-ping @chrisduerr @nixpulvis in case this slipped through your queues ;)

chrisduerr

re-ping @chrisduerr @nixpulvis in case this slipped through your queues ;)

It did indeed. Apologies for the late reply.

This is missing the documentation in the default alacritty.yml to be updated. The default config file should always work properly and represent the code structure.

This PR also requires a rebase to allow it to build again.

chrisduerr · 2019-08-05T18:58:23Z

alacritty_terminal/src/config/mod.rs

+
+    // TODO: DEPRECATED
+    #[serde(default, deserialize_with = "failure_default")]
+    visual_bell: BellConfig,


Deprecated fields should be moved to the end of the config.

There also is no deprecation warning for this field which informs people about this change. This should be added.

Additionally the old config fields must still work properly during the deprecation period. We can't just remove fields.

chrisduerr · 2019-08-05T19:03:05Z

alacritty_terminal/src/term/mod.rs

            start_time: None,
        }
    }

    /// Ring the visual bell, and return its intensity.
    pub fn ring(&mut self) -> f64 {
+        self.run_command();


A newline after self.run_command() would be nice to make it clear it's logically separated from the rest of the bell logic.

chrisduerr · 2019-08-05T19:03:27Z

alacritty_terminal/src/term/mod.rs

+        Bell {
+            animation: bell_config.animation,
+            duration: bell_config.duration(),
+            command: bell_config.command.clone(),
            start_time: None,
        }
    }

    /// Ring the visual bell, and return its intensity.


This should be changed from visual bell to just bell, since it also executes the bell command.

chrisduerr · 2019-08-05T19:04:35Z

alacritty_terminal/src/term/mod.rs

+        }
+    }
+
+    pub fn run_command(&self) {


This function name does not correctly represent what is done inside the function. A generic command execution should not print bell errors.

I'd recommend inlining this into the run method.

I personally don't mind a Bell::run_command function like this, though I don't care much either way since Bell::ring is so short. I don't see why logging errors can't be part of the run_command logic.

If this were to be left as it's own function it could be hidden as an internal detail by removing the pub declaration.

chrisduerr · 2019-08-05T19:06:49Z

alacritty_terminal/src/term/mod.rs

-        let visual_bell_config = &config.visual_bell;
-        self.animation = visual_bell_config.animation;
-        self.duration = visual_bell_config.duration();
+        let bell_config = &config.bell;


Defining this variable here has no benefits, since config.bell and bell_config are exactly the same length.

kchibisov · 2020-01-10T23:48:28Z

While I like the idea of running commands on BEL, I don't really like the proposed integration into alacritty, however I've created a proposal on how an integration of this and similar features should be performed in alacritty in some kind of maintainable way. see #3182

chrisduerr · 2020-01-15T20:19:02Z

While I like the idea of running commands on BEL, I don't really like the proposed integration into alacritty, however I've created a proposal on how an integration of this and similar features should be performed in alacritty in some kind of maintainable way. see #3182

Due to this improved design for handling things like this and the amount of time this PR has been stale without any interest in it, I'm going to close this PR.

If there is interest in picking this back up, I'd recommend starting out with the new design suggested in #3182. If there are any uncertainties, please leave them in that issue.

Thanks for your work.

robertgzr force-pushed the bell-command branch from 5e7c499 to 1b2d517 Compare April 10, 2019 21:22

nixpulvis approved these changes Apr 11, 2019

View reviewed changes

chrisduerr requested changes Apr 11, 2019

View reviewed changes

chrisduerr added the C - awaiting progress label Apr 23, 2019

robertgzr added 2 commits June 20, 2019 15:31

Add support for running commands on BEL

2329362

Adds a new configuration option under `visual_bell` called `bell_command` that reuses the command formatting of the keybindings. This allows for things like playing a notification sound. See alacritty#1528 (comment) Closes alacritty#1528

bell command v2

ff8f9de

robertgzr force-pushed the bell-command branch from 1b2d517 to ff8f9de Compare June 29, 2019 18:33

robertgzr commented Jun 29, 2019

View reviewed changes

alacritty_terminal/src/term/mod.rs Show resolved Hide resolved

chrisduerr requested changes Aug 5, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

kchibisov mentioned this pull request Jan 10, 2020

Provide Trigger mechanism to call user scripts #3182

Closed

chrisduerr added C - waiting on author and removed C - awaiting progress labels Jan 14, 2020

chrisduerr closed this Jan 15, 2020

kchibisov mentioned this pull request Jun 14, 2020

Add option to run command on bell #3848

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for running commands on BEL #2298

Add support for running commands on BEL #2298

robertgzr commented Apr 10, 2019 •

edited

robertgzr commented Apr 10, 2019

nixpulvis left a comment

nixpulvis Apr 11, 2019

chrisduerr Apr 11, 2019

robertgzr Jun 29, 2019

nixpulvis Aug 12, 2019 •

edited

chrisduerr left a comment

HeyBanditoz commented Jun 16, 2019

robertgzr commented Aug 5, 2019

chrisduerr left a comment

chrisduerr Aug 5, 2019

chrisduerr Aug 5, 2019

chrisduerr Aug 5, 2019

chrisduerr Aug 5, 2019

nixpulvis Aug 12, 2019 •

edited

chrisduerr Aug 5, 2019

This comment has been minimized.

kchibisov commented Jan 10, 2020

chrisduerr commented Jan 15, 2020 •

edited

Add support for running commands on BEL #2298

Add support for running commands on BEL #2298

Conversation

robertgzr commented Apr 10, 2019 • edited

robertgzr commented Apr 10, 2019

nixpulvis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixpulvis Aug 12, 2019 • edited

Choose a reason for hiding this comment

chrisduerr left a comment

Choose a reason for hiding this comment

HeyBanditoz commented Jun 16, 2019

robertgzr commented Aug 5, 2019

chrisduerr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixpulvis Aug 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

kchibisov commented Jan 10, 2020

chrisduerr commented Jan 15, 2020 • edited

robertgzr commented Apr 10, 2019 •

edited

nixpulvis Aug 12, 2019 •

edited

nixpulvis Aug 12, 2019 •

edited

chrisduerr commented Jan 15, 2020 •

edited