Fix realDpi function for Mac
ClosedPublic

Authored by sbragin on Feb 9 2018, 8:08 PM.

Details

Summary

Removes old Utils::realDpi function for Mac. The old code employs functions
CGDisplayCurrentMode and CGDisplayIOServicePort, which are deprecated as
of versions 10.6 and 10.9, respectively. Now Mac uses the same code, as
Linux does, since it works out of the box.

This commit automatically fixes the long-standing bug of having an
implementation of realDpiX() and realDpiY(), which are not declared in
the utils.h file.

Diff Detail

Repository
R223 Okular
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sbragin created this revision.Feb 9 2018, 8:08 PM
Restricted Application added a project: Okular. · View Herald TranscriptFeb 9 2018, 8:08 PM
Restricted Application added a subscriber: Okular. · View Herald Transcript
sbragin requested review of this revision.Feb 9 2018, 8:08 PM

Can you please expand the commit message? Is the old code not required anymore because Qt now "just works"? Does it apply also to the minimum Qt versions required by Okular, on all the supported versions of macOS?

sbragin edited the summary of this revision. (Show Details)Feb 9 2018, 8:11 PM
sbragin added a reviewer: Okular.
sbragin edited the summary of this revision. (Show Details)

Can you please expand the commit message? Is the old code not required anymore because Qt now "just works"? Does it apply also to the minimum Qt versions required by Okular, on all the supported versions of macOS?

It does work. The old one uses deprecated methods, doesn't work properly and can not be even compiled, in fact.

I have tested against Qt 5.9.3 and 5.10 on Mac OS 10.12.

Works with Qt 5.8.0.

@kde-mac anyone that has a Mac can test/review this?

If noone disagrees in a week i'll commit it.

In our company we use the logicalDpiX()/logicalDpiY() on the widget we paint to, that works on macOS, too.

I assume the physicalDotsPerInchY() should work the same.

Sidenote: do we need to go over QScreen? We just use the function of the widget in question which is there via QPaintDevice.
Which here would be just widgetOnScreen->physicalDpi....

rjvbb added a subscriber: rjvbb.EditedFeb 11 2018, 12:38 PM

It does work. The old one uses deprecated methods, doesn't work properly and can not be even compiled, in fact.

How so? I have been building Okular on Mac for a long time (IIRC since before the master branch was KF5 based). I'm on Qt 5.9.3 and OS X 10.9.5 and the only change I currently make to utils.cpp is:

diff --git a/core/utils.cpp b/core/utils.cpp
index c9bfc2d56be26ee8a49164eb93f566352adb0696..b8c955fef9a4de7659632d7ee509961b4a540937 100644
--- a/core/utils.cpp
+++ b/core/utils.cpp
@@ -134,7 +134,7 @@ QSizeF Utils::realDpi(QWidget* widgetOnScreen)
         return err;
     }
 
-double Utils::realDpiX()
+static double realDpiX()
 {
     double x,y;
     CGDisplayErr err = GetDisplayDPI( CGDisplayCurrentMode(kCGDirectMainDisplay),
@@ -144,7 +144,7 @@ double Utils::realDpiX()
     return err == CGDisplayNoErr ? x : 72.0;
 }
 
-double Utils::realDpiY()
+static double realDpiY()
 {
     double x,y;
     CGDisplayErr err = GetDisplayDPI( CGDisplayCurrentMode(kCGDirectMainDisplay),

Which is probably the equivalent of the proposed change :) The resulting code compiles, however, on 10.9.5 and newer (IIRC also on 10.13).

Does it apply also [...] on all the supported versions of macOS?

What are the supported versions of OS X anyway, beyond those called macOS? All versions that run an unpatched minimum Qt version (that would mean OS X 10.9)?

rjvbb accepted this revision.Feb 11 2018, 12:46 PM

On a sideways related note:
I have at least one patch that improves Okular/Mac integration, allowing it to act as an alternative or complement to the native Preview.app .

I've never made the effort to assess and polish them up for upstreaming because I rarely use Okular on Mac, but if someone wants to cherry-pick they're here:
https://github.com/RJVB/macstrop/tree/master/kf5/kf5-okular/files

This revision is now accepted and ready to land.Feb 11 2018, 12:46 PM
sbragin edited the summary of this revision. (Show Details)Feb 11 2018, 12:47 PM

I would go for the Qt functions, as their results are consistent with what Qt itself will later use anyways.
My only nitpick is why one goes over QScreen and not just asks the widget.

In our company we use the logicalDpiX()/logicalDpiY() on the widget we paint to, that works on macOS, too.

I assume the physicalDotsPerInchY() should work the same.

I think they are not exactly the same. physicalDotsPerInchY() would give twice difference in the resolution depending on whether one uses the native or HiDPI mode. But logicalDpiX()/logicalDpiY() would return the same value in both cases.

Sidenote: do we need to go over QScreen? We just use the function of the widget in question which is there via QPaintDevice.
Which here would be just widgetOnScreen->physicalDpi....

Well, that's a relevant question, but should be addressed to the Okular developers. I just kept the code that was already there.

I think they are not exactly the same. physicalDotsPerInchY() would give twice difference in the resolution depending on whether one uses the native or HiDPI mode. But logicalDpiX()/logicalDpiY() would return the same value in both cases.

No, I meant: if the logical variant works for us here, the physical variant should work fine, too.
That they not give you the same results in all cases is clear, given they are logical vs. physical.

It does work. The old one uses deprecated methods, doesn't work properly and can not be even compiled, in fact.

How so? I have been building Okular on Mac for a long time (IIRC since before the master branch was KF5 based). I'm on Qt 5.9.3 and OS X 10.9.5 and the only change I currently make to utils.cpp is:

Hi René! Nice to see you here! Apart from warnings and the errors, that you had fixed in your patch also, the old version was giving me some crap, while I was testing non-native resolutions. I don't have 10.9 at hand, so, it would be good if you could check that part of Linux/pure Qt code. I confirm that it works with 10.11 and 10.12.

On a sideways related note:
I have at least one patch that improves Okular/Mac integration, allowing it to act as an alternative or complement to the native Preview.app .

I've never made the effort to assess and polish them up for upstreaming because I rarely use Okular on Mac, but if someone wants to cherry-pick they're here:
https://github.com/RJVB/macstrop/tree/master/kf5/kf5-okular/files

Yes, those things are in progress.

No, I meant: if the logical variant works for us here, the physical variant should work fine, too.
That they not give you the same results in all cases is clear, given they are logical vs. physical.

Ok, got it.

rjvbb added a comment.EditedFeb 11 2018, 1:47 PM

Hi Sergio,

the old version was giving me some crap, while I was testing non-native resolutions.

Well, deprecated code is probably not maintained beyond changes required to compile it, so may not function correctly with newer hardware or drivers. Other than that Apple is sometimes surprisingly conservative in keeping deprecate functions and features (then again, this is in *Core*Foundation).

I don't have 10.9 at hand, so, it would be good if you could check that part of Linux/pure Qt code. I confirm that it works with 10.11 and 10.12.

Do we agree that my patch is the minimum way of achieving the same thing your patch does? Not that I want to be lazy, but if it is I can already confirm that I have not noticed any issues with using the standard Utils::realDPI function.

Did you verify the actual size at which elements are shown? If so maybe you can your test document and protocol so I can verify this on 10.9 (and maybe have it verified on 10.13 by one of my users)?

Edit: wait, is that actually you? =]

Yes, those things are in progress.

Don't hesitate to borrow from my implementation(s) instead of reinventing the wheel.

sbragin added a comment.EditedFeb 11 2018, 2:53 PM

Do we agree that my patch is the minimum way of achieving the same thing your patch does? Not that I want to be lazy, but if it is I can already confirm that I have not noticed any issues with using the standard Utils::realDPI function.

I agree that your patch is the minimal way to have Okular compiled on Mac. Initially, I had implemented an analogous remedy, when I had first encountered this bug (by the way, the bug was introduced with commit f42a3ba more than two years ago (!)). But before opening this issue here, I found some time to actually test the old code a bit. So, the existing code is outdated and not ensured to be supported on later versions of Mac. At the current moment, it should give (almost) the same result as the "new" one, if the user doesn't have some weird display settings (e.g., if the user doesn't set the resolution to 720p instead of native 1080p; of course, not many people would like to do that). The "new" one works fine and eliminates the necessity of having separate parts for Mac and non-Mac. But of course, I am not in charge to decide whether the old code should be kept or not.

Edit: wait, is that actually you? =]

We discussed the option of submitting patches to Okular into upstream the other day.

rjvbb added a comment.Feb 11 2018, 3:11 PM
I agree that your patch is the minimal way to have Okular compiled on Mac.

Not just that, it should result in the same code being used, with just the old code being included as inaccessible "junk DNA". No?

Evidently that's not a proper fix for upstream inclusion.

(e.g., if the user doesn't set the resolution to 720p instead of native 1080p; of course, not many people would like to do that).

Not on a regular screen, no. Maybe the effects are less "in your face" on a Retina screen, but why would you do it (except when running some old video game or similar)?

We discussed the option of submitting patches to Okular into upstream the other day.

OK, sorry I didn't realise that, I had issues from at least 2 people (or rather, user-IDs) recently.
So I probably don't have to ask not to change the build system so that all-inclusive app bundle builds become the default coupled to CMake's APPLE platform token? :)

Did you verify the actual size at which elements are shown? If so maybe you can your test document and protocol so I can verify this on 10.9 (and maybe have it verified on 10.13 by one of my users)?

So, I just created a simple app, and tested the part of the code in question only. The behavior for the Qt part was the same as in Linux. I also wrote a sample, which uses methods from Apple frameworks only (CGDisplayScreenSize and CGDisplayBounds), it gave the same result as the Qt code

I agree that your patch is the minimal way to have Okular compiled on Mac.

Not just that, it should result in the same code being used, with just the old code being included as inaccessible "junk DNA". No?

Evidently that's not a proper fix for upstream inclusion.

Sorry, I didn't quite get what you mean. Your patch just makes realDpiX and realDpiY global, but the rest is the same. Am I missing something?

(e.g., if the user doesn't set the resolution to 720p instead of native 1080p; of course, not many people would like to do that).

Not on a regular screen, no. Maybe the effects are less "in your face" on a Retina screen, but why would you do it (except when running some old video game or similar)?

It was just testing, nothing more. You never know, what a user decides to do.

rjvbb added a comment.Feb 11 2018, 4:28 PM
Sorry, I didn't quite get what you mean. Your patch just makes realDpiX and realDpiY global, but the rest is the same. Am I missing something?

Maybe I'm missing something because I took only quick glances at the code. I was thinking that my patch was simply moving the duplicate Utils::realDPI out of the way, with the result that the application would be using the standard implementation of that function. Apparently I should have looked just a little better :-/

I now tested this patch and confirm that it works on 10.9 with Qt 5.9.3 .

A PDF generated via the print-to-pdf on an A4 sheet renders slightly too large. Maybe just a bit more so than with my own patch but that comparison was hardly done in a scientific manner (holding a piece of paper to my screen that may no longer be exactly A4). Either way, monitor DPI indications are rarely exact so Okular would need a calibration feature if it really wants to display content at a 1:1 scale.

It was just testing, nothing more. You never know, what a user decides to do.

Of course, who are we to tell them to stop ruining their eyes :)

rjvbb added a comment.Feb 11 2018, 7:33 PM

Curious, I cannot give a green light (too), please consider that done...

aacid added a comment.Feb 18 2018, 6:06 PM

So should i commit this or is there a better patch coming? (yes, i didn't really read all those posts)

So should i commit this or is there a better patch coming? (yes, i didn't really read all those posts)

Please, go ahead.

aacid closed this revision.Feb 18 2018, 6:22 PM
This revision was automatically updated to reflect the committed changes.