From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Hirn <joseph.hirn@gmail.com> Subject: Re: org-export-latex-hyperref-options-format Date: Thu, 20 Feb 2014 16:53:25 -0600 Message-ID: <CALY_e1q5tqVOT3mkf-PrkjwR=Z8sZo40JFX1MTGdG9aNPPr7wQ@mail.gmail.com> References: <CALY_e1oJLqW+NEKSs=kdCF8VCwiUfyOLi+1AALZGiTNSAOsrAA@mail.gmail.com> <87a9dpo0as.fsf@gmail.com> <CALY_e1rKrd8R3=wJQ6wZDs0Zx9YbaRnho8LNegUKZ=Bq-EHhcA@mail.gmail.com> <8738jhnx1m.fsf@gmail.com> <CALY_e1rqjkmaj0gFKN3JdZ4hVGQsLHfs5a3NXw0r3983Qnhu+g@mail.gmail.com> <878ut72qyo.fsf@gmail.com> <CALY_e1rogor3qbcB_giGJTWHWw0T6QOZg2+euMTL_-TVfaYAXQ@mail.gmail.com> <87zjlm13st.fsf@gmail.com> <8672745997102408713@unknownmsgid> <CALY_e1pnQ+hRKwcuifuAZrAKyStvVN9E8UU02SKCEg3ij9CHew@mail.gmail.com> <87vbw91pac.fsf@gmail.com> <CALY_e1puF1G+tb5h=WJ+w1HNFjEXKA08723YuZrKgu5=fGCQsA@mail.gmail.com> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=089e013d0db0804fa604f2de5f79 Return-path: <emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org> Received: from eggs.gnu.org ([2001:4830:134:3::10]:54945) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from <joseph.hirn@gmail.com>) 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 <joseph.hirn@gmail.com>) 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 <joseph.hirn@gmail.com>) 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 <emacs-orgmode@gnu.org>; Thu, 20 Feb 2014 14:53:25 -0800 (PST) In-Reply-To: <CALY_e1puF1G+tb5h=WJ+w1HNFjEXKA08723YuZrKgu5=fGCQsA@mail.gmail.com> List-Id: "General discussions about Org-mode." <emacs-orgmode.gnu.org> List-Unsubscribe: <https://lists.gnu.org/mailman/options/emacs-orgmode>, <mailto:emacs-orgmode-request@gnu.org?subject=unsubscribe> List-Archive: <http://lists.gnu.org/archive/html/emacs-orgmode> List-Post: <mailto:emacs-orgmode@gnu.org> List-Help: <mailto:emacs-orgmode-request@gnu.org?subject=help> List-Subscribe: <https://lists.gnu.org/mailman/listinfo/emacs-orgmode>, <mailto:emacs-orgmode-request@gnu.org?subject=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 <n.goaziou@gmail.com> Cc: "emacs-orgmode@gnu.org" <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 <joseph.hirn@gmail.com> 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 <n.goaziou@gmail.com>wrote: > >> Hello, >> >> Joe Hirn <joseph.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, 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 <div dir=3D"ltr"><div>With recommended changes.=A0</div><div><br></div><div= >******** BEGIN PATCH *********</div><div>diff --git a/ox-latex.el b/ox-lat= ex.el</div><div>index 19f055e..f6e5a09 100644</div><div>--- a/ox-latex.el</= div> <div>+++ b/ox-latex.el</div> <div>@@ -103,7 +103,7 @@</div><div>=A0<span style=3D"white-space:pre-wrap">= </span> =A0 (:latex-class-options "LATEX_CLASS_OPTIONS" nil nil= t)</div><div>=A0<span style=3D"white-space:pre-wrap"> </span> =A0 (:latex= -header "LATEX_HEADER" nil nil newline)</div> <div>=A0<span style=3D"white-space:pre-wrap"> </span> =A0 (:latex-header-e= xtra "LATEX_HEADER_EXTRA" nil nil newline)</div><div>-<span style= =3D"white-space:pre-wrap"> </span> =A0 (:latex-hyperref-p nil "texht&= quot; org-latex-with-hyperref t)</div> <div>+<span style=3D"white-space:pre-wrap"> </span> =A0 (:latex-hyperref n= il nil org-latex-hyperref-template t)</div><div>=A0<span style=3D"white-spa= ce:pre-wrap"> </span> =A0 ;; Redefine regular options.</div><div>=A0<span = style=3D"white-space:pre-wrap"> </span> =A0 (:date "DATE" nil &q= uot;\\today" t)))</div> <div>=A0</div><div>@@ -341,10 +341,18 @@ the toc:nil option, not to those g= enerated with #+TOC keyword."</div><div>=A0 =A0:group 'org-export-= latex</div><div>=A0 =A0:type 'string)</div><div>=A0</div><div>-(defcust= om org-latex-with-hyperref t</div> <div>- =A0"Toggle insertion of \\hypersetup{...} in the preamble."= ;</div><div>+(defcustom org-latex-hyperref-template "\\hypersetup{\n p= dfkeywords=3D{%k},\n =A0pdfsubject=3D{%d},\n =A0pdfcreator=3D{%c}}\n"<= /div><div> + =A0"Template for hyperref package options.</div><div>+</div><div>+Va= lue is a format string, which can contain the following placeholders:</div>= <div>+</div><div>+ =A0%k for KEYWORDS line</div><div>+ =A0%d for DESCRIPTIO= N line</div> <div>+ =A0%c for CREATOR line</div><div>+</div><div>+An empty string disabl= es the setup."</div><div>=A0 =A0:group 'org-export-latex</div><div= >- =A0:type 'boolean)</div><div>+ =A0:type 'string)</div><div>=A0</= div><div> =A0;;;; Headline</div><div>=A0</div><div>@@ -1118,12 +1126,13 @@ holding ex= port options."</div><div>=A0 =A0 =A0 ;; Title</div><div>=A0 =A0 =A0 (f= ormat "\\title{%s}\n" title)</div><div>=A0 =A0 =A0 ;; Hyperref op= tions.</div><div> - =A0 =A0 (when (plist-get info :latex-hyperref-p)</div> <div>- =A0 =A0 =A0 (format "\\hypersetup{\n =A0pdfkeywords=3D{%s},\n = =A0pdfsubject=3D{%s},\n =A0pdfcreator=3D{%s}}\n"</div><div>-<span styl= e=3D"white-space:pre-wrap"> </span> =A0 =A0 =A0 (or (plist-get info :keywor= ds) "")</div> <div>-<span style=3D"white-space:pre-wrap"> </span> =A0 =A0 =A0 (or (plist-= get info :description) "")</div><div>-<span style=3D"white-space:= pre-wrap"> </span> =A0 =A0 =A0 (if (not (plist-get info :with-creator)) &qu= ot;"</div> <div>-<span style=3D"white-space:pre-wrap"> </span> (plist-get info :creat= or))))</div><div>+ =A0 =A0 (format-spec (plist-get info :latex-hyperref)</d= iv><div>+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(format-spec-make</div><div>+ = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?k (or (plist-get info :keywords) "= ;")</div> <div>+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?d (or (plist-get info :descript= ion)"")</div><div>+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?c (if (p= list-get info :with-creator)</div><div>+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0(plist-get info :creator)</div><div>+ =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"")))</div> <div>=A0 =A0 =A0 ;; Document start.</div><div>=A0 =A0 =A0 "\\begin{doc= ument}\n\n"</div><div>=A0 =A0 =A0 ;; Title command.</div><div>********= END PATCH *********</div></div><div class=3D"gmail_extra"><br><br><div cla= ss=3D"gmail_quote"> On Thu, Feb 20, 2014 at 3:52 PM, Joe Hirn <span dir=3D"ltr"><<a href=3D"= mailto:joseph.hirn@gmail.com" target=3D"_blank">joseph.hirn@gmail.com</a>&g= t;</span> wrote:<br><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0= .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div dir=3D"ltr">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><br><= /div> <div>I'll incorporate your feedback into a new patch.=A0</div></div><di= v class=3D"HOEnZb"><div class=3D"h5"><div class=3D"gmail_extra"><br><br><di= v class=3D"gmail_quote">On Thu, Feb 20, 2014 at 2:51 PM, Nicolas Goaziou <s= pan dir=3D"ltr"><<a href=3D"mailto:n.goaziou@gmail.com" target=3D"_blank= ">n.goaziou@gmail.com</a>></span> wrote:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex"><div>Hello,<br> <br> Joe Hirn <<a href=3D"mailto:joseph.hirn@gmail.com" target=3D"_blank">jos= eph.hirn@gmail.com</a>> writes:<br> <br> </div><div>> I was able to test this on my local machine and it seems to= work as we<br> > discussed.<br> ><br> > If there are any other changes to the patch you'd like to see, ple= ase let<br> > me know.<br> <br> </div>Thank you for the patch. Here are a few comments.<br> <div><br> > - =A0 (:latex-hyperref-p nil "texht" org-latex-with-hyperref= t)<br> > + =A0 (:latex-hyperref-p nil "texht" (if org-latex-hyperref-= template t) t)<br> <br> </div>I think we can drop the "-p" suffix since this is no longer= a predicate.<br> So the property can be named :latex-hyperref.<br> <br> Also we can replace "texht" with nil since it doesn't make mu= ch sense to<br> specify a full template from the OPTIONS line.<br> <br> Eventually, the default value should be `org-latex-hyperref-template'.<= br> <br> This boils down to the following line:<br> <br> =A0 (:latex-hyperref nil nil org-latex-hyperref-template t)<br> <div><br> > -(defcustom org-latex-with-hyperref t<br> > - =A0"Toggle insertion of \\hypersetup{...} in the preamble."= ;<br> > +<br> > +(defcustom org-latex-hyperref-template "\\hypersetup{\n<br> > =A0pdfkeywords=3D{%k},\n =A0pdfsubject=3D{%d},\n =A0pdfcreator=3D{%c}}= \n"<br> > + =A0"The value of \\hyperrefsetup{...} in the preamble. String i= s a<br> > format-spec which accepts keywords for %k (pdfkeywords), %d<br> > (pdfdescription) and %c (pdfcreator). Set to nil for no \\hyperrefsetu= p."<br> > =A0 =A0:group 'org-export-latex<br> > - =A0:type 'boolean)<br> > + =A0:type 'string)<br> <br> </div>The first line of the docstring should contain complete sentences onl= y.<br> I would say something along the lines:<br> <br> =A0 "Template for hyperref package options.<br> <br> Value is a format string, which can contain the following placeholders:<br> <br> =A0 %k for KEYWORDS line<br> =A0 %d for DESCRIPTION line<br> =A0 %c for CREATOR line<br> <br> An empty string disables the setup."<br> <br> Since you specify :type as 'string, it is wrong to expect a nil value i= n<br> the variable. Note that nil is not an absolute necessity. We can allow<br> to disable the template with an empty string instead.<br> <div><br> > + =A0 =A0 =A0 (format-spec org-latex-hyperref-template<br> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(format-spec-make<br> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?k (or (plist-get info :keyw= ords) "")<br> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?d (or (plist-get info :desc= ription)"")<br> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?c (if (plist-get info :with= -creator)<br> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(plist-get in= fo :creator)<br> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0""))))<= br> <br> </div>You are not using the :latex-hyperref property. This should be:<br> <br> =A0 (format-spec (plist-get info :latex-hyperref)<br> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0...)<br> <br> <br> Regards,<br> <span><font color=3D"#888888"><br> --<br> Nicolas Goaziou<br> </font></span></blockquote></div><br></div> </div></div></blockquote></div><br></div> --089e013d0db0804fa604f2de5f79--