Fall back to reported DPI if Xft.dpi is not present #1477

Oblomov · 2017-01-26T13:28:34Z

No description provided.

Elv13 · 2017-01-26T13:55:29Z

As discussed on IRC, please change this patch so it doesn't mess the DPI if it's below 200 (or 160, or random number in the wild). This http://dpi.lv/ websites show no "official" HiDPI monitors below 200.

Rationals: It is a behavior change and a very unexpected one for 95% of the userbase. On the other hand, I own an HiDPI device for testing my apps and it is truly unusable when DPI is set to 96. So Awesome has to fix itself as well as it can. But only when not doing so renders it unusable.

(This is a parallel issue and doesn't mean I approve or disprove the patch itself.)

psychon

If you want to compute some DPI values: How about using the average of the screen's outputs?

local function get_dpi(s)
  s = screen[s]
  local mm_per_inch = 25.4
  local sum, count = 0, 0
  local geo = s.geometry
  for _, o in pairs(s.outputs) do
    local dpi_width = geom.width * mm_per_inch / o.mm_width
    local dpi_height = geom.height * mm_per_inch / o.mm_height
    sum = sum + dpi_width + dpi_height
    count = count + 2
  end
  return count == 0 and 96 or sum/count
end

(Without RandR, this will fall back to 96 again)

psychon · 2017-01-26T13:59:40Z

root.c

+{
+    lua_pushinteger(L, globalconf.screen->width_in_millimeters);
+    lua_pushinteger(L, globalconf.screen->height_in_millimeters);
+    return 2;


I think we already talked about this yesterday, but ok:

Find a machine with two monitors. Start awesome when only one monitor is active. Then xrandr --output FOO --auto --above BAR the other monitor. Query the resulting value that you get from the function that this patch added.

The "size in MM" will stay what it was when the X11 connection opened. The "size in pixels" will include the new monitor (event_handle_configurenotify from event.c explicitly updates the value). The resulting DPI value will be "funny", to say the least.

This can be handled in the same way RANDR itself handles it internally: updating the *_in_millimeters too, keeping the ratio constant. I can add a commit that does this if deemed appropriate.

Scratch that, we actually get the new physical size via RRChangeNotifyEvent, we just need to keep track of the update.

Oh right, I actually missed that.

psychon · 2017-01-26T14:00:15Z

lib/beautiful/xresources.lua

@@ -83,8 +83,16 @@ function xresources.get_dpi(s)
        if awesome and awesome.xrdb_get_value then
            xresources.dpi = tonumber(awesome.xrdb_get_value("", "Xft.dpi"))


What does "link in comment" in the commit message refer to? What kind of comment can a commit message have?

I meant the comment added to the function in the actual commit. I can rephrase the commit message to include the link itself to make it clearer.

psychon · 2017-01-26T14:12:19Z

lib/beautiful/xresources.lua

+            local root = require("root")
+            _, h = root.size()
+            _, hmm = root.size_mm()
+            xresources.dpi = round(h*25.4/hmm)


I do not like 25.4-magic-values and you never even tested this.

I can define the constants inches_to_millimeter to explain the 25.4

As for testing, I'm actually running awesome with this code now, did I miss something?

Uhm, require("root") failed quite badly on Travis since no such module was found. It's just a global variable.

That's interesting, how the heck did it work here then? I don't even get an error in the log. Very odd. I'll get rid of it.

Oh, actually it fails on Travis while running unit tests, which is half-unrelated. Wow, I did not know that the C code makes things like root requireable....

Oblomov · 2017-01-26T16:28:08Z

Elv13:

As discussed on IRC, please change this patch so it doesn't mess the DPI if it's below 200 (or 160, or random number in the wild). This http://dpi.lv/ websites show no "official" HiDPI monitors below 200.

[snip]

On the other hand, I own an HiDPI device for testing my apps and it is truly unusable when DPI is set to 96. So Awesome has to fix itself as well as it can. But only when not doing so renders it unusable.

(This is a parallel issue and doesn't mean I approve or disprove the patch itself.)

I also feel that this is a matter which is orthogonal to my patch, since funky not-really-HiDPI scaling can also affect current awesome if the user sets Xft.dpi resource to a value such a 140. However, if you feel that this should be solved first, I could work on that before getting back to the fallback aspect.

Rationals: It is a behavior change and a very unexpected one for 95% of the userbase.

Well, unless 95% of the userbase actually changes their Xorg configuration to report anything other than 96 (which is what Xorg does unless told otherwise), they won't really be affected by the change, but I do get your point about it being a breaking change.

Oblomov · 2017-01-26T16:49:39Z

psychon:

If you want to compute some DPI values: How about using the average of the screen's outputs?

As someone who frequently has to work with mixed DPI setups, I would recommend against the solution. The result is that things end up appearing too small in high-DPI monitors, and too large in low-DPI monitors. Picking just the highest (or lowest, but in my experience highest is better) gives better results.

Of course, the ideal solution is handling different DPI monitors separately, so that things do not appear significantly larger/smaller in any of them. However, this would require extensive engine to the theme engine, as all the properties would have to become a function of screen at least.

Xrandr provides new physical sizes in millimeters when sending a RRChangeNotifyEvent, either to preserve DPI when monitors are added, or to change DPI when a DPI change was requested. Keep track of the changes so that our DPI information is always fresh.

This function returns the (alleged) physical size of the root window.

actionless · 2017-01-26T21:22:49Z

Of course, the ideal solution is handling different DPI monitors separately, so that things do not appear significantly larger/smaller in any of them. However, this would require extensive engine to the theme engine, as all the properties would have to become a function of screen at least.

do you mean this https://awesomewm.org/apidoc/libraries/beautiful.xresources.html#set_dpi + https://awesomewm.org/apidoc/libraries/beautiful.xresources.htm#apply_dpi?

if you'll feed them screen as argument it should do the thing

however not all the theme variables can be functions. but i would be thinking in that direction, like it's already implemented for theme.wallpaper in order to handle different wallpaper size on different screen

blueyed · 2017-01-26T23:19:50Z

Just some drive-by comments:

set_dpi / apply_dpi work quite well already for me
my "HiDPI" screen is the laptop's internal one, but only with ~170 - still almost twice the default of 96.
I've experimented with some start-with-dpi wrapper that would be used by awesome to start new programs, and would temporarily change my DPI settings (via xsettingsd and xrdb), launch the program and set it back. It uses/used beautiful.xresources.get_dpi(s) - and I'm setting it per screen (but mainly hardcoded currenty).

Some snippet from that: (yes, I do not know why I've used workarea - I think the documentation can be improved/clearer on the differences).

for s in screen do
  local inch = width / 25.4
  local dpi = math.floor(s.workarea.width / inch)
  beautiful.xresources.set_dpi(dpi, s)
end

Oblomov · 2017-01-27T06:53:02Z

@actionless : the thing is, themes in general do no use apply_dpi with a screen argument. If not everything can be made into a function, some other way would have to be envisioned. An option would be to have themes only report unscaled lengths and sizes (i.e. with lengths at the reference 96 DPI), and then have widgets scale them depending on the screen they get rendered on.

However, I think that this is a separate topic of discussion that has its merits independently from this changeset (whose only aim is to fallback to what the user set up as core DPI, rather than what to do with it). I'll look into opening a separate issue to track the discussion about how to handle this.

@blueyed : I have had cursory discussions with @psychon on IRC about that, and we could even have per-screen DPI autoconfiguration as well, by using RANDR (of course without removing the user's option to override it) and keeping it dynamically update on RRChangeNotifyEvent. The biggest hurdle is choosing what DPI to use when outputs overlap (my idea is to use the highest DPI, since such a setup is generally used to provide a 'zoomed in' view anyway, but this should be decided anyway).

There is another thing that should be handled: again, it's not directly related to this changeset, but if we pick the native DPIs, it's going to be more important, as in most case the actual DPIs are not integer multiples of the reference one. And we should probably look into separating UI scaling from 'proper' DPI scaling. For example, @blueyed monitor has 170 DPI, which means a scaling of 1.77 over the reference 96 DPI. My eDP has 235 DPI, which would mean a scaling of 2.45 over the reference 96 DPI. in my experience, non-integer scaling produces sub-optimal results. The UI scaling should preferably follow integer steps (unless the user wants differently), although we should still have the option to use the 'true' scaling (to draw 'exact' lengths). I'll open a discussion about this too.

As documented in <https://keithp.com/~keithp/talks/xtc2001/paper/xft.html#sec-editing>, the fallback value for Xft.dpi should be the vertical DPI reported by X. On Xorg, this will generally be 96, unless the user has overridden the value by either forcing Xorg to report the EDID-derived DPI, or by setting the DPI themselves (via configuration, command line, or xrandr). The 96 value is kept as ultimate fallback if anything goes wrong.

codecov-io · 2017-01-27T12:13:49Z

Current coverage is 79.66% (diff: 28.57%)

Merging #1477 into master will decrease coverage by 0.02%

@@             master      #1477   diff @@
==========================================
  Files           276        276          
  Lines         17044      17051     +7   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          13582      13583     +1   
- Misses         3462       3468     +6   
  Partials          0          0

Powered by Codecov. Last update 5a914b3...f22ef50

Elv13 · 2017-05-25T07:45:50Z

@psychon any comments on the current version of this? I am trying to see if we can clear the backlog a bit (I guess you already notified ;) )

psychon

Seems like I missed the latest version of these patches

Elv13 · 2017-05-25T20:41:31Z

Thanks

psychon · 2017-05-26T11:01:42Z

This broke the build on Travis: https://travis-ci.org/awesomeWM/awesome/builds/236151601

    lib/beautiful/xresources.lua:94:17: setting non-standard global variable _
    lib/beautiful/xresources.lua:94:20: setting non-standard global variable h
    lib/beautiful/xresources.lua:95:17: setting non-standard global variable _
    lib/beautiful/xresources.lua:95:20: setting non-standard global variable hmm
    lib/beautiful/xresources.lua:96:20: accessing undefined variable hmm
    lib/beautiful/xresources.lua:97:44: accessing undefined variable h
    lib/beautiful/xresources.lua:97:57: accessing undefined variable hmm

This fixes warnings that were introduced with f22ef50 / awesomeWM#1477. Signed-off-by: Uli Schlachter <[email protected]>

actionless · 2017-05-26T13:14:27Z

lib/beautiful/xresources.lua

+        if not xresources.dpi then
+            if root then
+                local mm_to_inch = 25.4
+                _, h = root.size()


yup, those should be local

but i am wondering why those weren't catched by travis before merging

Travis caught it. Just no one looked at the result: https://travis-ci.org/awesomeWM/awesome/builds/195847851

This fixes warnings that were introduced with f22ef50 / #1477. Signed-off-by: Uli Schlachter <[email protected]>

Elv13 · 2017-05-26T20:15:01Z

That one's on me, sorry.

psychon · 2017-05-27T06:50:36Z

I guess I didn't notice it either.

actionless · 2017-05-28T03:51:55Z

i didn't mean to blame anybody, i just thought what the tests were false positive (wasn't looking on the status when asking)

This fixes warnings that were introduced with f22ef50 / awesomeWM#1477. Signed-off-by: Uli Schlachter <[email protected]>

psychon requested changes Jan 26, 2017

View reviewed changes

Oblomov added 2 commits January 26, 2017 22:04

Add root.size_mm

f7d8233

This function returns the (alleged) physical size of the root window.

Oblomov force-pushed the dpifallback branch from 113ffa4 to 187d7f9 Compare January 26, 2017 21:19

Oblomov force-pushed the dpifallback branch from 187d7f9 to f22ef50 Compare January 27, 2017 12:03

psychon approved these changes May 25, 2017

View reviewed changes

psychon added this to the next: pull requests to be merged soon milestone May 25, 2017

Elv13 merged commit 27c3b96 into awesomeWM:master May 25, 2017

psychon added a commit to psychon/awesome that referenced this pull request May 26, 2017

Fix Luacheck warnings

e622742

This fixes warnings that were introduced with f22ef50 / awesomeWM#1477. Signed-off-by: Uli Schlachter <[email protected]>

psychon mentioned this pull request May 26, 2017

Fix Luacheck warnings #1798

Merged

actionless reviewed May 26, 2017

View reviewed changes

blueyed pushed a commit that referenced this pull request May 26, 2017

Fix Luacheck warnings (#1798)

e7a1745

This fixes warnings that were introduced with f22ef50 / #1477. Signed-off-by: Uli Schlachter <[email protected]>

petoju pushed a commit to petoju/awesome that referenced this pull request Sep 10, 2017

Fix Luacheck warnings (awesomeWM#1798)

e45b8d7

This fixes warnings that were introduced with f22ef50 / awesomeWM#1477. Signed-off-by: Uli Schlachter <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fall back to reported DPI if Xft.dpi is not present #1477

Fall back to reported DPI if Xft.dpi is not present #1477

Oblomov commented Jan 26, 2017

Elv13 commented Jan 26, 2017 •

edited

Loading

psychon left a comment

psychon Jan 26, 2017

Oblomov Jan 26, 2017

Oblomov Jan 26, 2017

psychon Jan 27, 2017

psychon Jan 26, 2017

Oblomov Jan 26, 2017

psychon Jan 26, 2017

Oblomov Jan 26, 2017

psychon Jan 26, 2017

Oblomov Jan 26, 2017

psychon Jan 26, 2017

Oblomov commented Jan 26, 2017

Oblomov commented Jan 26, 2017

actionless commented Jan 26, 2017 •

edited

Loading

blueyed commented Jan 26, 2017

Oblomov commented Jan 27, 2017

codecov-io commented Jan 27, 2017 •

edited

Loading

Elv13 commented May 25, 2017

psychon left a comment

Elv13 commented May 25, 2017

psychon commented May 26, 2017

actionless May 26, 2017

psychon May 26, 2017

Elv13 commented May 26, 2017

psychon commented May 27, 2017

actionless commented May 28, 2017

		@@ -83,8 +83,16 @@ function xresources.get_dpi(s)
		if awesome and awesome.xrdb_get_value then
		xresources.dpi = tonumber(awesome.xrdb_get_value("", "Xft.dpi"))

Fall back to reported DPI if Xft.dpi is not present #1477

Fall back to reported DPI if Xft.dpi is not present #1477

Conversation

Oblomov commented Jan 26, 2017

Elv13 commented Jan 26, 2017 • edited Loading

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

Oblomov commented Jan 26, 2017

Oblomov commented Jan 26, 2017

actionless commented Jan 26, 2017 • edited Loading

blueyed commented Jan 26, 2017

Oblomov commented Jan 27, 2017

codecov-io commented Jan 27, 2017 • edited Loading

Current coverage is 79.66% (diff: 28.57%)

Elv13 commented May 25, 2017

psychon left a comment

Choose a reason for hiding this comment

Elv13 commented May 25, 2017

psychon commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elv13 commented May 26, 2017

psychon commented May 27, 2017

actionless commented May 28, 2017

Elv13 commented Jan 26, 2017 •

edited

Loading

actionless commented Jan 26, 2017 •

edited

Loading

codecov-io commented Jan 27, 2017 •

edited

Loading