Re: [patch][ox-html] Support for level based containers

[Rasmus <rasmus@gmx.us>]

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

Hi,

Thanks for the—as always—useful comments.  I have attached a new
version of the patch with better doc and a bit better functionality, I
think.

Nicolas Goaziou <n.goaziou@gmail.com> writes:

> I don't know enough HTML to have an opinion here.

Then we are two. . .

>> and what changes are necessary, if any.
>
> Some comments follow.
>
>> +(defcustom org-html-container-element '(("div" . "section")
>> +					("div" . "article")
>> +					("div" . "div"))
>> +  "HTML elements to use for wrapping sections.
>>  Can be set with the in-buffer HTML_CONTAINER property or for
>>  publishing, with :html-container.
>>
>> -Note that changing the default will prevent you from using
>> -org-info.js for your website."
>> +Should be a list of cons cells with positions corresponding to a
>
> A "list of cons cells" is an "alist".

I think I knew that at some point, but forgot along the way.  At least
"list of cons" is better than "list of tuples". . .

>> +section.  If `org-html-html5-fancy' is t the cdr is used
>> +otherwise the car.
>
> "is non-nil" is better than "is t". Also, you shouldn't use
> `org-html-html5-fancy': see below.

>> +Note that changing the default will prevent you from
>> +using org-info.js for your website."
>>    :group 'org-export-html
>>    :version "24.4"
>>    :package-version '(Org . "8.0")
>> -  :type 'string)
>> +  :type '(repeat (cons string string)))
>
> There is an `alist' type. See (info "(elisp) Composite Types")

I went for another solution as I would prefer not to break backward
comparability.

>> +      (let* ((hc (plist-get info :html-container))
>> +	     (n (org-export-get-relative-level headline info)))
>
> You don't need a starred `let' here. Also, I suggest to not use short
> variable names. IMO "container-alist" is better than "hc" and "level"
> better than "n".

Good rule of thumb.

>> +	(cond ((listp hc)
>> +	       (or (funcall (if org-html-html5-fancy 'cdr-safe 'car-safe)
>> +			    (nth (1- (min n (length hc))) hc)) "div"))
>
> You shouldn't use directly the variable `org-html-html5-fancy' since its
> value can be overridden with external properties (e.g., during
> a publishing process, with a specific setup). Instead, it should be:
>
>   (plist-get info :html-html5-fancy)
>
> As a rule of thumb, don't use variables when there's a property in INFO
> for them.

You are right.  Thanks for spotting it.

> Also, I don't think you need to use `car-safe' instead of `car' and
> `cdr-safe' instead of `cdr'.

My "defense" is, people could make mistakes, e.g. provide the list
'("div" "div" "div").  But you are right.

> Eventually, due to the (min n (length hc)) (which should be documented
> in the docstring) and the fact that the alist cannot be empty, the
> `funcall' never evaluates to nil. Therefore, the `or' is not necessary.

See below.

>> +	      ((and (stringp hc) (= 1 n))
>> +	       (plist-get info :html-container))
>
> Note that this branch is always false since HC shouldn't be a string,
> per the defcustom type, but an alist.

Yeah, but since ATM it's a string, there may be people who've
customized it in there init.el or in project definition.  I wouldn't
want to break it for them. . .  I have adjusted the docstring to
consider this.


>> +	      (t "div")))))
>
> Given the recommendations above, the whole `cond' could be rewritten:
>
>   (funcall (if (plist-get info :html-html5-fancy) #'cdr #'car)
>            (nth (1- (min level (length container-alist))) container-alist))

More elegant. 

Thanks,
Rasmus

--
Hooray!

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Support-for-level-based-containers-in-ox-html.patch --]
[-- Type: text/x-diff, Size: 3313 bytes --]

From 3514d5bc9311f7e38fc9518c6a8f450c9fa3a37a Mon Sep 17 00:00:00 2001
From: Rasmus <w530@pank.eu>
Date: Sun, 16 Mar 2014 00:01:35 +0100
Subject: [PATCH] Support for level-based containers in ox-html

* ox-html.el (org-html-container-element): List of
cons of section-level containers.
(org-html--container): Redefine to consider
org-html-container-element.
---
 lisp/ox-html.el | 48 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/lisp/ox-html.el b/lisp/ox-html.el
index a8c924f..f34b3a0 100644
--- a/lisp/ox-html.el
+++ b/lisp/ox-html.el
@@ -940,17 +940,36 @@ versions 8 and below."
   :package-version '(Org . "8.0")
   :type 'boolean)
 
-(defcustom org-html-container-element "div"
-  "HTML element to use for wrapping top level sections.
+(defcustom org-html-container-element '(("div" . "section")
+					("div" . "article")
+					("div" . "div"))
+  "HTML elements to use for wrapping sections.
 Can be set with the in-buffer HTML_CONTAINER property or for
 publishing, with :html-container.
 
-Note that changing the default will prevent you from using
-org-info.js for your website."
+Should be an alist where each cons-cell corresponds to a section
+levels.  The first cons-cell is used for top-level headlines, the
+second cons-cell is used for second level headlines and so forth.
+Headlines with levels higher than the length of the alist use the
+last cons-cell.
+
+If the output is \"html5\" (see `org-html-doctype') and
+html5-fancy is non-nil (see `org-html-html5-fancy') then the cdr
+of cons-cell is used.  Otherwise the car is used.
+
+May also be a single string corresponding to the top-level
+container for backward comparability.
+
+Note that changing the default will prevent you from
+using org-info.js for your website."
   :group 'org-export-html
   :version "24.4"
   :package-version '(Org . "8.0")
-  :type 'string)
+  :type '(choice (repeat :tag "Fine control"
+                         (choice (cons :tag "level"
+                                       (string :tag "Default")
+                                       (string :tag "Fancy HTML5"))))
+                 (string :tag "Control top level only" :value "div")))
 
 (defcustom org-html-divs
   '((preamble  "div" "preamble")
@@ -2411,10 +2430,21 @@ holding contextual information."
 		(org-html--container headline info)))))))
 
 (defun org-html--container (headline info)
-  (or (org-element-property :HTML_CONTAINER headline)
-      (if (= 1 (org-export-get-relative-level headline info))
-	  (plist-get info :html-container)
-	"div")))
+  "Select an appropriate HTML container for a headline.
+
+The container is selected based on level and depend on
+:html-container corresponding to org-html-container-element."
+  (let ((container-alist (plist-get info :html-container))
+	(level (org-export-get-relative-level headline info)))
+    (cond ((listp container-alist)
+	   (funcall (if (and (org-html-html5-p info)
+			     (plist-get info :html-html5-fancy)) #'cdr #'car)
+		    (nth (1- (min level (length container-alist)))
+			 container-alist)))
+	  ;; Previously :html-container took a string as an argument.
+	  ((and (stringp container-alist) (= 1 level))
+	   (plist-get info :html-container))
+	  (t "div"))))
 
 ;;;; Horizontal Rule
 
-- 
1.9.0