search

syl20bnr / spacemacs

A community-driven Emacs distribution - The best editor is neither Emacs nor Vim, it's Emacs *and* Vim!
http://spacemacs.org
GNU General Public License v3.0
23.57k stars 4.9k forks source link

don't require pdf-sync during config #16311

Closed dankessler closed 3 months ago

dankessler commented 3 months ago

Requiring pdf-sync has some unfortunate side effects that seem to break pdf-tools. In brief, it loads something that we otherwise wanted to autoload.

In more detail: when we manually require pdf-sync, that in turn requires pdf-view, which causes pdf-view.el to be loaded and pdf-view-mode to be defined. However, use-package had configured pdf-view-mode to be autoloaded (by its specification of :mode in use-package). Loading it manually doesn't trigger the autoload, so all of the configuration that is waiting to happen in pdf-tools's use-package :config block doesn't get triggered. Most critically, (pdf-tools-install) doesn't get triggered, which prevents pdf-view-mode from functioning properly.

If we don't require pdf-sync, then we end up setting a variable (pdf-sync-forward-display-action) which isn't yet bound. Fortunately, pdf-tools appropriately defines this variable using defcustom, so it won't override whatever we set it to here :)

bcc32 commented 3 months ago

I don't object to this change, but I think there is still an issue if pdf-view is directly loaded by some package without loading pdf-tools. I think we should instead fix the loading (#16315).

dankessler commented 3 months ago

I don't object to this change, but I think there is still an issue if pdf-view is directly loaded by some package without loading pdf-tools. I think we should instead fix the loading (#16315).

I have no objection to this proposal. I've amended the PR accordingly to use pdf-loader-install during :init and deleted the deferred pdf-tools-install that was in the :config block.

smile13241324 commented 3 months ago

I think we have a conflict now would you mind checking whether this PR is still needed @bcc32 @dankessler?

bcc32 commented 3 months ago

I think just the original change of removing the (require 'pdf-sync) line is good, may as well load less eagerly if we can. (the other one is redundant with #16315 which has already been merged).

smile13241324 commented 3 months ago

@dankessler can you do the proposed changes? Then we are ready for merge.

dankessler commented 3 months ago

@smile13241324 Done; sorry for the confusion!

When @bcc32 chimed in I thought they were suggesting I modify my PR but I now realize they were cross-referencing their already submitted (but related) PR, which is now merged.