> (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 . 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 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 > > > > * lisp/doc-view.el (): . > (): . > ... > > * 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 > >