Skip to content

follow=keyboard: Fall back to follow=mouse instead of XDefaultScreen()#708

Merged
tsipinakis merged 1 commit into
dunst-project:masterfrom
cdown:mouse_fallback
Apr 27, 2020
Merged

follow=keyboard: Fall back to follow=mouse instead of XDefaultScreen()#708
tsipinakis merged 1 commit into
dunst-project:masterfrom
cdown:mouse_fallback

Conversation

@cdown

@cdown cdown commented Apr 27, 2020

Copy link
Copy Markdown
Contributor

In dwm and similar window managers, it's common to often have empty
tags, and navigate to them with the intent to create clients there. If I
navigate to one of those empty tags with a dunst notification visible
and follow=keyboard set, the notification warps over to the default
screen. If I then open a client, it then warps back, which is pretty
jarring.

This is mostly an artefact of the implementation of follow=keyboard --
if we fail to get a focused window, we use the default screen. However
this case isn't necessarily really a "failure" on window managers like
dwm where it's a common occurrence to end up with no clients on the
screen, whereas that would be significantly rarer on (say) GNOME or KDE.

A guess that's more likely to fit user expectations is falling back to
where the mouse pointer currently is, since this indicates the currently
focused monitor that the window manager would create a client on. This
avoids warping back to that monitor again when a client is created.

@cdown

cdown commented Apr 27, 2020

Copy link
Copy Markdown
Contributor Author

Video demonstration of the problem and solution, with dunst HEAD first, and dunst with this patch after:

https://www.youtube.com/watch?v=STXILe-udt0

In dwm and similar window managers, it's common to often have empty
tags, and navigate to them with the intent to create clients there. If I
navigate to one of those empty tags with a dunst notification visible
and follow=keyboard set, the notification warps over to the default
screen.  If I then open a client, it then warps back, which is pretty
jarring.

This is mostly an artefact of the implementation of follow=keyboard --
if we fail to get a focused window, we use the default screen. However
this case isn't necessarily really a "failure" on window managers like
dwm where it's a common occurrence to end up with no clients on the
screen, whereas that would be significantly rarer on (say) GNOME or KDE.

A guess that's more likely to fit user expectations is falling back to
where the mouse pointer currently is, since this indicates the currently
focused monitor that the window manager would create a client on. This
avoids warping back to that monitor again when a client is created.
@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #708 into master will increase coverage by 0.10%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
+ Coverage   67.13%   67.24%   +0.10%     
==========================================
  Files          29       29              
  Lines        5006     4995      -11     
==========================================
- Hits         3361     3359       -2     
+ Misses       1645     1636       -9     
Flag Coverage Δ
#unittests 67.24% <0.00%> (+0.10%) ⬆️
Impacted Files Coverage Δ
src/x11/screen.c 0.00% <0.00%> (ø)
src/rules.c 50.00% <0.00%> (-1.62%) ⬇️
src/option_parser.c 86.62% <0.00%> (-0.04%) ⬇️
src/x11/x.c 2.35% <0.00%> (+0.02%) ⬆️
src/dunst.c 10.47% <0.00%> (+0.09%) ⬆️
src/menu.c 15.48% <0.00%> (+0.09%) ⬆️
src/notification.c 61.93% <0.00%> (+0.37%) ⬆️
src/log.c 79.06% <0.00%> (+1.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 523d5e1...e8fc45d. Read the comment docs.

@tsipinakis tsipinakis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thanks for taking the time to fix it!

@tsipinakis tsipinakis merged commit 6b60c52 into dunst-project:master Apr 27, 2020
@cdown

cdown commented Apr 27, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for the quick review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants