Upstream pdf roll

[aikrahguzar]

Hi, @vedang,
I am opening this (as a draft) to have some concrete starting point for merging support for displaying multiple pages simultaneously.

It is a draft because because some things need to be sorted out,

1. Currently this requires my fork of image-roll. Since @dalanicolai wants to focus on other projects and for me image-roll is just a means to an end, I think it would be better to import image-roll.el into pdf-roll.el. This mostly requires changing the prefix for functions and it pretty mechanical but I wanted to get yours and @dalanicolai's feedback about this. To me it seems like the best thing to do since it simplifies the life of users since they don't have to install a dependency and the file itself is around 400 lines long currently so not a very large addition especially for a project of the size of pdf-tools. I went ahead and added the image-roll code directly to the pdf-roll.el so now no external package is needed.

2. What strategy should be used to convey that the feature is experimental and might not interact well with every feature currently available? I prefer a message when pdf-view-roll-minor-mode is enabled. But I am fine with the suggestion of a flag suggested in New package image-roll (continuous-scroll) #104

3. One possible degradation in performance I know of is the loss of caching in pdf-links-read-link-action since with multiple pages displayed the labels depend on the position of a page among displayed pages. I think the current behavior can be preserved when pdf-view-roll-minor-mode is not used by caching only the first displayed page. The result will be messy but it is doable.

4. This also introduces some backward incompatibility since we need to track which page is involved for operations, e.g. pdf-view-active-region now has a different format which includes the page on which the region exists. Similarly isearch results also include information about the page. These changes are not user visible when using pdf-tools as an application but they do change some api.

5. Mostly feedback is needed from people to know what breaks. The parts I use work fine but I can't say that will be the case for everyone.

Also this PR includes #29, #113, #188 because it was too difficult for me to remove those changes from among much larger changes.

P.S. I will be travelling starting Friday and will be without my laptop and access to non-work email so I will get back to whatever discussion happens only in August.

Thanks for maintaining pdf-tools.

[vedang]

Hey @aikrahguzar ,

Thank you for opening the draft. My plan is to review and merge #188 and #39 first, followed by #29 and #113 if they still make sense (I am not sure right now as I will need to read through all the code and the places that are impacted in the pdf-tools code base)

After that, you can maybe rebase your changes on top of that work and we can then revisit this.

I will update this thread once the listed PRs are merged into pdf-tools. Once you are back in August, please rebase your work on top of the latest master and then I can pick up the code review.

Thanks,
Vedang

[aikrahguzar]

Hey @aikrahguzar ,

Thank you for opening the draft. My plan is to review and merge #188 and #39 first, followed by #29 and #113 if they still make sense (I am not sure right now as I will need to read through all the code and the places that are impacted in the pdf-tools code base)

After that, you can maybe rebase your changes on top of that work and we can then revisit this.

I will update this thread once the listed PRs are merged into pdf-tools. Once you are back in August, please rebase your work on top of the latest master and then I can pick up the code review.

That plan is good for me, thanks a lot! This change set is quite a big so I think it will take some time to review too but hopefully some people try it in the meanwhile to surface potential issues.

I was use the minor mode that will be added in #39 but stopped using it before I worked on this. I imagine the interaction of the two features will not be totally smooth and some changes similar to the one is text-selection code will need to be made. But it is better to wait for #39 to merge before making those changes.

[quotuva]

Data point:

I tried it like so:
Latest of emacs-29.1-rc1 branch.
Installed the aikrahguzar:upstream-pdf-roll branch and aikrahguzar:image-roll.el
emacs -Q
Loaded pdf-tools and tablist; ran pdf-tools-install; loaded image-roll.el and (require 'pdf-roll)
Opened this pdf.
M-x pdf-view-roll-minor-mode
Scrolling down using SPC, I get this error at the end of the 2nd page:

Debugger entered--Lisp error: (error "Invalid image specification: (space :width 25 :height 1000)")
  signal(error ("Invalid image specification: (space :width 25 :height 1000)"))
  error("Invalid image specification: %s" (space :width 25 :height 1000))
  image-display-size((space :width 25 :height 1000) t)
  image-next-line(56)
  image-scroll-up(nil)
  pdf-view-scroll-up-or-next-page(nil)
  funcall-interactively(pdf-view-scroll-up-or-next-page nil)
  call-interactively(pdf-view-scroll-up-or-next-page nil nil)
  command-execute(pdf-view-scroll-up-or-next-page)

Also, isearch only searches one page.

[aikrahguzar]

Data point:

Thanks for testing.

I tried it like so: Latest of emacs-29.1-rc1 branch. Installed the aikrahguzar:upstream-pdf-roll branch and aikrahguzar:image-roll.el emacs -Q Loaded pdf-tools and tablist; ran pdf-tools-install; loaded image-roll.el and (require 'pdf-roll) Opened this pdf. M-x pdf-view-roll-minor-mode Scrolling down using SPC, I get this error at the end of the 2nd page:

Debugger entered--Lisp error: (error "Invalid image specification: (space :width 25 :height 1000)")
  signal(error ("Invalid image specification: (space :width 25 :height 1000)"))
  error("Invalid image specification: %s" (space :width 25 :height 1000))
  image-display-size((space :width 25 :height 1000) t)
  image-next-line(56)
  image-scroll-up(nil)
  pdf-view-scroll-up-or-next-page(nil)
  funcall-interactively(pdf-view-scroll-up-or-next-page nil)
  call-interactively(pdf-view-scroll-up-or-next-page nil nil)
  command-execute(pdf-view-scroll-up-or-next-page)

I think this should be fixed now. Please let me know if it isn't.

Also, isearch only searches one page.

Can you explain more? When I use isearch it searches all visible pages.

[quotuva]

Tested it again with the latest emacs-29 branch:

I think this should be fixed now. Please let me know if it isn't.

This works now; but I have an apology to make: apparently I was running master rather than upstream-pdf-roll last time. Sorry, if it was spurious.

Can you explain more? When I use isearch it searches all visible pages.

Right now, for me, when not in pdf-roll-minor-mode isearch throws an error (wrong-type-argument listp 1) in the minibuffer on doing C-s test.
When the mode is enabled, the above search does not work again (when trying from the 1st page) because, it seems, the text is not present in the 1st page. Otherwise - for eg. when trying C-s element - it works perfectly.

(Some detail on my methodology - in case anyone else wants to try: The loading of pdf-tools and tab-list was done by: (require 'package), M-x customize-variable package-load-list: add pdf-tools and tablist, M-x package-initialize)

[aikrahguzar]

This works now; but I have an apology to make: apparently I was running master rather than upstream-pdf-roll last time. Sorry, if it was spurious.

I think those remappings were needed but I am confused that you got the backtrace you showed with master. That should not be possible.

Right now, for me, when not in pdf-roll-minor-mode isearch throws an error (wrong-type-argument listp 1) in the minibuffer on doing C-s test. When the mode is enabled, the above search does not work again (when trying from the 1st page) because, it seems, the text is not present in the 1st page. Otherwise - for eg. when trying C-s element - it works perfectly.

Thanks for explaining. I think I fixed both the problems. isearch should work regardless of mode and search should continue to next pages. Please test and report back.

[quotuva]

That should not be possible.

Oh, OK. I assumed so because I did not switch branches after cloning.

I think I fixed both the problems

Yes, great! Things work well now. One additional minor issue is that cancelling an isearch started from a point at the middle of a page returns it to the top of the page.
I'll be using it as my daily driver and will report if anything else comes up.

[aikrahguzar]

One additional minor issue is that cancelling an isearch started from a point at the middle of a page returns it to the top of the page.

Is this only with this branch or also with the current released version of pdf-tools? I don't see anything that can cause such a change at first glance but I will look in more detail later.

I'll be using it as my daily driver and will report if anything else comes up.

Thanks!

[quotuva]

Is this only with this branch or also with the current released version

Hmm, I guess it's nothing new.

Another bug I encountered: Open the pdf; activate the minor mode; scroll down using SPC so that the 1st page is off the screen; Then press C-v:

Debugger entered--Lisp error: (error "Invalid image specification: (space :width 25 :height 1000)")
  signal(error ("Invalid image specification: (space :width 25 :height 1000)"))
  error("Invalid image specification: %s" (space :width 25 :height 1000))
  image-display-size((space :width 25 :height 1000) t)
  image-next-line(56)
  image-scroll-up(nil)
  funcall-interactively(image-scroll-up nil)
  call-interactively(image-scroll-up nil nil)
  command-execute(image-scroll-up)

[aikrahguzar]

Debugger entered--Lisp error: (error "Invalid image specification: (space :width 25 :height 1000)")
  signal(error ("Invalid image specification: (space :width 25 :height 1000)"))
  error("Invalid image specification: %s" (space :width 25 :height 1000))
  image-display-size((space :width 25 :height 1000) t)
  image-next-line(56)
  image-scroll-up(nil)
  funcall-interactively(image-scroll-up nil)
  call-interactively(image-scroll-up nil nil)
  command-execute(image-scroll-up)

That is an image-mode command that fails because image-mode expects an image to be at point-min which isn't unless the first page is being displayed. I fixed this using an advice and the commands shouldn't error out now. However vertical scrolling using those commands will not be intuitive since the scrolling will stop at the beginning/end of the page. However they can be useful for that reason too and I think this matches the behavior with the minor mode off.

[aikrahguzar]

I have gone ahead and combined the content of image-roll.el file into the pdf-roll.el. So people can now test this branch without installing any other package.

Everything is working for me on initial testing and hopefully I will for others too.

[roshanshariff]

Hi @aikrahguzar, thanks for your amazing work on this feature! I've been testing your branch on emacs 28 on Linux and emacs 29 on macOS, and things are looking good so far. The only issue I've encountered is with scrolling.

On Linux with emacs 28, the mouse wheel scrolling seems to be very slow, perhaps moving only one pixel at a time. It's very smooth, but it takes a lot of spinning the mouse wheel to scroll an appreciable amount. Scrolling using the keyboard arrow keys works fine.

On macOS with emacs 29, the scrolling is fine with pixel-scroll-precision-mode disabled, but when it is enabled, the mouse wheel or trackpad scrolling doesn't do anything in PDF buffers. Again, scrolling with the keyboard arrow keys works fine.

Let me know how if I can help debug.

EDIT: With pdf-view-roll-minor-mode disabled, the scrolling is fine on Linux, but behaves exactly the same on macOS. So the latter case probably has nothing to do with your changes.

[aikrahguzar]

On Linux with emacs 28, the mouse wheel scrolling seems to be very slow, perhaps moving only one pixel at a time. It's very smooth, but it takes a lot of spinning the mouse wheel to scroll an appreciable amount. Scrolling using the keyboard arrow keys works fine.

I am on Emacs 29 and linux and scrolling works for me so probably generation of mouse events changed in some way. Can you scroll for roughly the same time on Emacs 28 and Emacs 29 and compare with shows up in view-lossage. The mouse-wheel code is pretty simple,

(defun pdf-roll-scroll-mouse-wheel (event)
  "Scroll according to mouse wheel EVENT."
  (interactive "e")
  (with-selected-window (posn-window (event-start event))
    (pcase (event-basic-type event)
      ('wheel-down (pdf-roll-scroll-forward))
      ('wheel-up (pdf-roll-scroll-backward))
      (_ (error "Event must be wheel down or wheel up event")))))

so slow scrolling probably means that somehow Emacs 28 is generating less wheel-up/down events.

On macOS with emacs 29, the scrolling is fine with pixel-scroll-precision-mode disabled, but when it is enabled, the mouse wheel or trackpad scrolling doesn't do anything in PDF buffers. Again, scrolling with the keyboard arrow keys works fine.

Let me know how if I can help debug.

EDIT: With pdf-view-roll-minor-mode disabled, the scrolling is fine on Linux, but behaves exactly the same on macOS. So the latter case probably has nothing to do with your changes.

pixel-scroll-precision-mode doesn't work for pdf-view-mode and I think also for image-mode because Emacs can reset vscroll for a lot of reasons so we it is saved and actively reapplied if it differs from the saved value. So any change to vscroll must update the stored value to be effective. I think this can be worked around by updating the stored value instead of resetting it if this-command is mwheel-scroll.

Edit: I tried the idea above (replacing wheel-scroll by pixel-scroll-precision since that is the command used) and it sort of works: scrolling goes smoothly as long as page doesn't need to change. But when it need to change, we end up with the beginning of current page and I can't see how to solve that.

[aikrahguzar]

On Linux with emacs 28, the mouse wheel scrolling seems to be very slow, perhaps moving only one pixel at a time. It's very smooth, but it takes a lot of spinning the mouse wheel to scroll an appreciable amount. Scrolling using the keyboard arrow keys works fine.

Actually, I think removing bindings for <wheel-up> and <wheel-down> from pdf-view-roll-minor-mode-map should fix this e.g.

(keymap-set pdf-view-roll-minor-mode-map "<wheel-up>" nil)
(keymap-set pdf-view-roll-minor-mode-map "<wheel-down>" nil)

(I think this particular snippet needs compat on Emacs 28).

Let know if it works.

[roshanshariff]

Thank @aikrahguzar for your help! Unfortunately, removing the bindings didn't help, and I see that you've removed all the scrolling functionality anyway in your latest commits. After a little investigation, I think the problem is that mwheel-scroll calls mwheel-scroll-up/down-function with an amt that is based on mouse-wheel-scroll-amount, which is usually the number of text lines to scroll. But in pdf-tools it is taken to be the number of pixels, which results in slow scrolling. (All this is on emacs 28.)

[aikrahguzar]

Thank @aikrahguzar for your help! Unfortunately, removing the bindings didn't help, and I see that you've removed all the scrolling functionality anyway in your latest commits. After a little investigation, I think the problem is that mwheel-scroll calls mwheel-scroll-up/down-function with an amt that is based on mouse-wheel-scroll-amount, which is usually the number of text lines to scroll. But in pdf-tools it is taken to be the number of pixels, which results in slow scrolling. (All this is on emacs 28.)

You are right, pdf-roll-scroll-forward/backward were incompatible with what mwheel-scroll needs. I have now changes the amount to be the number of lines which is converted to pixels using frame-char-height. This should bring scrolling in line with existing pdf-tools and image-mode behavior. Let me know if it works on Emacs 28.

I removed the scrolling functionality because I am trying to get pdf-roll to reuse existing options/customization instead of introducing more. Right now the only options left are color and size of the margins. I think those have to stay but maybe there is a better way to pick a default value for them? Especially for color there is probably a face whose background can be used.

@roshanshariff , yesterday I realized that even though I had fixed the region to appear on the correct page, it still always applied to the first page visible. I think it is correct now but if you can test annotations/pdf-view-kill-ring-save let me if they work as expected, especially when the region is not on the first page visible.

[roshanshariff]

Very nice @aikrahguzar, your recent commit with the line-to-pixel conversion has fixed mouse wheel scrolling! I can also confirm that dragging with the mouse to select a region works and displays a highlight on the correct page (if that's what you meant) and pdf-view-kill-ring-save kills the highlighted text.

[aikrahguzar]

Very nice @aikrahguzar, your recent commit with the line-to-pixel conversion has fixed mouse wheel scrolling! I can also confirm that dragging with the mouse to select a region works and displays a highlight on the correct page (if that's what you meant) and pdf-view-kill-ring-save kills the highlighted text.

Thanks for testing! Can you also test annotations e.g. pdf-annot-add-highlight-markup-annotation?

[roshanshariff]

Looks like highlight annotations are also working, even on the second visible page on the screen 👍

[aikrahguzar]

@roshanshariff I think pixel-precision-scroll-mode on Emacs 29 should work now. There is some problem with scrolling backwards if pixel-scroll-use-momentum is non-nil but I think that is actually the problem mentioned on the top of pixel-scroll-precision.

[aikrahguzar]

To people who want to have pixel-scroll-precision-mode along with pdf-view-roll-minor-mode: you probably need something like this in your config,

(add-hook 'pdf-view-roll-minor-mode-hook
          (defun +pdf-roll-adjust-precision-scroll ()
            (if pdf-view-roll-minor-mode
                (progn (kill-local-variable 'pixel-scroll-precision-mode)
                       (kill-local-variable 'mwheel-coalesce-scroll-events))
              (setq-local pixel-scroll-precision-mode nil)
              (setq-local mwheel-coalesce-scroll-events t))))

this is because stock pdf-view-mode doesn't work with pixel-scroll-precision-mode and has code to disable it.

Edit: Not necessary anymore since pdf-view-roll-minor-mode does it now.

[roshanshariff]

I can confirm pixel-scroll-precision-mode works now! And with your latest commit, no extra configuration is required.

I did find that pdf-view-roll-minor-mode doesn't work with the pdf-view slicing feature, e.g. with pdf-view-set-slice-from-bounding-box. I get an "Invalid image specification" error in pdf-roll-maybe-slice-image. But, as you said, it's not necessary to get all the features working with this PR, so maybe fixing that is not on the critical path.

[aikrahguzar]

I can confirm pixel-scroll-precision-mode works now! And with your latest commit, no extra configuration is required.

I did find that pdf-view-roll-minor-mode doesn't work with the pdf-view slicing feature, e.g. with pdf-view-set-slice-from-bounding-box. I get an "Invalid image specification" error in pdf-roll-maybe-slice-image. But, as you said, it's not necessary to get all the features working with this PR, so maybe fixing that is not on the critical path.

This used to work so it was me messing something up during recent changes and it did turn out to be trying to slice an already slice image. Should be fixed now.

In general I would like to make every feature that can reasonably to work with continuous display of pages to work. Most of the changes required are fairly straightforward: look for where pdf-view-current-page is used and then adapt the code to iterate over displayed pages. This sometimes require changing the format of some dynamical variables since we need to store the page information instead of relying on implicit state. I think all changes outside pdf-roll.el are of more or less this nature.

[quotuva]

Thanks for all the work!

Just a nit: with the latest commit, C-p no longer scrolls by one line on mine; though C-n does. Does anyone else see this?

[aikrahguzar]

Thanks for all the work!

Just a nit: with the latest commit, C-p no longer scrolls by one line on mine; though C-n does. Does anyone else see this?

I missed changing the interactive spec before and because of evil I wasn't calling pdf-view-roll-scroll-backward interactively so didn't notice. Should be fixed now.

[roshanshariff]

Hi @aikrahguzar, just a head's up: your last commit (I'm guessing accidentally) moved the autoload cookie from before the definition of pdf-view-roll-minor-mode to pdf-roll-initialize. This prevents the minor mode from being enabled if you add it to pdf-view-mode-hook without loading pdf-roll first.

[aikrahguzar]

Hi @aikrahguzar, just a head's up: your last commit (I'm guessing accidentally) moved the autoload cookie from before the definition of pdf-view-roll-minor-mode to pdf-roll-initialize. This prevents the minor mode from being enabled if you add it to pdf-view-mode-hook without loading pdf-roll first.

Thanks @roshanshariff , should be fixed now.

[aikrahguzar]

(M-x server-start if necessary) Switch to your branch: C-x p v, b s aikrahguzar/upstream-pdf-roll RET

Then (I used M-& for these)

git rebase master RET
git rebase --skip # whenever a conflict arose in README.org
...
git add image-roll.el # on the first such conflict
EDITOR=emacsclient git rebase --continue  # use the prefilled commit message
git rebase --skip # more conflicts in README.org
...
git rm image-roll.el # on a second such conflict
EDITOR=emacsclient git rebase --continue

And it was done.

Thanks a lot! I did that with the help of magit. Seems to have worked!

[brongulus]

Didn't realise it's been a year! Just wanted to ask if the PR is still a WIP or is awaiting review from the package maintainer? Thanks!

[aikrahguzar]

Didn't realise it's been a year! Just wanted to ask if the PR is still a WIP or is awaiting review from the package maintainer? Thanks!

Awaiting a review.