[ஞாயிறு ஜூலை 10, 2022] Eli Zaretskii wrote:

> Please name the new input method "tamil-phonetic", not just "tamil",
> so that users who type "C-u C-\ tamil TAB" could have some means of
> making the decision which one to choose.

Done.

>> +;; This is needed since the Unicode codepoint order does not reflect
>> +;; the actual order in the Tamil language.
>> +(defvar quail-tamil-itrans--consonant-order
>> +  '(("க" . 0) ("ங" . 1) ("ச" . 2) ("ஞ" . 3) ("ட" . 4) ("ண" . 5)
>> +    ("த" . 6) ("ந" . 7) ("ப" . 8) ("ம" . 9) ("ய" . 10) ("ர" . 11)
>> +    ("ல" . 12) ("வ" . 13) ("ழ" . 14) ("ள" . 15) ("ற" . 16) ("ன" . 17)
>> +    ("ஜ" . 18) ("ஸ" . 19) ("ஷ" . 20) ("ஹ" . 21) ("க்ஷ" . 22)
>> +    ("க்‌ஷ" . 23) ("ஶ" . 24)))
>
> Since the characters are ordered in the correct order, I wonder why we
> need the explicit ordinal numbers here: they are determined by the
> index of the character in the list.

Ah yes, we could use seq-position, I forgot about that.  Now done.

>> +(defun quail-tamil-itrans-compute-syllable-table (vowels consonants)
>> +  "Return the syllable table for the input method as a string.
>> +VOWELS is a list of (VOWEL SIGN TRANS) where VOWEL is a string or
>> +character representing the Tamil vowel character, SIGN is the
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> What does it mean "character representing ... character"?  Can you
> clarify this confusing part of the doc string?

I mean to say that VOWEL can be the datatypes string or character.  But
now, I cut that part out since I say no such thing for CONSONANT as
well.

>> +vowel sign corresponding to VOWEL or nil for none,
>
> Likewise here: "vowel corresponding to VOWEL"?

It should be vowel sign corresponding to VOWEL.  I'm not sure how to
phrase it better, I borrowed the term "vowel sign" from the Unicode name
(e.g., name of ு a.k.a. #x0bc1).

>>                                                    and TRANS is
>> +the input sequence to insert VOWEL.
>
> The input sequence is generally a sequence of ASCII characters, is
> that right?  If so, I think telling that would make the documentation
> more clear.  Also, TRANS is a peculiar name for something described as
> "input sequence", so maybe rename it to INPUT-SEQ?
>
>> +CONSONANTS is a list of (CONSONANT TRANS...) where CONSONANT is
>> +the Tamil consonant character, and TRANS is one or more strings
>> +that describe how to insert CONSONANT."
>
> Same here regarding TRANS and its description.

Now done.

>> +  (setq vowels (sort vowels (lambda (x y) (string-lessp (car x) (car y))))
>> +        consonants (sort consonants
>> +                         (lambda (x y)
>> +                           (< (or (assoc-default (car x) quail-tamil-itrans--consonant-order) 10000)
>> +                              (or (assoc-default (car y) quail-tamil-itrans--consonant-order) 10000)))))
>
> Can you wrap these long lines, so that they would be easier to read?

I hope it is better now.

>> +  (let ((digits "௦௧௨௩௪௫௬௭௮௯")
>>  	(width 6) clm)
>>      (with-temp-buffer
>> -      (insert "\n" (make-string 18 ?-) "+")
>> -      (when digitp (insert (make-string 60 ?-)))
>> +      (insert "\n" (make-string 18 ?-))
>> +      (when digitp
>> +        (insert "+" (make-string 60 ?-)))
>>        (insert "\n")
>>        (insert
>>         (propertize "\t" 'display '(space :align-to 5)) "various"
>> -       (propertize "\t" 'display '(space :align-to 18)) "|")
>> +       (propertize "\t" 'display '(space :align-to 18)))
>>        (when digitp
>>          (insert
>> -         (propertize "\t" 'display '(space :align-to 45)) "digits"))
>> -      (insert "\n" (make-string 18 ?-) "+")
>> +          "|" (propertize "\t" 'display '(space :align-to 45)) "digits"))
>> +      (insert "\n" (make-string 18 ?-))
>
> Did you test those :align-to specs when display-line-numbers is in
> use?

Seems to work fine from a short test on my side.

>> +;;;
>> +;;; Tamil phonetic input method
>> +;;;
>> +
>> +;; Define the input method straightaway.
>> +(quail-define-package "tamil" "Tamil" "ழ" t
>> + "Customisable Tamil phonetic input method.
>
> See above regarding the name of the input method.

Done.

>> +    ;; Consonants.
>> +    ("க்" "k" "g") ("ங்" "ng") ("ச்" "ch" "s") ("ஞ்" "nj") ("ட்" "t" "d")
>> +    ("ண்" "N") ("த்" "th" "dh") ("ந்" "nh") ("ப்" "p" "b") ("ம்" "m")
>> +    ("ய்" "y") ("ர்" "r") ("ல்" "l") ("வ்" "v") ("ழ்" "z" "zh")
>> +    ("ள்" "L") ("ற்" "rh") ("ன்" "n")
>> +    ;; Sanskrit.
>> +    ("ஜ்" "j") ("ஸ்" "S") ("ஷ்" "sh") ("ஹ்" "h")
>> +    ("க்‌ஷ்" "ksh") ("க்ஷ்" "ksH") ("ஶ்" "Z")
>> +
>> +    ;; Misc.  ஃ is neither a consonant nor a vowel.
>> +    ("ஃ" "F" "q")
>> +    ("ௐ" "OM"))
>> +  "List of input sequences to translate to Tamil characters.
>> +Each element should be (CHARACTER . TRANSLATIONS) where CHARACTER
>
> The (CHARACTER . TRANSLATIONS) form seems to imply the elements are
> cons cells, but the value itself uses lists.  Suggest to say instead
>
>   Each element should be (CHARACTER TRANSLATIONS...)
>

Done.

>> +is the Tamil character, and TRANSLATIONS is a list of input
>> +sequences to translate to that character.
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> "sequences which produce that character" is better.  And I suggest to
> use INPUT-SEQUENCES here, not TRANSLATIONS, for the reason explained
> above.
>

Done.

>> +CHARACTER is considered as a consonant (மெய் எழுத்து) if it ends
>> +with a pulli.
>
> What is a "pulli"?  It is not a character name AFAICT.
>

It is the Tamil name for virama.  I use pulli over virama since I don't
think any Tamil reader would know it.  But I put virama in brackets now
for future maintainers.

>> +CHARACTER is that is neither a vowel nor a consonant are
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Typo and/or redundant words here.
>

Fixed, thanks.

>> +considered as \"miscellaneous\" characters and are inserted as
>> +is.
>
> Not sure what this wants to say: the fact that characters are inserted
> in some way seems to be unrelated to the description of the value.
> What is this about?

I tried to allude to the miscellaneous section in the docstring but I
don't think it is really necessary.  Now removed.

>> +The input sequence for consonant+vowel pairs (உயிர்மெய் எழுத்துக்கள்)
>> +is the input sequence for the consonant followed by the
>> +corresponding vowel."
>
> Isn't that obvious?  If not, the non-obvious part(s) should be
> mentioned explicitly.

Thinking twice, yes, it should be obvious.  I removed this part.

>> +  :group 'tamil-input
>> +  :type '(alist :key-type string :value-type (repeat string))
>> +  :set #'tamil--setter
>> +  :options
>
> This defcustom lacks the :version tag.
>

Oops, now fixed.

Updated patch attached.