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

Some work on this package #19

Open
aikrahguzar opened this issue Jun 25, 2023 · 2 comments
Open

Some work on this package #19

aikrahguzar opened this issue Jun 25, 2023 · 2 comments

Comments

@aikrahguzar
Copy link

aikrahguzar commented Jun 25, 2023

Hi,
This has been a great package to use with pdf-tools. I was running into issues mostly with grey boxes appearing but also with some functionality in pdf-tools not working. So I have done quite a bit of work on this package and also your fork of pdf-tools to resolve those issues. The current state can be seen on my fork of image-roll and the continuous-scroll branch of my fork of pdf-tools. The image-roll fork is by now very close to a rewrite, but I wanted to ask if you wanted to upstream the changes here, in any case you might be able to use (or might already have done so) some of the ideas for your other projects which I haven't tried because I like pdf-tools too much.

First the things that pdf-tools feature that work at least for me:

  1. Tooltip arrows now appear.
  2. pdf-sync works both forward and backwards.
  3. pdf-links-action-perform now shows links from all visible pages. I can't find a way to precompute labels so this comes at the expense of not using the cache. This might lead to bad performance on older machines but it is good enough for me.
  4. Enlarging/shrinking pages work smoothly. Previously it lead to grey boxes some time.
  5. Same with scrolling and window resizing. Scrolling also works for all page sizes.

Things that don't work:

  1. Text selection: it work on the first page but will cause jumps it tried on subsequent pages. I can't figure out how it works so I can't figure out what to change. Text selection now works on any displayed page. The only limitation is that it is not possible to have a selection that spans multiple pages.
  2. I-search: I might have broken it since you made some changes and it doesn't work anymore. I-search and pdf-occur should both work now. I am encountering this issue which I think is upstream but other than that everything seems fine.
  3. Probaly many more features I don't regularly use. However fixing an issue seems to require the same basic drill that can seen in the code in many places by now.

Now for the changes in how image-roll works,

  1. Page sizes are not computed at the start. Rather a page is displayed and using a combination of pos-visible-in-window-p and line-pixel-height it is determined whether there is till space to display more pages.

  2. The use of goto-char and point movement commands can cause grey boxes. It is better to use set-window-point and set-window-start with no-force argument non-nil.

  3. Margins are drawn separately from pages using overlays. This is so that we can leave the point on the position holding these overlays so Emacs doesn't recenter window during redisplay. Since, Emacs doesn't allow making line-height thinner than the height of the default dace so to allow for thin margins, the default face is locally remapped to one which has a font attribute with size 1. The point is left on the first such margin if it is visible.

  4. Vscroll is clamped between 1 and height of the current page minus 1. This makes sure window-start doesn't change when shrinking/enlarging and changing window size.

  5. Most of this work is done in a pre-redisplay function. This might be controversial but makes it easier to react to changes in window-size, page-size etc and I don't think this will cause more resource usage than window-configuration-change-hook.

I am interested in hearing on what people using this package think of the changes. If someone is motivated I can most of the changes in the pdf-tools branch can be packaged into advices that are applied when pdf-roll is loaded and which do differet things depnding on the value of pdf-view-roll-minor-mode. This will make it easier for people to try the package without switching from the upstream pdf-tools. There will be quite a few advices but I think it is manageable. Searching my branch I see about 30 uses of image-roll or pdf-view-roll in the pdf-tools in files other than pdf-roll. From this I would guess that we make do with less than 15 advices, which is not great but also not terrible.

@dalanicolai
Copy link
Owner

This sounds great! Thanks a lot for your work on this package, and sorry for the very late reply. Unfortunately, I am currently very busy, and I don't have any time to work on this package. There is a request for merging the pdf-roll branch into pdf-tools here. I will not be able to work on it, but if you are interested, then maybe you can work on it. Feel free to start from the pdf-roll branch (i.e. from your 'rewrite'), and announce your improved version on e.g. Reddit. I will at some point definitely take a look at your changes to the code. Thanks for the suggestions and the explanations. B.t.w, I have written an improved version (I hope) of image-roll here, in doc-scroll.el, it uses overlays-in instead of calculating the position of the overlays with lisp. Good luck!

@aikrahguzar
Copy link
Author

This sounds great! Thanks a lot for your work on this package, and sorry for the very late reply. Unfortunately, I am currently very busy, and I don't have any time to work on this package. There is a request for merging the pdf-roll branch into pdf-tools here. I will not be able to work on it, but if you are interested, then maybe you can work on it. Feel free to start from the pdf-roll branch (i.e. from your 'rewrite'), and announce your improved version on e.g. Reddit. I will at some point definitely take a look at your changes to the code. Thanks for the suggestions and the explanations. B.t.w, I have written an improved version (I hope) of image-roll here, in doc-scroll.el, it uses overlays-in instead of calculating the position of the overlays with lisp. Good luck!

I can put sometime into getting this merged in near but not too near future. Hopefully this will pan out 🤞

I announced this on emacs-tangents here https://lists.gnu.org/archive/html/emacs-tangents/2023-06/msg00005.html but other than that I haven't done anything. Announcement on reddit would have been nice but I don't want to create an account there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants