Mixed-DPI groundwork #2053

Oblomov · 2017-10-07T21:41:53Z

Some more groundwork to improve support for mixed-DPI setups.

It pre-computes the per-screen DPI, while still allowing overrides, it allows choosing a rounding method for the used DPI, and font loading now allows an associated screen, to be used for height computation.

(Some of these make more sense view in terms of the subsequent fixes spread around the lib part, the work is pushed to my mixed-dpi branch, but still needs a more thorough review.)

codecov · 2017-10-07T21:53:39Z

Codecov Report

Merging #2053 into master will increase coverage by 0.12%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #2053      +/-   ##
==========================================
+ Coverage   79.55%   79.67%   +0.12%     
==========================================
  Files         404      405       +1     
  Lines       27652    27713      +61     
  Branches      991      991              
==========================================
+ Hits        21998    22081      +83     
+ Misses       5218     5198      -20     
+ Partials      436      434       -2

Flag	Coverage Δ
#c_code	`65.67% <ø> (+0.21%)`	⬆️
#functionaltests	`66.11% <87.5%> (+0.19%)`	⬆️
#samples	`67.36% <34.54%> (-0.06%)`	⬇️
#unittests	`65.65% <49.01%> (-0.11%)`	⬇️

Impacted Files	Coverage Δ
objects/screen.c	`53.15% <ø> (+0.87%)`	⬆️
lib/gears/dpi.lua	`100% <100%> (ø)`
lib/awful/hotkeys_popup/widget.lua	`91.61% <100%> (+0.07%)`	⬆️
lib/awful/wibar.lua	`80.4% <100%> (ø)`	⬆️
lib/gears/init.lua	`100% <100%> (ø)`	⬆️
lib/awful/mouse/snap.lua	`81.16% <100%> (ø)`	⬆️
lib/awful/screen.lua	`79.23% <100%> (+4.23%)`	⬆️
lib/wibox/widget/textbox.lua	`94.24% <100%> (+1.33%)`	⬆️
lib/beautiful/xresources.lua	`69.64% <20%> (-9.39%)`	⬇️
lib/naughty/core.lua	`68.54% <75%> (-0.28%)`	⬇️
... and 11 more

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 5ac6db5...b823218. Read the comment docs.

psychon · 2017-10-08T07:50:12Z

tests/test-screen-changes.lua

-        if #fake_screen == 0 then
-            return true
-        end
+        return #fake_screen == 0


The test runner (tests/_runner.lua) does the following after a step ran:

If it returned true, it continues with the next step

If it returns false, the whole test is considered to have failed

If it returns nil, this step is retried after 0.1 seconds, but only up to 5 times. Then the test is considered to have failed.

So, before this commit, the garbage collector had up to 5 calls to collectgarbage("collect") time to get rid of the old screen. Now the screen must be removed on the first collection cycle.

I guess this change is the reason why the Travis build failed. Also, this cannot stall, since the test runner only tries up to five times and then gives up.

Please remove this commit.

The Travis build was failing even before I introduced this commit, due to (I think) references to screen being held in the new font cache. But thanks for the explanation, I'll remove the commit.

psychon · 2017-10-08T07:56:37Z

lib/beautiful/xresources.lua

-    if dpi_per_screen[s] then
-        return dpi_per_screen[s]
+    if s then
+        return dpi_per_screen[s] or s.dpi


If I query the DPI of some screen (which has no outputs), then the result will always be 96, since this is what the get_dpi function above returns. Shouldn't this rather use some of the calculations below instead if no outputs are available?
So, my proposal:

if dpi_per_screen[s] then return dpi_per_screen[s] end if #s.outputs > 0 then return s.dpi end

psychon · 2017-10-08T08:04:16Z

lib/beautiful/xresources.lua

-    if dpi_per_screen[s] then
-        return dpi_per_screen[s]
+    if s then
+        return dpi_per_screen[s] or s.dpi


And the next problem: People who have wrong dimensions in their EDID data and overrode this via Xft.dpi will now have Xft.dpi being ignored. Since Xft.dpi is some explicit configuration, I think it should have higher priority than auto-configuration.

Meaning that I would propose the following order of settings:

Forced settings via Lua: dpi_per_screen[s]

Forced settings via xrdb: Xft.dpi

Automatically computed from outputs: s.outputs

Automatically computed from the size of the root window (the code below)

Good old 96

psychon · 2017-10-08T08:09:07Z

lib/beautiful/xresources.lua

+-- 96 DPI, according to the function used
+-- @tparam function func a function to be applied to the DPI scaling factor
+function xresources.set_dpi_rounding(func)
+    dpi_scale_rounding = func


Sorry, but this function is useless. It gets as argument the value dpi/96. If you want to e.g. ceil this, you could just do set_dpi(s, 96*math.ceil(get_dpi(s)/96)) in your rc.lua (or just compute the wanted DPI value by hand). So, this does not add any new possibilities.

Put differently: Isn't it enough to be able to override DPI per-screen?

If you want a rounding function, I would more expect the user to override the round call in apply_dpi. However, that function must return integers, so this might not be all that useful either.

psychon · 2017-10-08T08:10:07Z

lib/beautiful/xresources.lua

@@ -145,6 +145,7 @@ end
 -- @tparam[opt] integer|screen s The screen.
 -- @treturn integer Resulting size (rounded to integer).
 function xresources.apply_dpi(size, s)
+    if not size then return size end


Why is this needed? Which code would pass a nil (or false) here?

in my mixed-dpi branch I remove a number of uses of the global apply_dpi, shifting the computation of the dpi scaling at use time rather than define time. There are a few cases in which the value is optional with a number of fallbacks that can all be nil (local somevar = args.stuff or default.stuff or blah.stuff). Rather than leaving that and then adding a new somevar = somevar and dpi(somevar, s) or nil line I decided to just wrap everything in a dpi(..., s), which then needs to pass through nil values.

Okay, but I guess then this behaviour should be added to the documentation of this function.

psychon · 2017-10-08T08:12:11Z

lib/beautiful/xresources.lua

@@ -64,6 +64,9 @@ function xresources.get_current_theme()
 end


+--- Store the cached or user-set per-screen DPI
+-- Values are tables with key dpi (the dpi) and set_from (user, auto, xrdb)
+-- The same is done for xresources.dpi


What this commit actually does is to add a cache to DPI values. Why? Isn't the existing state of recomputing this "all the time" good enough? Do you have some example where adding a cache here speeds something up significantly?

psychon · 2017-10-08T08:16:08Z

lib/beautiful/init.lua

-        return fonts[name]
+    local fonts_key = {name, screen}
+    if fonts[fonts_key] then
+        return fonts[fonts_key]


Whoops. I totally forgot about this function ignoring DPI.

Anyway, the above cache check does not work. Lua tables are only equal to themselves, i.e. the index operator does not do a deep comparison:

$ lua -e 't={} t[{}] = 42 print(t[{}])' nil

Thus, the above doesn't cache check does not work.

Also, I wonder if it makes sense to have a version of this function that gets DPI as argument and another that gets a screen object, gets the screen's DPI and then just calls the other function (same for get_font_height). Thoughts?

(At least the cache only needs the DPI, not the screen itself)

Otherwise we don't get the events that mark a runtime DPI update.

When picking the font height, offer the possibility to compute it based on the DPI of a specific screen, to improve support for mixed-DPI environments.

In the process of deprecation of the old xresources.*dpi API, the global font DPI management is moved to beautiful itself. (awful.screen was also a good candidate, but this led to some pretty messy nearly irresolvable interdependencies between awful and beautiful.)

Allow consistent font scaling in mixed-DPI environments.

The per-screen DPI does not make use of Xft.dpi now, and if RANDR information is not available we fall back to the core DPI.

Scale a length to account for the DPI of the given screen.

Not all user appreciated fractional scaling, make it easier for them to snap to integers.

Values should be defined unscaled, and scaled based on the DPI of the screen where the popup appears.

Oblomov · 2017-10-28T18:59:17Z

Ping @psychon @blueyed @Elv13 review?

psychon

Dunno if I really like this or not, but here are some comments. What do others think? @Elv13 @blueyed @actionless

psychon · 2017-10-30T15:33:12Z

lib/gears/dpi.lua

+        end
+    end
+    return dpi
+end


Is this a good idea to have? If we want to have per screen DPIs, there should be no "global" DPI that could accidentally be used in code instead.

psychon · 2017-10-30T15:35:01Z

lib/beautiful/init.lua

@@ -98,6 +100,41 @@ local active_font
 --- The current theme path (if any)
 -- @tfield string beautiful.theme_path

+--- Font DPI API
+-- @section font_dpi


This might be a good idea why for fonts the number of pixels per inch of your monitor is different than for non-fonts.

Since I fail to parse my own sentence: The docs here could explain the concept for "fonts get a different DPI" a bit here.

psychon · 2017-10-30T15:35:35Z

lib/beautiful/init.lua

+function beautiful.screen_font_dpi(scr)
+    scr = scr and screen[scr] or screen.primary
+    return beautiful.get_font_dpi()*scr.dpi/screen.primary.dpi
+end


@Elv13 What do you think of this?

psychon · 2017-10-30T15:37:14Z

lib/beautiful/init.lua

@@ -255,7 +256,7 @@ end
 -- @tparam[opt] screen scr Screen to get the DPI information from.
 --   If unspecified, the global font DPI setting will be used.
 function beautiful.get_font_height(name, scr)
-    return load_font(name, beautiful.xresources.get_dpi(scr)).height
+    return load_font(name, beautiful.screen_font_dpi(scr)).height


Hm, this opens quite a can of worms.
Primary screen changed? Font height changed on all screens!
Primary screen changed DPI value? Font height changed on all screens!
Some random screen changed DPI value? Font height changed for that screen.

I bet that nothing will handle these correctly.

psychon · 2017-10-30T15:41:35Z

lib/gears/dpi.lua

+-- So we offer the possibility for the user to choose a global policy,
+-- and all DPI usage will take this into consideration.
+-- Note that rounding is only applied to autocomputed DPI values, never
+-- when the user sets the DPI themselves.


I can't say I'm convinced that this is a good idea, but at least the default is identity.
@Elv13 What do you think?

psychon · 2017-10-30T15:43:41Z

tests/test-dpi.lua

+
+runner.run_steps(steps)
+
+-- vim: filetype=lua:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80


Yay for tests! :-)

I think this should be done as an unit test instead of integration (i.e. as spec/gears/dpi_spec.lua). gears.dpi does not use much APIs, so that can easily be mocked (it just needs root, I think).

I'll look into this.

Feel free to ping me if needed.

psychon · 2017-10-30T15:46:28Z

lib/wibox/widget/textbox.lua

    end
+    local dpi = beautiful.screen_font_dpi(s)


This changes the semantics of which DPI value is used when this function is called with s=nil. Previously, it used some concept of "core DPI" while now it uses the DPI of the primary screen.
@Elv13 Does that count as a regression?

psychon · 2017-10-30T15:47:14Z

lib/beautiful/xresources.lua

+    if s then
+        return s:dpi_scale(size)
+    else
+        return round(size * require("beautiful").screen_font_dpi()/96)


Why does this use font DPI? If you just use the font DPI everywhere, then s.dpi is not needed.

There's an issue about “what has this call being used for in legacy code” and “how do we transition from there to the API we want now”.

Among other things, this means that the only case where you access the DPI without a screen specification is if you want to know the user-specified font scaling, since there is (or should not be) any concept of “global DPI” (otherwise).

psychon · 2017-10-30T15:50:47Z

lib/wibox/widget/textbox.lua

-    if s then
-        dpi = screen[s].dpi
-    else
+    if not s then


What is with :fit and :draw? This commits make the "non-widget" functions use a different DPI value than the actual drawing does.

psychon

Blocked by #2091, we should first decide about the route before we go hiking.

Oblomov force-pushed the mixed-dpi-groundwork branch from 4323d91 to 3c62c42 Compare October 7, 2017 21:53

Oblomov force-pushed the mixed-dpi-groundwork branch 7 times, most recently from 1e7a7a8 to b6210ca Compare October 8, 2017 06:25

psychon reviewed Oct 8, 2017

View reviewed changes

Oblomov force-pushed the mixed-dpi-groundwork branch 7 times, most recently from ced6bdf to 1994a6f Compare October 17, 2017 21:31

Oblomov force-pushed the mixed-dpi-groundwork branch 2 times, most recently from bbfbab8 to 6c8ce09 Compare October 22, 2017 15:49

Oblomov added 2 commits October 22, 2017 23:43

screen: ask for RRScreenChangeNotify events too

52b41ec

Otherwise we don't get the events that mark a runtime DPI update.

beautiful: screen-based get_font_height

d6a5114

When picking the font height, offer the possibility to compute it based on the DPI of a specific screen, to improve support for mixed-DPI environments.

Oblomov force-pushed the mixed-dpi-groundwork branch 2 times, most recently from 4b41b2b to 1f9e1ff Compare October 22, 2017 22:17

blueyed added the enhancement label Oct 22, 2017

Oblomov added 14 commits October 23, 2017 00:48

gears.dpi: function to retrieve the global DPI

678471a

beautiful: per-screen font DPI

9b4a8b6

Allow consistent font scaling in mixed-DPI environments.

beautiful: use new font_dpi API

39e41a9

Clean up per-screen DPI

59d9ef9

The per-screen DPI does not make use of Xft.dpi now, and if RANDR information is not available we fall back to the core DPI.

screen: dpi_scale function

87ab7fb

Scale a length to account for the DPI of the given screen.

DPI rounding policy

3e2a134

Not all user appreciated fractional scaling, make it easier for them to snap to integers.

Basic DPI testing

43e33e5

xresources.apply_dpi(): use s:dpi_scale directly

b816df5

textbox: switch to font DPI API

38a8932

wibar: take screen into account in default size

cacb494

hotkey popup: apply DPI scaling at wibox creation time

61464c3

Values should be defined unscaled, and scaled based on the DPI of the screen where the popup appears.

naughty: per-screen DPI scaling

d740a7d

awful.mouse.snap: use new screen DPI scaling

b823218

Oblomov force-pushed the mixed-dpi-groundwork branch from 1f9e1ff to b823218 Compare October 22, 2017 22:52

blueyed requested review from blueyed, psychon and Elv13 October 28, 2017 19:06

psychon reviewed Oct 30, 2017

View reviewed changes

psychon mentioned this pull request Oct 30, 2017

What's the high-DPI plan for awesome? #2091

Closed

psychon requested changes Oct 30, 2017

View reviewed changes

Mixed-DPI groundwork #2053

Mixed-DPI groundwork #2053

Oblomov commented Oct 7, 2017

codecov bot commented Oct 7, 2017 •

edited

psychon Oct 8, 2017 •

edited

Oblomov Oct 8, 2017

psychon Oct 8, 2017

psychon Oct 8, 2017

psychon Oct 8, 2017

psychon Oct 8, 2017

Oblomov Oct 8, 2017

psychon Oct 8, 2017

psychon Oct 8, 2017

psychon Oct 8, 2017

Oblomov commented Oct 28, 2017

psychon left a comment

psychon Oct 30, 2017

psychon Oct 30, 2017

psychon Oct 30, 2017

psychon Oct 30, 2017

psychon Oct 30, 2017

psychon Oct 30, 2017

psychon Oct 30, 2017

Oblomov Oct 30, 2017

psychon Oct 31, 2017

psychon Oct 30, 2017

psychon Oct 30, 2017

Oblomov Oct 30, 2017

psychon Oct 30, 2017

psychon left a comment


		runner.run_steps(steps)

		-- vim: filetype=lua:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80

Mixed-DPI groundwork #2053

Are you sure you want to change the base?

Mixed-DPI groundwork #2053

Conversation

Oblomov commented Oct 7, 2017

codecov bot commented Oct 7, 2017 • edited

Codecov Report

psychon Oct 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oblomov commented Oct 28, 2017

psychon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psychon left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 7, 2017 •

edited

psychon Oct 8, 2017 •

edited