Skip to content

Clamp maximum notification width to screen width#973

Merged
fwsmit merged 5 commits into
dunst-project:masterfrom
milaq:screen_width_fix
Nov 29, 2021
Merged

Clamp maximum notification width to screen width#973
fwsmit merged 5 commits into
dunst-project:masterfrom
milaq:screen_width_fix

Conversation

@milaq
Copy link
Copy Markdown
Contributor

@milaq milaq commented Nov 12, 2021

This fixes the notification bleeding into other screens as discussed in #951.

This fixes the main issue described in ticket for X11.
This code will work for Wayland eventually once get_active_screen is implemented correctly in wl.c.
For now we increase the stub resolution to a value which does not get exceeded with "common" screen resolutions.

Thanks an best regards

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 12, 2021

Codecov Report

Merging #973 (5137b88) into master (45c7c12) will decrease coverage by 0.42%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #973      +/-   ##
==========================================
- Coverage   60.59%   60.17%   -0.43%     
==========================================
  Files          44       44              
  Lines        6629     6785     +156     
==========================================
+ Hits         4017     4083      +66     
- Misses       2612     2702      +90     
Flag Coverage Δ
unittests 60.17% <0.00%> (-0.43%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/draw.c 0.94% <0.00%> (-0.01%) ⬇️
src/wayland/wl.c 0.00% <0.00%> (ø)
src/dbus.c 52.17% <0.00%> (-11.38%) ⬇️
src/utils.c 88.55% <0.00%> (-2.26%) ⬇️
src/queues.c 91.44% <0.00%> (-0.19%) ⬇️
src/option_parser.c 80.10% <0.00%> (ø)
test/queues.c 98.86% <0.00%> (+0.09%) ⬆️
src/rules.c 76.85% <0.00%> (+0.21%) ⬆️

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 45c7c12...5137b88. Read the comment docs.

@fwsmit
Copy link
Copy Markdown
Member

fwsmit commented Nov 12, 2021

Thank you for implementing this. I've actually implemented the wayland display size detection. I'll push that onto your branch and reword you commit message.

Avoid bleeding the notification window into other screens.
This fixes a regression introduced with 208b967.
This stub resolution is only used when follow mode is used, since dunst
cannot easily detect the active screen then.
This works around having the notification cut off at 1080 pixels under
Wayland with screens larger than 1080p, of which there are plenty
in use nowadays.
4k UHD as a stub should suffice here for most cases.
Comment thread src/draw.c Outdated
Comment thread src/draw.c
@milaq
Copy link
Copy Markdown
Contributor Author

milaq commented Nov 12, 2021

Thank you for implementing this. I've actually implemented the wayland display size detection. I'll push that onto your branch and reword you commit message.

Nice! Thanks for the fast response.

@fwsmit
Copy link
Copy Markdown
Member

fwsmit commented Nov 23, 2021

I've tested it and it seems to work great. We may want to have some documentation about this for other people to use this feature.
Also, do you still want to implement that a special width value (for example 0 or -1) makes sure the notification fills the entire width?

@milaq
Copy link
Copy Markdown
Contributor Author

milaq commented Nov 26, 2021

Unfortunately, I have too limited time at the moment to work around the integer parsing troubles in the config file.

As of now with these commits, the notification width always gets clamped to current screen width.
I think this is the correct behavior as the notification bleeding into other screens is not desirable at any time and therefore considered a bug.
Because of that we should merge this PR as a "fix" and leave the config parsing open as an enhancement TODO to at least restore the correct behavior like before the patch series a few weeks ago. Furthermore, it does not get in the way of the regular behavior of Dunst.

We could write a sentence or two in the manpages. Something like:

The notification width will always clamp to the curreent screen width.
If you want the notifcation to stretch the entire screen dynamically, you may set the width to a high enough number, which none of your screens exceed (e.g. 10000).

What do you think?

@fwsmit
Copy link
Copy Markdown
Member

fwsmit commented Nov 26, 2021

Unfortunately, I have too limited time at the moment to work around the integer parsing troubles in the config file.

As of now with these commits, the notification width always gets clamped to current screen width.
I think this is the correct behavior as the notification bleeding into other screens is not desirable at any time and therefore considered a bug.
Because of that we should merge this PR as a "fix" and leave the config parsing open as an enhancement TODO to at least restore the correct behavior like before the patch series a few weeks ago. Furthermore, it does not get in the way of the regular behavior of Dunst.

No problem, it's completely fine to leave it as is for now.

We could write a sentence or two in the manpages. Something like:

The notification width will always clamp to the curreent screen width.
If you want the notifcation to stretch the entire screen dynamically, you may set the width to a high enough number, which none of your screens exceed (e.g. 10000).

What do you think?

Yes, seems good. Could you add that to the man page, then I'll merge this PR :)

@fwsmit
Copy link
Copy Markdown
Member

fwsmit commented Nov 29, 2021

I've added it to the man page, so I'll merge it after the checks are finished

@fwsmit fwsmit merged commit aa9ff0d into dunst-project:master Nov 29, 2021
@fwsmit
Copy link
Copy Markdown
Member

fwsmit commented Nov 29, 2021

Merged, thanks for the fix

@milaq
Copy link
Copy Markdown
Contributor Author

milaq commented Nov 30, 2021

Sorry, did not see your message in time.
Thanks for the manpage addition. Your wording sounds better either way ;)

Good to see this merged!

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