[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] add epub support to doc-view
From: |
Stefan Monnier |
Subject: |
Re: [PATCH] add epub support to doc-view |
Date: |
Mon, 10 Jan 2022 21:48:16 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) |
> Here is a patch to add epub support to doc-view, via the mutool
> command of the mupdf library,I guess the patch needs no further
> explanation. I've read the info about sending patches, however,
> I don't understand the section about the changelog (it mentions that
> there is a changelog or something, but it doesn't say where). Also,
> the info tells us to write the commit log entries, but it does not say
> where to do this.Looking at the changelog files, I guess that info is
> just really outdated (am I right?). Anyway, the patch is in the
> attachment. If there is some part of the 'protocol' that I did not
> understand then please inform me about it.
Looks pretty good, see below for some comments/questions.
Regarding the changelog, we just need you to give us something like
doc-view.el: Add support for EPub
<Some explanation for the general idea>
* lisp/doc-view.el (<fun1>): <Do this>.
(<var2>): <Do that>.
...
* lisp/files.el (auto-mode-alist): Extend doc-view-mode-maybe
regexp to match `.epub` files.
> (define-obsolete-variable-alias 'doc-view-unoconv-program
> - 'doc-view-odf->pdf-converter-program
> - "24.4")
> + 'doc-view-odf->pdf-converter-program
> + "24.4")
Thanks. At this point, the last two args can be placed on the same
line ;-)
> (defun doc-view-new-window-function (winprops)
> + ;; TODO: write documentation!
> ;; (message "New window %s for buf %s" (car winprops) (current-buffer))
> (cl-assert (or (eq t (car winprops))
> (eq (window-buffer (car winprops)) (current-buffer))))
I plead guilty, but if you give me a more specific question, I'll find
it easier to answer it (even in the form of a docstring or comment).
> @@ -738,7 +740,7 @@ doc-view-kill-proc
> (interactive)
> (while (consp doc-view--current-converter-processes)
> (ignore-errors ;; Some entries might not be processes, and maybe
> - ;; some are dead already?
> + ;; some are dead already?
> (kill-process (pop doc-view--current-converter-processes))))
> (when doc-view--current-timer
> (cancel-timer doc-view--current-timer)
Here the auto-indentation gets it wrong, sadly.
> @@ -810,11 +812,12 @@ doc-view-mode-p
> (executable-find doc-view-dvipdf-program))
> (and doc-view-dvipdfm-program
> (executable-find doc-view-dvipdfm-program)))))
> - ((memq type '(postscript ps eps pdf))
> - (or (and doc-view-ghostscript-program
> - (executable-find doc-view-ghostscript-program))
> - (and doc-view-pdfdraw-program
> - (executable-find doc-view-pdfdraw-program))))
> + ((memq type '(postscript ps eps pdf epub))
> + (if-let (command (and (memq type '(pdf epub)) (executable-find
> "mutool")))
> + command
> + (unless (eq type 'epub)
> + (and doc-view-ghostscript-program
> + (executable-find doc-view-ghostscript-program)))))
> ((eq type 'odf)
> (and doc-view-odf->pdf-converter-program
> (executable-find doc-view-odf->pdf-converter-program)
Hmm... for PDF files, this changes the previous behavior where we
checked for the presence of `doc-view-pdfdraw-program` rather than "mutool".
Was there a strong reason to hardcode "mutool" here?
Depending on the reason, we may be better off splitting the epub
handling into its own `cond` branch.
Also, I think the rest of the code will still launch
`doc-view-pdfdraw-program` rather than `mutool`, so there's
something fishy.
> - ((or 'pdf 'djvu)
> + ((or 'pdf 'epub 'djvu)
> + (when (eq doc-view-doc-type 'epub)
> + (unless (eq doc-view-pdf->png-converter-function
> 'doc-view-pdf->png-converter-mupdf)
> + (user-error "Viewing epub documents requires the`mutool' command
> to be available,
> +and `doc-view-pdf->png-converter-function' variable set to
> +`doc-view-pdf->png-converter-mupdf'")))
This error message needs to be shorter. It probably shouldn't talk
about `mutool`. Even with this simplification I think we're going to
have to be creative to make it short enough to fit in a single line of
80 columns. We should probably just say something like
doc-view-pdf->png-converter-function invalid for EPub
and then add in the docstring of `doc-view-pdf->png-converter-function`
a note about the fact that only `doc-view-pdf->png-converter-mupdf`
supports EPub.
The rest looks good.
Stefan
Re: [PATCH] add epub support to doc-view,
Stefan Monnier <=
Re: [PATCH] add epub support to doc-view, dalanicolai, 2022/01/11
- Re: [PATCH] add epub support to doc-view, dalanicolai, 2022/01/11
- Re: [PATCH] add epub support to doc-view, Stefan Monnier, 2022/01/11
- Re: [PATCH] add epub support to doc-view, dalanicolai, 2022/01/13
- Re: [PATCH] add epub support to doc-view, dalanicolai, 2022/01/14
- Re: [PATCH] add epub support to doc-view, dalanicolai, 2022/01/14