Re: Patch: ERC italic text support

[Amin Bandali]

Hi Barna,

Thank you for your patch.  The form was sent off-list.  Please let us
know when the copyright assignment process is complete, so we can
proceed.

I took your patch for a test run, and it works for the most part.  One
corner case in which it did not work properly, which you mentioned in
#erc, is alternating between italic and non-italic.

Please see my feedback below.

[...]
>
> ERC lacked support for the ']' control character, it didn't
> display italic text as italic.
>
> - Added erc-italic-face
>
> - Updated the regexes
>
> - Added cond branches and extended the let expressions

Please see the "Commit messages" section of the CONTRIBUTE file for how
to format your commit message according to the Emacs/GNU conventions.
It also helps to look at the commit messages for recent commits by other
contributors. :-)

> 
> --- lisp/erc/erc-goodies.el | 24 +++++++++++++++++++----- 1 file
> changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/lisp/erc/erc-goodies.el b/lisp/erc/erc-goodies.el
> index 94d5de280c..0e9b9384b3 100644
> --- a/lisp/erc/erc-goodies.el
> +++ b/lisp/erc/erc-goodies.el
> @@ -241,6 +241,10 @@ erc-underline-face
>    "ERC underline face."
>    :group 'erc-faces)
>  
> +(defface erc-italic-face '((t :slant italic))
> +  "ERC italic face."
> +  :group 'erc-faces)
> +

I think it would be best to add the italic-related bits directly after
the bold-related bits (as opposed to after underline parts) throughout
the patch, since bold and italic are quite often used together or seen
near each other.

[...]
> @@ -419,7 +426,7 @@ erc-controls-interpret
>                                   bg nil))

You forgot to set italicp back to nil here in the "\C-o" case of the
`erc-controls-interpret' function.

[...]
> @@ -432,11 +439,11 @@ erc-controls-strip
>        s)))
>  
>  (defvar erc-controls-remove-regexp
> -  "\C-b\\|\C-_\\|\C-v\\|\C-g\\|\C-o\\|\C-c[0-9]?[0-9]?\\(,[0-9][0-9]?\\)?"
> +  
> "\C-b\\|\C-_\\|\C-]\\|\C-v\\|\C-g\\|\C-o\\|\C-c[0-9]?[0-9]?\\(,[0-9][0-9]?\\)?"
>    "Regular expression which matches control characters to remove.")
>  
>  (defvar erc-controls-highlight-regexp
> -  (concat "\\(\C-b\\|\C-v\\|\C-_\\|\C-g\\|\C-o\\|"
> +  (concat "\\(\C-b\\|\C-v\\|\C-_\\|\C-]\\|\C-g\\|\C-o\\|"
>            "\C-c\\([0-9][0-9]?\\)?\\(,\\([0-9][0-9]?\\)\\)?\\)"
>            "\\([^\C-b\C-v\C-_\C-c\C-g\C-o\n]*\\)")

You missed adding \C-] to the last line of
`erc-controls-highlight-regexp'.  Adding that fixes the alternating
issue for me.

[...]

Can you send a v2 addressing the above points?  Other than that, it
looks good to me!

Thanks,
amin

signature.asc
Description: PGP signature