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

Add pdf-tools to Package-Requires #19

Closed
wants to merge 1 commit into from
Closed

Conversation

terlar
Copy link

@terlar terlar commented Apr 20, 2023

This package is directly requiring pdf-tools, but doesn't specify it as a requirement in the Package-Requires specification. I didn't verify if any older version was possible, or if newer version would be required due to usage of newer functions. This is a quite old pdf-tools which was the version around the time this package was created. Another option would be to depend on the latest pdf-tools (1.0.0), but that is only half a year old.

I hope that someone that knows more about this package can correct this if it is wrong.

Thank you.

@petermao
Copy link
Member

If you use this package with PDFs, certainly it is advantageous to use pdf-tools, but it is not required. Org-noter will still work with doc-view. Furthermore, if you read EPUB or DJVU exclusively, you would have no need for pdf-tools.

Org-noter ships with org-noter-supported-modes specifying all modes for which we have modules, and if the user does not have the supporting package(s), a "nudge" message appears. Users can pare down org-noter-supported-modes to only include the ones they use, suppressing the "nudge" message.

Perhaps in the modules we should specify the oldest version that works, but I haven't tested older versions of pdf-tools with org-noter.

Copy link
Member

@petermao petermao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments in #19 (comment)

@terlar
Copy link
Author

terlar commented Apr 22, 2023

It is currently required, as the main file requires org-noter-core:
https://github.com/org-noter/org-noter/blob/master/org-noter.el#L45

and org-noter-core requires pdf-tools:
https://github.com/org-noter/org-noter/blob/master/org-noter-core.el#L26

If it is not required, that functionality should be loaded behind the appropriate guards or use autoloads?

This was added here:
ced2751

and as someone who was using the weirdNox/org-noter as that was what was defined by MELPA until the recent migration. I just ran into this missing dependency when the new package source was switched over.

@petermao
Copy link
Member

Ah, right. This is a bit of unfinished business. I believe the only thing in org-noter-core.el that requires pdf-tools is org-noter--show-arrow. https://github.com/org-noter/org-noter/blob/master/org-noter-core.el#L1256-L1305

To do this properly (ie, in concordance with the architectural change done by @c1-g), we need to move the meat of org-noter--show-arrow into a hook in the PDF module.

@dmitrym0, let's leave this PR open until one of us makes this change.

@petermao
Copy link
Member

If PR #22 doesn't introduce any new issues, I'll close this PR when it gets merged.

@petermao
Copy link
Member

petermao commented May 5, 2023

Superseded by PR #22

@petermao petermao closed this May 5, 2023
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

Successfully merging this pull request may close these issues.

None yet

2 participants