Hello Glenn,
Maybe I have missed your answer. If there was no answer yet, please take your time, no hurry, I just wanted to make sure that I have not overlooked anything.
  V.

________________________________
De : Vincent Belaïche <vincent.b.1@hotmail.fr>
Envoyé : mercredi 30 août 2017 11:45
À : Glenn Morris
Cc : Edward Reingold; emacs-devel
Objet : RE: cal-tex.el landscape patch


Hello,

Comments below,


De : Glenn Morris <rgm@gnu.org>
Envoyé : lundi 28 août 2017 19:55
À : Vincent Belaïche
Cc : Edward Reingold; emacs-devel
Objet : Re: cal-tex.el landscape patch
>
>Thanks, comments below.
>
>> --- a/lisp/calendar/cal-tex.el
>> +++ b/lisp/calendar/cal-tex.el
>> @@ -259,12 +259,37 @@ cal-tex-list-diary-entries
>>  (defun cal-tex-preamble (&optional args)
>>    "Insert the LaTeX calendar preamble into `cal-tex-buffer'.
>>  Preamble includes initial definitions for various LaTeX commands.
>> -Optional string ARGS are included as options for the article document class."
>> +Optional string ARGS are included as options for the article
>> +document class with inclusion of default values \"12pt\" for
>> +size, and \"a4paper\" for paper unless size or paper are already
>> +specified in ARGS.  When ARGS is omitted, by default the option
>> +\"12pt,a4paper\" is passed.
>
>I think in hindsight my suggestion to default to 12pt was not a good
>one, because it makes the argument parsing ugly. Sorry.

So you mean that I should remove the argument parsing. But it cannot be
removed all together because anyway I parse this argument to detect
landscape option.

>And please don't default to A4 paper, since the default locale for
>Emacs is the US one (spelling etc).

Would it be acceptable then if letterpaper is specified. Specifying the
paper is a good idea because some latex distribution can be installed
with a default paper size different from letterpaper.

If we want \paperwidth and \paperheight defined to the correct value in
the document so that the LaTeX code compute the cell size autonmously
and the users can configure what paper size they want, then it is good
that paper is explicitely defined.

>
>> +Please note that if ARGS is \"\" then
>> +\"\\documentclass[]{article}\<file://\\documentclass[]{article}\>" is inserted, while if ARGS it `t'
>> +then \"\\documentclass{article}\<file://\\documentclass{article}\>" is inserted."
>
>This doesn't seem like a nice interface.  Why do you need the ARGS t
>case when it can be nil?

If nil, then the options list is the default one, that is in the case of
my path `12pt,a4paper'. It could make that an empty string generates
`\documentclass{article}' instead of `\documentclass[]{article}' that
would remove the need of having the `t' case.

I thought that it was good to let the function caller decide what they
want in the output, but since `\documentclass{article}' and
`\documentclass[]{article}' are equivalent this is not really usefull.


>
>>    (set-buffer (generate-new-buffer cal-tex-buffer))
>> -  (insert (format "\\documentclass%s{article}\n<file://\\documentclass%s{article}\n>"
>> -                  (if (stringp args)
>> -                      (format "[%s]" args)
>> -                    "")))
>> +  (save-match-data
>> +    (insert (format "\\documentclass%s{article}\n<file://\\documentclass%s{article}\n>"
>> +                    (cond
>> +                     ((stringp args)
>> +                      ;; set default size
>> +                      (unless (string-match "\\(^\\|,\\) *[0-9]+pt *\\(,\\|$\\)" args)
>> +                        (setq args (concat args ",12pt")))
>> +                      ;; set default paper
>> +                      (unless (string-match "\\(^\\|,\\) *\\([ab][4-5]\\|le\\(tter\\|gal\\)\\|executive\\)paper *\\(,\\|$\\)" args)
>> +                        (setq args (concat args ",a4paper")))
>> +                      (when (string= (substring args 0 1) ",")
>> +                        (setq args (substring args 1)))
>> +                      (format "[%s]" args))
>> +                     ((null args) "[12pt]")

Reading the patch, I realize that I did a mistake, it should have been

   ((null args) "[12pt,a4paper]")

Or with whatever paper size you want a default.


>>
>> + (t ""))))
>
>This seems overly complicated to me.

I see here the main point for acceptance of this patch. Would it be more
acceptabe if the (string-match blah blah blah) would be replaced by some
defsubst like

(cal-tex-has-size-p args)

(cal-tex-has-paper-p args)



>Again, it was a bad suggestion of mine to have default values.

As I wrote, having the paper size explicitely configured would help if
different paper sizes are to be supported in the future. And this
feature is part of the TO DO list at the beginning of cal-tex.el :

;; TO DO

[...]

;;     (*)  Make calendar styles for A4 paper.

Now, this paper and point size defaults should certainly be some
defcustom. Even more, there should be one first defcustom, call it
cal-tex-default-options for all the types of calendars, and one other
defcustom, call it cal-tex-default-options-alist, for types calendar
that need some particular configuration different from
cal-tex-default-options.

>string-match-p would avoid the need to save-match-data.

OK, noted. I will use this in the future.

  Vincent.