osa1 / tiny

A terminal IRC client
MIT License
997 stars 59 forks source link

ring terminal bell if desktop-notifications feature is disabled #309

Open ddiss opened 3 years ago

ddiss commented 3 years ago

I'm a big fan of tiny - thanks for developing it! I'd prefer to avoid pulling in the (many) notify-rust dependencies, so I build tiny with desktop-notifications disabled. With desktop-notifications disabled I'd still appreciate primitive a terminal bell notification. I tried to hack something up, but haven't had much luck with the following:

diff --git a/libtiny_tui/src/notifier.rs b/libtiny_tui/src/notifier.rs
index 0f2d49c..4119cf3 100644
--- a/libtiny_tui/src/notifier.rs
+++ b/libtiny_tui/src/notifier.rs
@@ -21,7 +21,10 @@ fn notify(summary: &str, body: &str) {
 }

 #[cfg(not(feature = "desktop-notifications"))]
-fn notify(_summary: &str, _body: &str) {}
+fn notify(_summary: &str, _body: &str) {
+    // Ring the terminal bell if desktop notification support is disabled
+    print!("\x07");
+}
trevarj commented 3 years ago

My guess is that you need access to the tty for this, which means that the bell control character should be output through termbox. If I add a small function to termbox::lib.rs, I can call it on private message, but not in the notifier due to design reasons. I think it will take a bit more hacking to get it to work in the Notifier code. @osa1 might know a less hacky way

pub fn bell(&mut self) {
    if let Some(tty) = &mut self.tty {
        tty.write(b"\x07").unwrap();
    }
}
osa1 commented 3 years ago

I'm a big fan of tiny - thanks for developing it!

Glad you liked it!

I have no idea how terminal bell works. It's possible that with the terminal modes we set bell doesn't work, I'm not sure.

In any case, I have a more general idea that will allow running any command, instead of just built-in notification functions: #261. @ddiss could you take a look at that issue and let me know if it would work for you?

Would be great if we could get rid of desktop-notifications feature entirely, while allowing more flexibility in how to show notifications.

trevarj commented 3 years ago

Just for clarification, the code snippet above does work and I can hear the bell with it. (alacritty doesn't seem to support the bell sound, so I had to use gnome-terminal)

osa1 commented 3 years ago

Ah, OK. In that case I guess with #261 you could use something like echo -ne '\007' as the notification command and it would work.

ddiss commented 3 years ago

Ah, OK. In that case I guess with #261 you could use something like echo -ne '\007' as the notification command and it would work.

This would be fine for me. Given that tiny is terminal based and the code to ring the bell minimal, I think it'd be nice to have built-in support, but I don't feel strongly about it :smiley: