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 &quot;LATEX_CLASS_OPTIONS&quot; nil nil=
 t)</div><div>=A0<span style=3D"white-space:pre-wrap">		</span> =A0 (:latex=
-header &quot;LATEX_HEADER&quot; nil nil newline)</div>

<div>=A0<span style=3D"white-space:pre-wrap">		</span> =A0 (:latex-header-e=
xtra &quot;LATEX_HEADER_EXTRA&quot; nil nil newline)</div><div>-<span style=
=3D"white-space:pre-wrap">		</span> =A0 (:latex-hyperref-p nil &quot;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 &quot;DATE&quot; nil &q=
uot;\\today&quot; t)))</div>

<div>=A0</div><div>@@ -341,10 +341,18 @@ the toc:nil option, not to those g=
enerated with #+TOC keyword.&quot;</div><div>=A0 =A0:group &#39;org-export-=
latex</div><div>=A0 =A0:type &#39;string)</div><div>=A0</div><div>-(defcust=
om org-latex-with-hyperref t</div>

<div>- =A0&quot;Toggle insertion of \\hypersetup{...} in the preamble.&quot=
;</div><div>+(defcustom org-latex-hyperref-template &quot;\\hypersetup{\n p=
dfkeywords=3D{%k},\n =A0pdfsubject=3D{%d},\n =A0pdfcreator=3D{%c}}\n&quot;<=
/div><div>

+ =A0&quot;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.&quot;</div><div>=A0 =A0:group &#39;org-export-latex</div><div=
>- =A0:type &#39;boolean)</div><div>+ =A0:type &#39;string)</div><div>=A0</=
div><div>

=A0;;;; Headline</div><div>=A0</div><div>@@ -1118,12 +1126,13 @@ holding ex=
port options.&quot;</div><div>=A0 =A0 =A0 ;; Title</div><div>=A0 =A0 =A0 (f=
ormat &quot;\\title{%s}\n&quot; 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 &quot;\\hypersetup{\n =A0pdfkeywords=3D{%s},\n =
=A0pdfsubject=3D{%s},\n =A0pdfcreator=3D{%s}}\n&quot;</div><div>-<span styl=
e=3D"white-space:pre-wrap">	</span> =A0 =A0 =A0 (or (plist-get info :keywor=
ds) &quot;&quot;)</div>

<div>-<span style=3D"white-space:pre-wrap">	</span> =A0 =A0 =A0 (or (plist-=
get info :description) &quot;&quot;)</div><div>-<span style=3D"white-space:=
pre-wrap">	</span> =A0 =A0 =A0 (if (not (plist-get info :with-creator)) &qu=
ot;&quot;</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) &quot=
;&quot;)</div>

<div>+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?d (or (plist-get info :descript=
ion)&quot;&quot;)</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&quot;&quot;)))</div>

<div>=A0 =A0 =A0 ;; Document start.</div><div>=A0 =A0 =A0 &quot;\\begin{doc=
ument}\n\n&quot;</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">&lt;<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&#39;t get candid critical feedback on my e-lisp, so =
it&#39;s great to learn more about its idioms and conventions.=A0<div><br><=
/div>

<div>I&#39;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">&lt;<a href=3D"mailto:n.goaziou@gmail.com" target=3D"_blank=
">n.goaziou@gmail.com</a>&gt;</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 &lt;<a href=3D"mailto:joseph.hirn@gmail.com" target=3D"_blank">jos=
eph.hirn@gmail.com</a>&gt; writes:<br>
<br>
</div><div>&gt; I was able to test this on my local machine and it seems to=
 work as we<br>
&gt; discussed.<br>
&gt;<br>
&gt; If there are any other changes to the patch you&#39;d like to see, ple=
ase let<br>
&gt; me know.<br>
<br>
</div>Thank you for the patch. Here are a few comments.<br>
<div><br>
&gt; - =A0 (:latex-hyperref-p nil &quot;texht&quot; org-latex-with-hyperref=
 t)<br>
&gt; + =A0 (:latex-hyperref-p nil &quot;texht&quot; (if org-latex-hyperref-=
template t) t)<br>
<br>
</div>I think we can drop the &quot;-p&quot; suffix since this is no longer=
 a predicate.<br>
So the property can be named :latex-hyperref.<br>
<br>
Also we can replace &quot;texht&quot; with nil since it doesn&#39;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&#39;.<=
br>
<br>
This boils down to the following line:<br>
<br>
=A0 (:latex-hyperref nil nil org-latex-hyperref-template t)<br>
<div><br>
&gt; -(defcustom org-latex-with-hyperref t<br>
&gt; - =A0&quot;Toggle insertion of \\hypersetup{...} in the preamble.&quot=
;<br>
&gt; +<br>
&gt; +(defcustom org-latex-hyperref-template &quot;\\hypersetup{\n<br>
&gt; =A0pdfkeywords=3D{%k},\n =A0pdfsubject=3D{%d},\n =A0pdfcreator=3D{%c}}=
\n&quot;<br>
&gt; + =A0&quot;The value of \\hyperrefsetup{...} in the preamble. String i=
s a<br>
&gt; format-spec which accepts keywords for %k (pdfkeywords), %d<br>
&gt; (pdfdescription) and %c (pdfcreator). Set to nil for no \\hyperrefsetu=
p.&quot;<br>
&gt; =A0 =A0:group &#39;org-export-latex<br>
&gt; - =A0:type &#39;boolean)<br>
&gt; + =A0:type &#39;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 &quot;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.&quot;<br>
<br>
Since you specify :type as &#39;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>
&gt; + =A0 =A0 =A0 (format-spec org-latex-hyperref-template<br>
&gt; + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(format-spec-make<br>
&gt; + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?k (or (plist-get info :keyw=
ords) &quot;&quot;)<br>
&gt; + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?d (or (plist-get info :desc=
ription)&quot;&quot;)<br>
&gt; + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ?c (if (plist-get info :with=
-creator)<br>
&gt; + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(plist-get in=
fo :creator)<br>
&gt; + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&quot;&quot;))))<=
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--