Don't assume that bookmark jump will display the buffer

[aikrahguzar]

The context for this change is that I use a workspace manager that saves the workspaces to a bookmark. When it is restoring the buffers, it calls bookmark-jump with a display-func argument which is just,

(lambda (buf) (setq buffer buf))

i.e. the buffer is not actually displayed. I think this is the right behavior, but in case of bookmarks for pdf buffers, this results in errors.

This change moves the hook that needs to be run from bookmark-after-jump-hook to a local hook in window-buffer-change-functions. This ensures that a window is always available when the code is run.

I think this is also better when restoring a large number of bookmarks since most of the work is deferred to when the user actually displays the buffer. Restoring a previous session is the case when this scenario occurs.

From what I can tell window-buffer-change-functions were introduced in Emacs 27 while pdf-tools supports Emacs 26. So I will convert this pr into a draft for now. Let me know if are willing to accept this change and I can try change this pr into which chooses the hook based on Emacs version.

[vedang]

Yes, I'm willing to accept this change as long as it handles Emacs 26 correctly.

Thank you for your work! Once you move the PR out of draft, I will test it and get back with feedback if any.

[aikrahguzar]

Yes, I'm willing to accept this change as long as it handles Emacs 26 correctly.

Thank you for your work! Once you move the PR out of draft, I will test it and get back with feedback if any.

Thanks a lot! I have adapted the code to fallback to bookmark-after-jump-hook if window-buffer-change-functions aren't available. It works for me and I think it is functionally equivalent for Emacs 26 but I can't test it and this isn't likely to be caught by CI. So hopefully someone with Emacs 26 can confirm that.

[vedang]

I worked with this code a little, and my feedback is that this change needs to be hidden behind a flag which can maintain the current behaviour, to avoid surprising current users.

I will do this over the weekend if it is easy to do. If I face a problem, I will let you know the exact issue and ask that you please debug / fix it so that I can merge this in.

[aikrahguzar]

I worked with this code a little, and my feedback is that this change needs to be hidden behind a flag which can maintain the current behaviour, to avoid surprising current users.

Thanks! I did surprise myself soon after submitting this pr but I fixed the problems and I think by now the code should just work. Since I have been messing with pdf-tools in other ways I made those changes on an another branch and forgot to add them on this one.

But basically they way code works now is that bookmark-after-jump-hook is used if the buffer is already displayed in a window. This was a serious bug in my original code which meant using bookmarks worked only if there was buffer was not displayed.

If buffer is not displayed and Emacs is recent enough window-buffer-change-functions is used. I think should not cause any surprises since normally restoring bookmark displays the buffer and the added hook will run immediately. The only thing this changes is that an error is not raised when the buffer is displayed. If you want to put it behind a flag, I think the only change need is to add the flag to the clauses of and in binding for hook in pdf-view-bookmark-jump-handler.

I sometimes have a quite a few pdf buffers open and sometimes don't switch to a buffer for the duration of a whole session. Since the buffer has never been displayed and all pdf buffers are automatically bookmarked when I close Emacs this resulted in the bookmark pointing to the beginning of the buffer. So I added a variable pdf-view--bookmark-to-restore which holds the bookmark. This variable is set to nil once the bookmark has been restored. If it is non-nil, it means the bookmark was never restored although it was meant to be so pdf-view-bookmark-make-record uses it directly.

Since making these changes a few months ago everything has been working smoothly for me. If you can think of other potential surprises I think it would be better to deal with them even if you decide to add the flag and it is changed to be non-nil.

I will do this over the weekend if it is easy to do. If I face a problem, I will let you know the exact issue and ask that you please debug / fix it so that I can merge this in.