Forgot to provide the link to the (info about the) pymupdf server, in case someone is interested... https://github.com/vedang/pdf-tools/pull/61 On Tue, 11 Jan 2022 at 10:59, dalanicolai wrote: > > (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 >> >>