[AUCTeX-devel] Re: Contribution to AUCTeX

[Ralf Angeli]

* Vincent Belaïche (2010-06-21) writes:

> I can split this into 3 differents/subsequent contributions with
> respective ChangeLog -- or not, it is up to your feedback.
>
> If you want 3 separate patches, then could you please tell me in which
> order I should provide them.

The order is not really important.  The idea is that once we finished
discussing one of the patches, I can immediately check it in.  We don't
have to wait until all of them are discussed.  And it obviously makes
the code review easier.

With the current set of patches we are as good as through, I think.  See
my remarks below.  So it is not that important anymore to split them.

> 2010-06-03  Vincent Belaïche  <address@hidden>

It would be good if we'd only use one email address for your ChangeLog
entries.  The last one got the Gmail address.

> + (defconst Texinfo-structuring-command-re
> +   (concat "^\\s-*@" 
> +       (funcall 'regexp-opt (mapcar 'car texinfo-section-list)
> +        'word))

`regexp-opt' only knows `words', not `word'.  Also, is the performance
really so bad that the regexp has to be computed here?  In latex.el this
is done one the fly which has the advantage that you don't have to reset
the "variable" when you change `texinfo-section-list'.

> + (defun Texinfo-mark-section (&optional to-be-marked)
> +   "Mark current section, with inclusion of any containing to-be-marked,
> + where current section is started by any of the structuring
> + commands matched by regexp in constant
> + `Texinfo-structuring-command-re'.

This docstring is not checkdoc-compliant.  It should probably also
mention `texinfo-section-list' instead of
`Texinfo-structuring-command-re'.

> + If optional argument TO-BE-MARKED is set to 1 or is a non nil empty
> + argument, then mark current node.  
> + 
> + If optional argument TO-BE-MARKED is set to 2 or to `-', then
> + mark current section, with exclusion of any subsections."

Hm, that interface seems a bit awkard.  I'm not sure it is very lispy to
use numeric values with the prefix argument like that.  The behavior
also differs from `LaTeX-mark-section'.  Perhaps there should be two
separate commands, one for nodes and one for sectioning?

With respect to the rest of the code, there are a few lines which are
longer than 80 characters and should be shortened.

> +     (define-key map "\C-c." 'Texinfo-mark-environment)
> +     (define-key map "\C-c*" 'Texinfo-mark-section)

Those bindings are already marked as being dubious in latex.el because
the Emacs key binding conventions do not really encourage to use them.
So perhaps we should not introduce them in Texinfo mode as well.

I'm not sure if there are better alternatives.  Lisp mode, for example,
has `C-M-h' for `mark-defun'.  (Does anybody know where this `h'
mnemonic stems from?)  But then we are still one binding short.

-- 
Ralf