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

[dalanicolai <dalanicolai@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9632 bytes --]

>  (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 ;-)


This is also due to the auto-indent, but I will correct it manually.

>  (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).
>

This I copied from pdf-tools. However, I am planning to enter this
documentation
myself, while I am implementing real continuous scroll. I have got that
working in
pdf-tools already, but I had to disable all kinds of minor modes there. So
I thought
let's start with a simpler task of implementing it in doc-view, however,
the same
difficulty, that I encountered also in pdf-tools is that somehow
`bookroll-mode-winprops-alist` (which is exactly the equivalent/copy of
`bookroll-mode-winprops-alist`) obtains a value `t`. I think I've spent
some hours
already investigating where it obtains it (somehow I fixed it after
debugging for
pdf-tools for few hours, but clearly I do not understand how, and have to
repeat the
same things again, well I guess it is a good exercise. Anyway, in the end I
hope to
add some useful documentation to prevent other hackers from 'wasting' their
time :)
Anyway, if I can keep it like this for now, that would be great, as the
TODO really
jumps out font lock wise, and also I can search for it.

> @@ -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.
>

You mean because it aligns with the line below so that it looks like it
belongs
to that one? Because otherwise, I think it does not look bad.

> +     ((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
>

Ah yes, I was aware of that, but indeed I did that consciously. I have very
good experiences
with (py)mupdf (I have implemented an alternative pdf-tools server using
it, and indeed it is
much faster than the epdfinfo server, well at least the mupdf vs poppler
parts.

really take a very quick look at the two histograms in this link
<https://hub.alfresco.com/t5/alfresco-content-services-blog/pdf-rendering-engine-performance-and-fidelity-comparison/ba-p/287618>
.
So I decided this behavior really wouldn't be a problem, why not prefer
mutool when it is
available?

Also, I think the rest of the code will still launch
> `doc-view-pdfdraw-program` rather than `mutool`, so there's
> something fishy.
>

That I do not understand immediately, for me the behavior is still somewhat
opaque. But I
will be trying to understand things further today (anyway for implementing
the continuous scroll)
and try to understand how it all works (and correct it here).

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.
>

That sounds good indeed. I will take care of that.

Thanks, for the feedback... I will try another 'round' probably sooner than
later :)





On Tue, 11 Jan 2022 at 03:48, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > 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
>
>

[-- Attachment #2: Type: text/html, Size: 12943 bytes --]