Hi Ihor,

Find attached an updated patchset, and comments below :)

Ihor Radchenko <yantar92@gmail.com> writes:

> Maybe “The other two options”? Also, using first/second here is a bit
> confusing because: (1) they are actually 3/4 in the above list; (2)
> engraved is listed last.

Docstring changed.

>> +The second more comprehensive option can be used with,
>
> *can be set with

Changed.

> I feel slightly confused about using the word “package” here. Which one
> refers to LaTeX package and which one to Emacs? I would state “Emacs
> package” explicitly to highlight contrast with “LaTeX package”.

Clarifications added to the docstring.

>> +\\{\\\FancyVerbLine}
>
> I’d like to see a comment on what it does.

Added.

> Same request to provide a comment. Also, what it that % TODO doing
> there? It is confusing.

A comment has been added, and the TODO removed.

> Also, it is unclear what the [breakable,xparse] options to tcolorbox are
> doing there and what will happen if the user removes them.

“breakable” allows the box to be broken across pages, and “xparse” provides
`\DeclareTColorBox'.

> Further, I am wondering what is going to happen if the user happens to
> have tcolorbox without options loaded via org-latex-packages-alist.

They could see: ERROR: LaTeX Error: Option clash for package tcolorbox.

They would need to tweak such a tcolorbox entry to include the options used
here.

>> +There is quite a lot of flexibility in what this preamble can be, as long as it:
>> +- Loads the fvextra package.
>> +- Loads the package xcolor (if it is not already loader elsewhere).
>> +- Defines a \“Code\” environment (note the capital C), which can be
>> +  later used to wrap \“Verbatim\” environments (provided by fvextra).
>
> The last point is not very clear. What kind of environment?

Anything that the user wants to do to modify the display of the generated
`Verbatim' environment.

>> +(defun org-latex-generate-engraved-preamble (info syntax-colours-p)
>> +  “Generate the preamble to setup engraved code.
>> +The result is constructed from `org-latex-engraved-preamble’ and
>> +`org-latex-engraved-options’.”
>
> This is relying on
>
> (:latex-engraved-options nil nil org-latex-engraved-options)
> (:latex-engraved-preamble nil nil org-latex-engraved-preamble)
>
> If it changes any time in future (e.g. to allow per-file settings), the
> docstring may be overlooked.

Docstring tweaked.

> I’d use FIRST-MATCH argument for org-element-map. It will be slightly
> faster on large buffers.

Ah nice, I’ll make use of that.

>> -                       (downcase org-lang)))
>> +                       (downcase lang)))
>
> I am not sure if this belongs to this patch. Please double check.

Ooops, moved to the correct patch.

>> +                            (mapcar ’length
>> +                                    (org-split-string (car code-info)
>> +                                                      “”)))))
>
> I am not sure how well it will work with e.g. Chinese characters in comments.

I’ve added a patch replacing `length' with `string-width'

> Maybe the functions could be rewritten using cl-defun with keys and
> &allow-other-keys and then called via apply on a let-bound arg plist?

Done.

All the best,
Timothy