From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Hirn Subject: Re: org-export-latex-hyperref-options-format Date: Thu, 20 Feb 2014 16:53:25 -0600 Message-ID: References: <87a9dpo0as.fsf@gmail.com> <8738jhnx1m.fsf@gmail.com> <878ut72qyo.fsf@gmail.com> <87zjlm13st.fsf@gmail.com> <8672745997102408713@unknownmsgid> <87vbw91pac.fsf@gmail.com> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=089e013d0db0804fa604f2de5f79 Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:54945) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WGcUi-0007eL-7R for emacs-orgmode@gnu.org; Thu, 20 Feb 2014 17:53:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WGcUg-0002oJ-Kz for emacs-orgmode@gnu.org; Thu, 20 Feb 2014 17:53:28 -0500 Received: from mail-oa0-x22d.google.com ([2607:f8b0:4003:c02::22d]:55852) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WGcUg-0002o9-DR for emacs-orgmode@gnu.org; Thu, 20 Feb 2014 17:53:26 -0500 Received: by mail-oa0-f45.google.com with SMTP id i11so3041395oag.32 for ; Thu, 20 Feb 2014 14:53:25 -0800 (PST) In-Reply-To: List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: Nicolas Goaziou Cc: "emacs-orgmode@gnu.org" --089e013d0db0804fa604f2de5f79 Content-Type: text/plain; charset=ISO-8859-1 With recommended changes. ******** BEGIN PATCH ********* diff --git a/ox-latex.el b/ox-latex.el index 19f055e..f6e5a09 100644 --- a/ox-latex.el +++ b/ox-latex.el @@ -103,7 +103,7 @@ (:latex-class-options "LATEX_CLASS_OPTIONS" nil nil t) (:latex-header "LATEX_HEADER" nil nil newline) (:latex-header-extra "LATEX_HEADER_EXTRA" nil nil newline) - (:latex-hyperref-p nil "texht" org-latex-with-hyperref t) + (:latex-hyperref nil nil org-latex-hyperref-template t) ;; Redefine regular options. (:date "DATE" nil "\\today" t))) @@ -341,10 +341,18 @@ the toc:nil option, not to those generated with #+TOC keyword." :group 'org-export-latex :type 'string) -(defcustom org-latex-with-hyperref t - "Toggle insertion of \\hypersetup{...} in the preamble." +(defcustom org-latex-hyperref-template "\\hypersetup{\n pdfkeywords={%k},\n pdfsubject={%d},\n pdfcreator={%c}}\n" + "Template for hyperref package options. + +Value is a format string, which can contain the following placeholders: + + %k for KEYWORDS line + %d for DESCRIPTION line + %c for CREATOR line + +An empty string disables the setup." :group 'org-export-latex - :type 'boolean) + :type 'string) ;;;; Headline @@ -1118,12 +1126,13 @@ holding export options." ;; Title (format "\\title{%s}\n" title) ;; Hyperref options. - (when (plist-get info :latex-hyperref-p) - (format "\\hypersetup{\n pdfkeywords={%s},\n pdfsubject={%s},\n pdfcreator={%s}}\n" - (or (plist-get info :keywords) "") - (or (plist-get info :description) "") - (if (not (plist-get info :with-creator)) "" - (plist-get info :creator)))) + (format-spec (plist-get info :latex-hyperref) + (format-spec-make + ?k (or (plist-get info :keywords) "") + ?d (or (plist-get info :description)"") + ?c (if (plist-get info :with-creator) + (plist-get info :creator) + ""))) ;; Document start. "\\begin{document}\n\n" ;; Title command. ******** END PATCH ********* On Thu, Feb 20, 2014 at 3:52 PM, Joe Hirn wrote: > I really appreciate your review. Sorry for the pedestrian code submission. > I don't get candid critical feedback on my e-lisp, so it's great to learn > more about its idioms and conventions. > > I'll incorporate your feedback into a new patch. > > > On Thu, Feb 20, 2014 at 2:51 PM, Nicolas Goaziou wrote: > >> Hello, >> >> Joe Hirn writes: >> >> > I was able to test this on my local machine and it seems to work as we >> > discussed. >> > >> > If there are any other changes to the patch you'd like to see, please >> let >> > me know. >> >> Thank you for the patch. Here are a few comments. >> >> > - (:latex-hyperref-p nil "texht" org-latex-with-hyperref t) >> > + (:latex-hyperref-p nil "texht" (if org-latex-hyperref-template t) t) >> >> I think we can drop the "-p" suffix since this is no longer a predicate. >> So the property can be named :latex-hyperref. >> >> Also we can replace "texht" with nil since it doesn't make much sense to >> specify a full template from the OPTIONS line. >> >> Eventually, the default value should be `org-latex-hyperref-template'. >> >> This boils down to the following line: >> >> (:latex-hyperref nil nil org-latex-hyperref-template t) >> >> > -(defcustom org-latex-with-hyperref t >> > - "Toggle insertion of \\hypersetup{...} in the preamble." >> > + >> > +(defcustom org-latex-hyperref-template "\\hypersetup{\n >> > pdfkeywords={%k},\n pdfsubject={%d},\n pdfcreator={%c}}\n" >> > + "The value of \\hyperrefsetup{...} in the preamble. String is a >> > format-spec which accepts keywords for %k (pdfkeywords), %d >> > (pdfdescription) and %c (pdfcreator). Set to nil for no >> \\hyperrefsetup." >> > :group 'org-export-latex >> > - :type 'boolean) >> > + :type 'string) >> >> The first line of the docstring should contain complete sentences only. >> I would say something along the lines: >> >> "Template for hyperref package options. >> >> Value is a format string, which can contain the following placeholders: >> >> %k for KEYWORDS line >> %d for DESCRIPTION line >> %c for CREATOR line >> >> An empty string disables the setup." >> >> Since you specify :type as 'string, it is wrong to expect a nil value in >> the variable. Note that nil is not an absolute necessity. We can allow >> to disable the template with an empty string instead. >> >> > + (format-spec org-latex-hyperref-template >> > + (format-spec-make >> > + ?k (or (plist-get info :keywords) "") >> > + ?d (or (plist-get info :description)"") >> > + ?c (if (plist-get info :with-creator) >> > + (plist-get info :creator) >> > + "")))) >> >> You are not using the :latex-hyperref property. This should be: >> >> (format-spec (plist-get info :latex-hyperref) >> ...) >> >> >> Regards, >> >> -- >> Nicolas Goaziou >> > > --089e013d0db0804fa604f2de5f79 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
With recommended changes.=A0

******** BEGIN PATCH *********
diff --git a/ox-latex.el b/ox-lat= ex.el
index 19f055e..f6e5a09 100644
--- a/ox-latex.el
+++ b/ox-latex.el
@@ -103,7 +103,7 @@
=A0= =A0 (:latex-class-options "LATEX_CLASS_OPTIONS" nil nil= t)
=A0 =A0 (:latex= -header "LATEX_HEADER" nil nil newline)
=A0 =A0 (:latex-header-e= xtra "LATEX_HEADER_EXTRA" nil nil newline)
- =A0 (:latex-hyperref-p nil "texht&= quot; org-latex-with-hyperref t)
+ =A0 (:latex-hyperref n= il nil org-latex-hyperref-template t)
=A0 =A0 ;; Redefine regular options.
=A0 =A0 (:date "DATE" nil &q= uot;\\today" t)))
=A0
@@ -341,10 +341,18 @@ the toc:nil option, not to those g= enerated with #+TOC keyword."
=A0 =A0:group 'org-export-= latex
=A0 =A0:type 'string)
=A0
-(defcust= om org-latex-with-hyperref t
- =A0"Toggle insertion of \\hypersetup{...} in the preamble."= ;
+(defcustom org-latex-hyperref-template "\\hypersetup{\n p= dfkeywords=3D{%k},\n =A0pdfsubject=3D{%d},\n =A0pdfcreator=3D{%c}}\n"<= /div>
+ =A0"Template for hyperref package options.
+
+Va= lue is a format string, which can contain the following placeholders:
=
+
+ =A0%k for KEYWORDS line
+ =A0%d for DESCRIPTIO= N line
+ =A0%c for CREATOR line
+
+An empty string disabl= es the setup."
=A0 =A0:group 'org-export-latex
- =A0:type 'boolean)
+ =A0:type 'string)
=A0
=A0;;;; Headline
=A0
@@ -1118,12 +1126,13 @@ holding ex= port options."
=A0 =A0 =A0 ;; Title
=A0 =A0 =A0 (f= ormat "\\title{%s}\n" title)
=A0 =A0 =A0 ;; Hyperref op= tions.
- =A0 =A0 (when (plist-get info :latex-hyperref-p)
- =A0 =A0 =A0 (format "\\hypersetup{\n =A0pdfkeywords=3D{%s},\n = =A0pdfsubject=3D{%s},\n =A0pdfcreator=3D{%s}}\n"
- =A0 =A0 =A0 (or (plist-get info :keywor= ds) "")
- =A0 =A0 =A0 (or (plist-= get info :description) "")
- =A0 =A0 =A0 (if (not (plist-get info :with-creator)) &qu= ot;"
- (plist-get info :creat= or))))
+ =A0 =A0 (format-spec (plist-get info :latex-hyperref)
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(format-spec-make
+ = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?k (or (plist-get info :keywords) "= ;")
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?d (or (plist-get info :descript= ion)"")
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?c (if (p= list-get info :with-creator)
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0(plist-get info :creator)
+ =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"")))
=A0 =A0 =A0 ;; Document start.
=A0 =A0 =A0 "\\begin{doc= ument}\n\n"
=A0 =A0 =A0 ;; Title command.
********= END PATCH *********


On Thu, Feb 20, 2014 at 3:52 PM, Joe Hirn <joseph.hirn@gmail.com&g= t; wrote:
I really appreciate your review. Sorry for the pedestrian = code submission. I don't get candid critical feedback on my e-lisp, so = it's great to learn more about its idioms and conventions.=A0

<= /div>
I'll incorporate your feedback into a new patch.=A0


On Thu, Feb 20, 2014 at 2:51 PM, Nicolas Goaziou <n.goaziou@gmail.com> wrote:
Hello,

Joe Hirn <jos= eph.hirn@gmail.com> writes:

> I was able to test this on my local machine and it seems to= work as we
> discussed.
>
> If there are any other changes to the patch you'd like to see, ple= ase let
> me know.

Thank you for the patch. Here are a few comments.

> - =A0 (:latex-hyperref-p nil "texht" org-latex-with-hyperref= t)
> + =A0 (:latex-hyperref-p nil "texht" (if org-latex-hyperref-= template t) t)

I think we can drop the "-p" suffix since this is no longer= a predicate.
So the property can be named :latex-hyperref.

Also we can replace "texht" with nil since it doesn't make mu= ch sense to
specify a full template from the OPTIONS line.

Eventually, the default value should be `org-latex-hyperref-template'.<= br>
This boils down to the following line:

=A0 (:latex-hyperref nil nil org-latex-hyperref-template t)

> -(defcustom org-latex-with-hyperref t
> - =A0"Toggle insertion of \\hypersetup{...} in the preamble."= ;
> +
> +(defcustom org-latex-hyperref-template "\\hypersetup{\n
> =A0pdfkeywords=3D{%k},\n =A0pdfsubject=3D{%d},\n =A0pdfcreator=3D{%c}}= \n"
> + =A0"The value of \\hyperrefsetup{...} in the preamble. String i= s a
> format-spec which accepts keywords for %k (pdfkeywords), %d
> (pdfdescription) and %c (pdfcreator). Set to nil for no \\hyperrefsetu= p."
> =A0 =A0:group 'org-export-latex
> - =A0:type 'boolean)
> + =A0:type 'string)

The first line of the docstring should contain complete sentences onl= y.
I would say something along the lines:

=A0 "Template for hyperref package options.

Value is a format string, which can contain the following placeholders:

=A0 %k for KEYWORDS line
=A0 %d for DESCRIPTION line
=A0 %c for CREATOR line

An empty string disables the setup."

Since you specify :type as 'string, it is wrong to expect a nil value i= n
the variable. Note that nil is not an absolute necessity. We can allow
to disable the template with an empty string instead.

> + =A0 =A0 =A0 (format-spec org-latex-hyperref-template
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(format-spec-make
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?k (or (plist-get info :keyw= ords) "")
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?d (or (plist-get info :desc= ription)"")
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?c (if (plist-get info :with= -creator)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(plist-get in= fo :creator)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0""))))<= br>
You are not using the :latex-hyperref property. This should be:

=A0 (format-spec (plist-get info :latex-hyperref)
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0...)


Regards,

--
Nicolas Goaziou


--089e013d0db0804fa604f2de5f79--