search

dalanicolai / pdf-tools

Emacs support library for PDF files.
GNU General Public License v3.0
4 stars 8 forks source link

pdf-view-next-line-or-next-page does not respect ARG when pdf-view-roll-minor-mode is enabled #2

Closed yantar92 closed 2 years ago

yantar92 commented 2 years ago

The current implementations of pdf-view-next-line-or-next-page and pdf-view-previous-line-or-previous-page ignore the optional argument when using pdf-view-roll-minor-mode. This appears to be intentional, but leaves little user control over granularity of scrolling:

(defun pdf-view-previous-line-or-previous-page (&optional arg)
  (interactive "p")
  (if pdf-view-roll-minor-mode
      (image-roll-scroll-backward)
    (pdf-view--previous-line-or-previous-page arg)))
dalanicolai commented 2 years ago

It is handy to know what functionality users like to see indeed. For setting the 'granularity' there is the customizable variable image-roll-step-size. I guess the documentation still needs some work.

Thanks for pointing these things out

yantar92 commented 2 years ago

It is handy to know what functionality users like to see indeed. For setting the 'granularity' there is the customizable variable image-roll-step-size. I guess the documentation still needs some work.

Note that I did notice `image-roll-step-size' and adjusted my personal config accordingly. However, it is generally not a good idea to break existing functions when you are implementing new features.

Ideally, your pdf-roll-minor-mode should do nothing but add the continuous scrolling (+possibly other derived features). All the existing user customisations should not be affected or just affected to the least possible extent.

dalanicolai commented 2 years ago

As far as I know I did not break any function(ality), just when the 'image-roll' is active, then I could not use the original pdf-view-next-line-or-next-page function. As I did/do not find passing the argument to the image-roll version of that function very useful, I did not find it necessary to implement it (after all, there is image-roll-step-size). However, I have implemented some quickfix that makes the arg get passed also to the image-roll version now.

Rewriting things to make the image-roll version scroll by line or by almost full screen etc. as provided by pdf-tools, is not very difficult, but it has no priority for me at the moment.

yantar92 commented 2 years ago

As far as I know I did not break any function(ality), just when the 'image-roll' is active, then I could not use the original pdf-view-next-line-or-next-page function.

That's what I consider broken functionality. pdf-view-roll-minor-mode says the following in its docstring:

If enabled display document on a virtual scroll providing continuous scrolling.

After reading the docstring, I expect to enable pdf-view-roll-minor-mode and get continuous scrolling. Everything else should work as usual. Which is not the case now.

As I did/do not find passing the argument to the image-roll version of that function very useful, I did not find it necessary to implement it (after all, there is image-roll-step-size). However, I have implemented some quickfix that makes the arg get passed also to the image-roll version now.

Thanks!

Rewriting things to make the image-roll version scroll by line or by almost full screen etc. as provided by pdf-tools, is not very difficult, but it has no priority for me at the moment.

Note that I do not demand you to close this issue right know or even soon. It was not hard for me do adapt my config. However, I do expect such things to be fixed in the release version. Of course, only after you take care about more important things.