Re: [PATCH] add epub support to doc-view

[Stefan Monnier <monnier@iro.umontreal.ca>]

> 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