Skip to content
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

Scale offset according to DPI #1039

Merged
merged 1 commit into from
Mar 21, 2022
Merged

Conversation

livanh
Copy link
Contributor

@livanh livanh commented Feb 20, 2022

Currently the notification size changes according to the "scale" option, but the offset does not. I believe it makes more sense to scale them together

@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2022

Codecov Report

Merging #1039 (0a86f09) into master (329c877) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1039   +/-   ##
=======================================
  Coverage   61.18%   61.18%           
=======================================
  Files          45       45           
  Lines        7003     7003           
=======================================
  Hits         4285     4285           
  Misses       2718     2718           
Flag Coverage Δ
unittests 61.18% <0.00%> (ø)

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

Impacted Files Coverage Δ
src/draw.c 11.61% <0.00%> (ø)

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 329c877...0a86f09. Read the comment docs.

@fwsmit
Copy link
Member

fwsmit commented Mar 2, 2022

Thanks for the improvement. I agree that it's good to scale more things. Have you tested this on X11 or wayland?

@livanh
Copy link
Contributor Author

livanh commented Mar 7, 2022

I tested it on X11. I'll test it on wayland as well if needed

@fwsmit
Copy link
Member

fwsmit commented Mar 8, 2022

It does not have an effect on wayland. calc_window_pos is only used in X11. The current behavior in wayland is:

  • When I set the scale to 1, the notification is offset X distance from the edge of the display.
  • When I set the scale to 2, the notification is offset 2X distance from the edge of the display.
  • etc.

Is this already the desired behaviour? What's the behaviour on X11? Can you maybe explain a bit why?

@livanh
Copy link
Contributor Author

livanh commented Mar 11, 2022

Yes, the behaviour on wayland is already what I wanted to achieve. On X11, an offset of N was always interpreted as "N pixels", regardless of the scale. It would have to be "scale x N pixels" to be consistent with wayland.

As a concrete example, let's say I have a panel at the bottom of the screen that is 30 pixels high, and I want the notifications to appear just above it, without overlapping. I can set origin = bottom and offset = 0x30 and it works as intended. Now, if I configure everything to scale to 2X, the panel is 60 pixels high (correct), the notifications are twice as big (correct), but the offset is still 30 pixels (incorrect, because now the notifications cover the top half of the panel). With this change, the offset will also be scaled to 60 pixels.

I hope the explanation is clear. If you need it, I can improve it or provide screenshots.

@fwsmit
Copy link
Member

fwsmit commented Mar 21, 2022

Thanks for the explanation @livanh. I get the need for this and it seems like a logical change (espcially so, since it's already like this on wayland). I'll go ahead an merge it!

@fwsmit fwsmit merged commit 8aaa019 into dunst-project:master Mar 21, 2022
roosta added a commit to roosta/etc that referenced this pull request Oct 21, 2022
dunst-project/dunst#1039

Offset was patched to take into account scale, which messed up my
positioning. Divide with scale (2) to get the correct offset.
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