[PATCH] Add user stylesheet support for doc-view EPUB support

[PATCH] Add user stylesheet support for doc-view EPUB support

[Kjartan Oli Agustsson]

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

With the addition of EPUB support to doc-view I took a shot at
adding support for mutool's user stylesheet feature, which allows the
user to control the layout of the EPUB with custom CSS rules.

I'm attaching the patch here if anyone is interested.  This is my first
attempt at contributing to Emacs, so I'm sure there is something I got
wrong/did sub-optimally.  Any criticism/suggestions for improvement are
welcome.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: User Stylesheet support --]
[-- Type: text/x-patch, Size: 1634 bytes --]

From f1f919d64d97a0f8bd614ef7b099ca123b66c4d6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kjartan=20=C3=93li=20=C3=81g=C3=BAstsson?=
 <kjartanoli@outlook.org>
Date: Fri, 4 Mar 2022 23:01:19 +0000
Subject: [PATCH] Add user stylesheet option for doc-view EPUB support

* lisp/doc-view.el (doc-view-start-process): Add user stylesheet to
process arguments when appropriate.
---
 lisp/doc-view.el | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lisp/doc-view.el b/lisp/doc-view.el
index 193cf42ea4..8658ed013f 100644
--- a/lisp/doc-view.el
+++ b/lisp/doc-view.el
@@ -226,6 +226,12 @@ doc-view-resolution
 Higher values result in larger images."
   :type 'number)
 
+(defcustom doc-view-mutool-user-stylesheet nil
+  "User stylesheet to use when converting EPUB documents to PDF."
+  :type '(choice (file :must-match t)
+                 (const nil))
+  :version "29.1")
+
 (defvar doc-view-doc-type nil
   "The type of document in the current buffer.
 Can be `dvi', `pdf', `ps', `djvu', `odf', 'epub', `cbz', `fb2',
@@ -1079,6 +1085,9 @@ doc-view-start-process
   (let* ((default-directory (or (unhandled-file-name-directory
                                  default-directory)
 			        (expand-file-name "~/")))
+        (args (if (and (eq doc-view-doc-type 'epub)
+                        doc-view-mutool-user-stylesheet)
+                  (cons (car args) (cons (format "-U%s" (expand-file-name doc-view-mutool-user-stylesheet)) (cdr args)))))
          (proc (apply #'start-process name doc-view-conversion-buffer
                       program args)))
     (push proc doc-view--current-converter-processes)
-- 
2.35.1


[-- Attachment #3: Type: text/plain, Size: 35 bytes --]


-- 
Kjartan Óli Ágústsson

Re: [PATCH] Add user stylesheet support for doc-view EPUB support

[Lars Ingebrigtsen]

Kjartan Oli Agustsson <kjartanoli@outlook.com> writes:

> @@ -1079,6 +1085,9 @@ doc-view-start-process

Adding support for CSS makes sense to me.  But is this the best place to
add it?  It's the general function that's called for all the different
conversions?

>    (let* ((default-directory (or (unhandled-file-name-directory
>                                   default-directory)
>  			        (expand-file-name "~/")))
> +        (args (if (and (eq doc-view-doc-type 'epub)
> +                        doc-view-mutool-user-stylesheet)
> +                  (cons (car args) (cons (format "-U%s" (expand-file-name doc-view-mutool-user-stylesheet)) (cdr args)))))

And I don't think this can be right -- for instance, it's called here:

    (doc-view-start-process "dvi->pdf" doc-view-dvipdfm-program
			    (list "-o" pdf dvi)
			    callback)))

And you're splicing in the stylesheet after "-o" (potentially -- I'm not
that familiar with the doc-view code), which will break things.  (And if
doc-view-mutool-user-stylesheet is nil, then args will also be bound to
nil, which will make all the calls fail.)

I've added Daniel to the CCs; perhaps he has further comments.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Add user stylesheet support for doc-view EPUB support

[Kjartan Oli Agustsson]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Kjartan Oli Agustsson <kjartanoli@outlook.com> writes:
>
>> @@ -1079,6 +1085,9 @@ doc-view-start-process
>
> Adding support for CSS makes sense to me.  But is this the best place to
> add it?  It's the general function that's called for all the different
> conversions?

Probably not, I've looked through doc-view.el again while trying to
address your other points, and I'm now fairly certain that a better
place would be `doc-view-pdf->png-converter-mupdf', which seems to
handle the case for `doc-view-epub-font-size' already.

I'll see if I can't handle this there, and submit a new patch once I get
that to work.
-- 
Kjartan Óli Ágústsson

Re: [PATCH] Add user stylesheet support for doc-view EPUB support

[Kjartan Oli Agustsson]

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


Kjartan Oli Agustsson <kjartanoli@outlook.com> writes:

> I'll see if I can't handle this there, and submit a new patch once I get
> that to work.

This should take care of that.  But now there's something else I'm
wondering about.  If I'm understanding
`doc-view-custom-set-epub-font-size' correctly, it is used as the setter
for `doc-view-epub-font-size' to regenerate epub documents when it is
changed.  Am I understanding this correctly, and if so would it make
sense to do the same for the user stylesheet?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Improved patch --]
[-- Type: text/x-patch, Size: 1885 bytes --]

From eae132bde6d25a7fa1125bb47c1971c93d4bc300 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kjartan=20=C3=93li=20=C3=81g=C3=BAstsson?=
 <kjartanoli@outlook.com>
Date: Mon, 7 Mar 2022 00:48:32 +0000
Subject: [PATCH] Add user stylesheet option for doc-view EPUB support

* lisp/doc-view.el (doc-view-start-process): Add user stylesheet to
process arguments when appropriate.
---
 lisp/doc-view.el | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lisp/doc-view.el b/lisp/doc-view.el
index 193cf42ea4..1ea3443667 100644
--- a/lisp/doc-view.el
+++ b/lisp/doc-view.el
@@ -226,6 +226,12 @@ doc-view-resolution
 Higher values result in larger images."
   :type 'number)
 
+(defcustom doc-view-mutool-user-stylesheet nil
+  "User stylesheet to use when converting EPUB documents to PDF."
+  :type '(choice (const nil)
+                 (file :must-match t))
+  :version "29.1")
+
 (defvar doc-view-doc-type nil
   "The type of document in the current buffer.
 Can be `dvi', `pdf', `ps', `djvu', `odf', 'epub', `cbz', `fb2',
@@ -1169,8 +1175,11 @@ doc-view-pdf->png-converter-mupdf
          (options `(,(concat "-o" png)
                     ,(format "-r%d" (round doc-view-resolution))
                     ,@(if pdf-passwd `("-p" ,pdf-passwd)))))
-    (when (and (eq doc-view-doc-type 'epub) doc-view-epub-font-size)
-      (setq options (append options (list (format "-S%s" doc-view-epub-font-size)))))
+    (when (eq doc-view-doc-type 'epub)
+      (when doc-view-epub-font-size
+        (setq options (append options (list (format "-S%s" doc-view-epub-font-size)))))
+      (when doc-view-mutool-user-stylesheet
+        (setq options (append options (list (format "-U%s" (expand-file-name doc-view-mutool-user-stylesheet)))))))
     (doc-view-start-process
      "pdf->png" doc-view-pdfdraw-program
      `(,@(doc-view-pdfdraw-program-subcommand)
-- 
2.35.1


[-- Attachment #3: Type: text/plain, Size: 35 bytes --]


-- 
Kjartan Óli Ágústsson

Re: [PATCH] Add user stylesheet support for doc-view EPUB support

[Lars Ingebrigtsen]

Kjartan Oli Agustsson <kjartanoli@outlook.com> writes:

> This should take care of that.

Thanks; applied to Emacs 29.

This change was small enough to apply without assigning copyright to the
FSF, but for future patches you want to submit, it might make sense to
get the paperwork started now, so that subsequent patches can be applied
speedily. Would you be willing to sign such paperwork?

> But now there's something else I'm wondering about.  If I'm
> understanding `doc-view-custom-set-epub-font-size' correctly, it is
> used as the setter for `doc-view-epub-font-size' to regenerate epub
> documents when it is changed.  Am I understanding this correctly, and
> if so would it make sense to do the same for the user stylesheet?

When the contents of the user stylesheet changes?  I guess that would
make sense, and you could use `file-has-changed-p' to implement that.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Add user stylesheet support for doc-view EPUB support

[Kjartan Oli Agustsson]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Kjartan Oli Agustsson <kjartanoli@outlook.com> writes:
>
>> This should take care of that.
>
> Thanks; applied to Emacs 29.
>
> This change was small enough to apply without assigning copyright to the
> FSF, but for future patches you want to submit, it might make sense to
> get the paperwork started now, so that subsequent patches can be applied
> speedily. Would you be willing to sign such paperwork?

I have already started the process.  I'm just waiting for a response
from assign@gnu.org.

>> But now there's something else I'm wondering about.  If I'm
>> understanding `doc-view-custom-set-epub-font-size' correctly, it is
>> used as the setter for `doc-view-epub-font-size' to regenerate epub
>> documents when it is changed.  Am I understanding this correctly, and
>> if so would it make sense to do the same for the user stylesheet?
>
> When the contents of the user stylesheet changes?  I guess that would
> make sense, and you could use `file-has-changed-p' to implement that.

That would make sense to.  I was also thinking if the value of the
variable is changed to point to a different file.

-- 
Kjartan Óli Ágústsson

Re: [PATCH] Add user stylesheet support for doc-view EPUB support

[Lars Ingebrigtsen]

Kjartan Oli Agustsson <kjartanoli@outlook.com> writes:

> That would make sense to.  I was also thinking if the value of the
> variable is changed to point to a different file.

Ah, I see.  Yes, that'd make sense.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Add user stylesheet support for doc-view EPUB support

[Kjartan Oli Agustsson]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Kjartan Oli Agustsson <kjartanoli@outlook.com> writes:
>
>> That would make sense to.  I was also thinking if the value of the
>> variable is changed to point to a different file.
>
> Ah, I see.  Yes, that'd make sense.

I'll probably mess around with implementing that while I wait for the
copyright paperwork.  But I've started thinking: is
`doc-view-mutool-user-stylesheet' a good name, or would
`doc-view-epub-user-stylesheet' or something else mentioning EPUB
instead of mutool be better?
-- 
Kjartan Óli Ágústsson

Re: [PATCH] Add user stylesheet support for doc-view EPUB support

[Lars Ingebrigtsen]

Kjartan Oli Agustsson <kjartanoli@outlook.com> writes:

> I'll probably mess around with implementing that while I wait for the
> copyright paperwork.  But I've started thinking: is
> `doc-view-mutool-user-stylesheet' a good name, or would
> `doc-view-epub-user-stylesheet' or something else mentioning EPUB
> instead of mutool be better?

`doc-view-epub-user-stylesheet' would probably be better, I think.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Add user stylesheet support for doc-view EPUB support

[Kjartan Oli Agustsson]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> `doc-view-epub-user-stylesheet' would probably be better, I think.

Then I'll change that in my next patch.  Speaking of my next patch, how
long does it usually take to get a reply from assign@gnu.org?

-- 
Kjartan Óli Ágústsson

Re: [PATCH] Add user stylesheet support for doc-view EPUB support

[Lars Ingebrigtsen]

Kjartan Oli Agustsson <kjartanoli@outlook.com> writes:

> Then I'll change that in my next patch.  Speaking of my next patch, how
> long does it usually take to get a reply from assign@gnu.org?

A week or two.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Add user stylesheet support for doc-view EPUB support

[Kjartan Oli Agustsson]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> A week or two.

Ok, then I'll wait a little longer before starting to think I did some
part of process wrong.

-- 
Kjartan Óli Ágústsson