D10415: Fix realDpi function for Mac

[Albert Astals Cid]

aacid added subscribers: kde-mac, aacid.
aacid added a comment.


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

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

REPOSITORY
R223 Okular

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular
Cc: aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[Christoph Cullmann]

cullmann added a comment.


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....

REPOSITORY
R223 Okular

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular
Cc: cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[René J.V. Bertin]

rjvbb added a comment.

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) 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)?

REPOSITORY
R223 Okular

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[René J.V. Bertin]

rjvbb accepted this revision.
rjvbb added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[Sergio Bragin]

sbragin edited the summary of this revision.

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[Luigi Toscano]

ltoscano added a comment.


This is the list. Okular supports from Qt 5.8:
https://doc.qt.io/qt-5.10/supported-platforms-and-configurations.html#qt-5-8

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[Christoph Cullmann]

cullmann added a comment.


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.

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[Sergio Bragin]

sbragin added a comment.

Post by Christoph Cullmann
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.

Post by Christoph Cullmann
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.

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[Christoph Cullmann]

cullmann added a comment.

Post by Sergio Bragin
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.

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[Sergio Bragin]

sbragin added a comment.

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

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.

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[René J.V. Bertin]

Hi Sergio,

Post by Sergio Bragin
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).

Post by Sergio Bragin
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)?

Post by Sergio Bragin
Yes, those things are in progress.

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

[Sergio Bragin]

sbragin added a comment.

Post by René J.V. Bertin
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 .
https://github.com/RJVB/macstrop/tree/master/kf5/kf5-okular/files

Yes, those things are in progress.

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[Sergio Bragin]

sbragin added a comment.

Post by Christoph Cullmann
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.

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[Sergio Bragin]

sbragin added a comment.

Post by René J.V. Bertin
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 <https://phabricator.kde.org/R223:f42a3bad65200267cfe04cf584c203e70a3a6ec0> more than two years ago (!)). But before opening this issue here, I found some time to actally 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.

Post by René J.V. Bertin
Edit: wait, is that actually you? =]

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

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[René J.V. Bertin]

rjvbb added a comment.

Post by Sergio Bragin
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.

Post by Sergio Bragin
(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)?

Post by Sergio Bragin
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? :)

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[Sergio Bragin]

sbragin added a comment.

Post by René J.V. Bertin
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

Post by René J.V. Bertin

Post by Sergio Bragin
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?

Post by René J.V. Bertin

Post by Sergio Bragin
(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.

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[René J.V. Bertin]

rjvbb added a comment.

Post by Sergio Bragin
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.

Post by Sergio Bragin
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 :)

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[René J.V. Bertin]

rjvbb added a comment.


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

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham

[Albert Astals Cid]

aacid added a comment.


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

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham, simgunz

[Sergio Bragin]

sbragin added a comment.

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

Please, go ahead.

REPOSITORY
R223 Okular

BRANCH
master

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham, simgunz

[Albert Astals Cid]

aacid closed this revision.

REPOSITORY
R223 Okular

REVISION DETAIL
https://phabricator.kde.org/D10415

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham, simgunz

[Albert Astals Cid]

This revision was automatically updated to reflect the committed changes.
Closed by commit R223:54c741844b76: Fix realDpi function for Mac (authored by sbragin, committed by aacid).

REPOSITORY
R223 Okular

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10415?vs=26841&id=27486

REVISION DETAIL
https://phabricator.kde.org/D10415

AFFECTED FILES
core/utils.cpp

To: sbragin, #okular, rjvbb
Cc: rjvbb, cullmann, aacid, kde-mac, ltoscano, #okular, michaelweghorn, ngraham, simgunz