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

Support for HiDPI display on vanilla emacs #13

Closed
wants to merge 4 commits into from
Closed

Support for HiDPI display on vanilla emacs #13

wants to merge 4 commits into from

Conversation

iostapyshyn
Copy link
Contributor

@iostapyshyn iostapyshyn commented May 13, 2021

This is related to the long-standing issue of displaying pdf contents in proper resolution on retina Macs using vanilla (not a fork) emacs:
politza/pdf-tools#51
doomemacs/doomemacs#4989

This commit of Emacs introduces a function frame-scale-factor which returns the scale factor of the backing store on NS framework in particular. So far pdf-tools has been using backing-scale-factor attribute of (frame-monitor-attributes), which is not a standard emacs attribute, but a feature provided by emacs-mac port.

Also, for some reason the scaling wasn't applied if png was used as the image type. I've tested around and png with scaling works flawlessly. Rather contrary, png is the only image type which successfully scales for me, and using imagemagick does not work (even on regular image buffers). image-io is also something not present in GNU Emacs and provided solely by emacs-mac

@dangom
Copy link

dangom commented May 13, 2021

This works but may need a small change:

;;(or (and (fboundp 'frame-scale-factor)
;;               (frame-scale-factor))

(or (and (fboundp 'frame-scale-factor)
               (truncate (frame-scale-factor)))

At least at my end, frame-scale-factor returns 2.0, which leads to problems with epdfinfo, since it expects a natural number:

pdf-info-query: epdfinfo: Expected natural number:1200.0

@iostapyshyn
Copy link
Contributor Author

@dangom Thanks for letting me know! For some reason I didn't encounter this on my system yet. Let me fix that

@rudolf-adamkovic
Copy link
Contributor

rudolf-adamkovic commented May 27, 2021

This PR cannot be merged soon enough! Thank you @iostapyshyn for fixing the problem.

@vedang vedang closed this in f771c93 May 29, 2021
@vedang
Copy link
Owner

vedang commented May 29, 2021

This change is now merged in :) Thank you @iostapyshyn for the PR!

@rudolf-adamkovic
Copy link
Contributor

@vedang I have just updated all Emacs packages on my workstation but see no difference. Is that because this PR, while merged, has not yet been released to MELPA?

@vedang
Copy link
Owner

vedang commented May 29, 2021

@salutis : I can see that the version is available on MELPA now. I don't know the exact times at which MELPA updates packages, but I don't think it's more than 30 mins from commit :)

You should have the change. Have you set pdf-view-use-scaling to t?

@rudolf-adamkovic
Copy link
Contributor

@vedang And you are right! I closed the book I was reading, called the auto-package-update-now function, and opened the book again. Everything is sharp now. Wohoo! Thank you so much.

@rudolf-adamkovic
Copy link
Contributor

rudolf-adamkovic commented May 29, 2021

@vedang Interesting. After the update isearch is broken. As I type, the PDF document is searched incrementally, but it also becomes more and more zoomed in. Then, when I confirm with RET, the PDF disappears. (Occur is broken as well.) Is this an issue on my end, or do you observe this too?

@vedang
Copy link
Owner

vedang commented May 29, 2021

@salutis I cannot repro this, both isearch and pdf-occur are working fine for me.

@vedang
Copy link
Owner

vedang commented May 29, 2021

@salutis : I am not on a Mac , I am on Linux. This might be the reason I am unable to repro the problem as well. Waiting on someone else from the Mac world to repro this and report it.

@vedang
Copy link
Owner

vedang commented May 29, 2021

@salutis : I have now confirmed that it's working correctly on Mac as well. (it == default pdf-tools with no extra configuration other than pdf-view-use-scaling set to t). Let me know if you are able to debug the problem on your end.

Update: My Emacs on Mac OSX does not have frame-scale-factor :( , so I am not able to confirm this / repro this on my machine. I will wait for reports from other people at this point in time, not looking into this further.

@iostapyshyn
Copy link
Contributor Author

@salutis @vedang

I can reproduce this on Emacs 28 but not on emacs-mac. In particular these lines in pdf-view.el look suspicious:

pdf-tools/lisp/pdf-view.el

Lines 920 to 922 in f771c93

(when (and (eq (framep-on-display) 'mac)
(= (pdf-util-frame-scale-factor) 2))
(list :data-2x ,image-data)))))))

Also I hope @nnicandro can shed some light on why scaling wasn't applied to pdf image type initially.

@rudolf-adamkovic
Copy link
Contributor

@iostapyshyn Bingo, I am on Emacs 28 as well.

@vedang
Copy link
Owner

vedang commented May 29, 2021

The problem is here:

(* width (pdf-util-frame-scale-factor))
(in pdf-info-renderpage.

I'm not sure why the width param is being multiplied with the scaling factor here, @nnicandro can you please help with this ? Also, imagemagick based scaling / rendering is not working on my setup. So I'm not sure if this was a bug earlier as well, but was masked since the code-path did not execute for png images.

For the moment, I am pushing a temporary fix - removing the multiplication of width and pdf-util-frame-scale-factor. Since I don't have access to a Mac running emacs-mac , I have tested it by hard-coding pdf-util-frame-scale-factor to 2 on my local setup, and everything seems to be working correctly.

@iostapyshyn @salutis : Please test this and let me know what you see.

vedang added a commit that referenced this pull request May 29, 2021
`pdf-view-image-size` passes in the width of the current page to
`pdf-info-renderpage` during an isearch. Use this as-is, instead of
multiplying it with the `pdf-util-frame-scale-factor`.

This is a potential fix for the bug raised by PR #13.

I'm not sure if this was a bug earlier as well, but was masked since
the code-path did not execute for `png` images.

I will treat this as a temporary change until I can better understand
the original reasons behind putting it in. Since I don't have access
to a Mac running `emacs-mac` , I have tested it by hard-coding
`pdf-util-frame-scale-factor` to 2 on my local setup, and everything
seems to be working correctly.
@iostapyshyn
Copy link
Contributor Author

iostapyshyn commented May 29, 2021

@vedang That change breaks the scaling. Basically there are two references of pdf-util-frame-scale-factor in the code, which is the one I linked in my previous comment and the one you mentioned. It looks like emacs-mac is doing something that vanilla emacs doesn't so the isearch works properly. That's why that snippet in pdf-view.el attracted my attention: :data-2x doesn't look like something vanilla emacs expects in create-image

Also there are a few places in code where scaling factor of 2 is hardcoded:

(* 2 image-width))))

(* 2 (car size)))))

I suppose that is something that we need to check once we figure out what is going on.

@vedang
Copy link
Owner

vedang commented May 29, 2021

@iostapyshyn : Vanilla Emacs won't get the :data-2x property because of the (eq (framep-on-display) 'mac) guard. This frame-type is not defined for Vanilla Emacs (looking at Emacs 28.0.50 source code). The frame-type mac seems to have been added in the emacs-mac source code.

Since you've tested that the scaling is breaking with 8b2debab commit, I am reverting it. I am also adding back the guard which enabled scaling for imagemagick type back into the code for the moment, so that isearch and occur are not broken for regular users.

Opening a new issue #19 (so that I have an open issue to remind me about this)

@iostapyshyn
Copy link
Contributor Author

@vedang I don't think the change broke something for regular (as in not Emacs 28 on Mac) users, did it? frame-scale-factor only returns 2.0 on NS in Emacs 28 and all other systems have 1.0 and therefore shouldn't be affected. Otherwise the pixelated pdf display without it is barely usable on Macs. I will continue investing why isearch breaks as well.

@vedang
Copy link
Owner

vedang commented May 29, 2021

@iostapyshyn : Since I don't have a Mac, I don't have anything breaking in front of my eyes :) . My understanding is that it isn't frame-scale-factor but the fact that pdf-view-use-scaling now runs on PNG images that is causing the break. Can you test this for me? You can do this by moving pdf-tools to before your changes were merged in and then removing the memq statement.

I will wait for you to get back before adding the guard back in. For the moment, I am only reverting the commit I have added above.

vedang added a commit that referenced this pull request May 29, 2021
This reverts commit 8b2deba, which
was introduced in an attempt to fix #13. I am now tracking this issue
under #19.

@iostapyshyn has confirmed that this change breaks scaling and is not
the correct fix.
@iostapyshyn
Copy link
Contributor Author

@vedang Since the version before my changes didn't use frame-scale-factor, removing the memq wouldn't change anything: the pdf-util-frame-scale-factor function returns 1 for vanilla emacs either way as it has no way to detect retina screens (something that is provided by frame-scale-factor since Emacs 28). Thus, everything would work as before and that's the result I get from the test case you requested.

Restoring memq but keeping my changes would cause pdf-util-frame-scale-factor to return 1 for png and 2 for imagemagick. But as already mentioned in this thread, the scaling doesn't seem to work with imagemagick.

So basically there are two possibilities:

  1. Keep the scaling for png and allow Emacs 28 users on Mac to have proper resolution rendering, but (temporarily) broken search
  2. Don't allow scaling for png and keep Emacs 28 users on Mac with half the resolution (see attached image) but working search function.

Screen Shot 2021-05-29 at 22 45 56

@iostapyshyn
Copy link
Contributor Author

I should add, users on Emacs 27 or lower and users on devices that return 1 for frame-scale-factor (everything but retina macs currently) should not be affected by any of these changes.

@vedang
Copy link
Owner

vedang commented May 29, 2021

👍 I am leaving the change in right now, with the hope that we can pinpoint the bug in the search and fix it before people notice and start complaining. I cannot debug this because I don't have the necessary setup. I will help with the investigation as I can. Since search is completely broken at this point in time, I will have to revert this change if we cannot find a fix in a reasonable amount of time.

I will also open a separate issue to track the breakage with imagemagick

@rudolf-adamkovic
Copy link
Contributor

FYI: I am back to "normal" Emacs 27.2 and search works. PDFs are fuzzy, though. 😞

@iostapyshyn
Copy link
Contributor Author

iostapyshyn commented May 30, 2021

@salutis this is to be expected, since frame-scale-factor is a new function in Emacs 28. Could you check my other pull request #22 with Emacs 28? It fixes the search issue.

@rudolf-adamkovic
Copy link
Contributor

@iostapyshyn Oh, I see. I replied in #22. Amazing work.

@vedang vedang added the new feature implementation This is a substantial code change and / or implements significant new functionality label Dec 31, 2021
aikrahguzar pushed a commit to aikrahguzar/pdf-tools that referenced this pull request Aug 3, 2023
aikrahguzar pushed a commit to aikrahguzar/pdf-tools that referenced this pull request May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature implementation This is a substantial code change and / or implements significant new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants