New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set urgent WM flag on bell on X11 systems #812

Merged
merged 2 commits into from Oct 21, 2017

Conversation

Projects
None yet
2 participants
@brennie
Copy link
Contributor

brennie commented Oct 4, 2017

This patch adds support for setting the urgent flag on systems using X11
when a terminal bell occurs.

When the window becomes focused, we will clear the urgent flag since not
all WMs will clear it automatically.

Fixes #181

@jwilm
Copy link
Owner

jwilm left a comment

Thanks for the PR! set_urgent implementation looks great. Have one concern about maybe calling it too frequently though.


if let Some(title) = terminal.get_next_title() {
self.window.set_title(&title);
}

if !bell_completed {

This comment has been minimized.

@jwilm

jwilm Oct 4, 2017

Owner

I think bell_completed is generally going to be false. It seems like this would raise the flag on pretty much every draw as a result.

Instead of using that indicator, would it be more correct if we just set a flag on Term when the bell is rang and clear the flag when setting urgency?

@brennie

This comment has been minimized.

Copy link
Contributor Author

brennie commented Oct 4, 2017

I've made the suggested changes locally, however, it does not appear all WMs (e.g. KWM) will clear the urgent flag once the window has focus, so I need to fix that up first.

@brennie brennie force-pushed the brennie:brennie/bell-urgent branch from 80a001f to 7a9011e Oct 4, 2017

@jwilm

This comment has been minimized.

Copy link
Owner

jwilm commented Oct 4, 2017

@brennie ah right. Should we only set the flag if the app isn't already focused? It appears to be set unconditionally right now.

@brennie

This comment has been minimized.

Copy link
Contributor Author

brennie commented Oct 4, 2017

I've verified with xprop that the XUrgencyHint flag is cleared when the window gets focus with this new patch.

@brennie

This comment has been minimized.

Copy link
Contributor Author

brennie commented Oct 4, 2017

Yeah, we probably should. I just realized if we set urgent while focused, it will still be urgent when we switch away. I'll work on that next :)

@brennie

This comment has been minimized.

Copy link
Contributor Author

brennie commented Oct 4, 2017

@jwilm So I'm pretty new to this code base. Where would you suggest I store state about whether or not the window is the focused window?
event::Processor::handle_event has access to the processor (and therefore the event::ActionContext), but not the window. Should this be stored on the Term? It seems not ideal store that information there. event::Processor::process_events does have access to the window, so would it make sense to pass in the Window to handle_event and change it there?

@brennie brennie force-pushed the brennie:brennie/bell-urgent branch 2 times, most recently from 48989a5 to 0757e9d Oct 4, 2017

@jwilm

This comment has been minimized.

Copy link
Owner

jwilm commented Oct 4, 2017

Setting a flag on Window sounds like the right thing.

@brennie brennie force-pushed the brennie:brennie/bell-urgent branch from 0757e9d to 1d54816 Oct 4, 2017

@brennie

This comment has been minimized.

Copy link
Contributor Author

brennie commented Oct 5, 2017

@jwilm I've made the requested changes.

@@ -282,6 +282,10 @@ impl Display {
self.window.set_title(&title);
}

if let Some(is_urgent) = terminal.next_is_urgent.take() {

This comment has been minimized.

@jwilm

jwilm Oct 5, 2017

Owner

Shouldn't this check the focus flag before raising the urgent flag?

Set urgent WM flag when bell is emitted on X11 systems
When the window becomes focused, we will clear the urgent flag since not
all WMs will clear it automatically.

Fixes #181

@brennie brennie force-pushed the brennie:brennie/bell-urgent branch from 1d54816 to 2075d1b Oct 6, 2017

@brennie

This comment has been minimized.

Copy link
Contributor Author

brennie commented Oct 6, 2017

I moved the check for whether or not the window is focused into the display implementation, so that each platform-specific impl doesn't have to worry about it.

@brennie

This comment has been minimized.

Copy link
Contributor Author

brennie commented Oct 15, 2017

@jwilm Ping on this? Anything else you need me to address?

@jwilm

jwilm approved these changes Oct 21, 2017

@jwilm

This comment has been minimized.

Copy link
Owner

jwilm commented Oct 21, 2017

Looks good; thanks!

@jwilm jwilm merged commit f6f2bcd into jwilm:master Oct 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

chrisduerr pushed a commit to chrisduerr/alacritty that referenced this pull request Apr 14, 2018

Set urgent WM flag on bell on X11 systems (jwilm#812)
Sets the urgent WM flag when bell is emitted on X11 systems.
Additionally, the flag is cleared on focus because not all WMs clear it
automatically.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment