bug#17755: 24.3; ERC user mode support

[Stefan Monnier]

> Here is the patch to add this feature

Thanks, here are some comments on it.  I wish someone who has worked on
ERC could say something.  I never use(d) IRC and have hence no clue what is
a "user mode prefix", for example.

> ***************
> *** 1244,1250 ****
>                          (erc-format-message
>                           'JOIN ?n nick ?u login ?h host ?c chnl))))))
>             (when buffer (set-buffer buffer))
> !           (erc-update-channel-member chnl nick nick t nil nil host login)
>             ;; on join, we want to stay in the new channel buffer
>             ;;(set-buffer ob)
>             (erc-display-message parsed nil buffer str))))))
> --- 1244,1250 ----
>                          (erc-format-message
>                           'JOIN ?n nick ?u login ?h host ?c chnl))))))
>             (when buffer (set-buffer buffer))
> !           (erc-update-channel-member chnl nick nick t nil nil nil nil nil 
> host login)
>             ;; on join, we want to stay in the new channel buffer
>             ;;(set-buffer ob)
>             (erc-display-message parsed nil buffer str))))))

In my opinion, erc-update-channel-member had too many arguments already.
Maybe some of these args should be combined into an erc-channel-user object?

> + (defsubst erc-channel-user-owner-p (nick)
> +   "Return t if NICK is an owner of the current channel."

Usually we say "non-nil" rather than "t", unless the callers need to
rely on the return value being t rather than some other non-nil value.

> + (defface erc-nick-prefix-face '((t :weight bold))
> +   "ERC face used for user mode prefix."
> +   :group 'erc-faces)
> +
> + (defface erc-my-nick-prefix-face '((t :weight bold))
> +   "ERC face used for my user mode prefix."
> +   :group 'erc-faces)

Try to use the :inherit property at least to link those two (so users
who just want to change the two without making them different only need
to change one of the two) and ideally by inheriting from some other face.

> + (defun erc-get-user-mode-prefix (user)
> +   (when user
> +     (cond ((erc-channel-user-voice-p user) "+")
> +           ((erc-channel-user-half-op-p user) "%")
> +           ((erc-channel-user-op-p user) "@")
> +           ((erc-channel-user-admin-p user) "&")
> +           ((erc-channel-user-owner-p user) "~")
> +           (t ""))))

Here I assume there's some kind of logic or convention.  If not, maybe
it would be appropriate to do something like add some `help-echo'
property to those extra chars?

One more thing: the above suggests that maybe
voice/halfop/op/admin/owner are mutually exclusive.  Is that the case?
Could these be collapsed into a single element which could have values
`voice', `halfop', `op', `admin', or `owner' (or nil)?

>   (defun erc-format-@nick (&optional user channel-data)
>     "Format the nickname of USER showing if USER is an operator or has voice.
>   Operators have \"@\" and users with voice have \"+\" as a prefix.
>   Use CHANNEL-DATA to determine op and voice status.
>   See also `erc-format-nick-function'."
>     (when user
> !     (let ((nick (erc-server-user-nickname user)))
> !       (concat (erc-propertize (erc-get-user-mode-prefix nick) 'face 
> 'erc-nick-prefix-face) nick))))

Please try to stay with 80 columns.

BTW, IIUC, ERC is not distributed separately from Emacs any more, so we
don't need to use compatibility crutches like erc-propertize any more
(tho it's fine to use it as well for now, and it could be removed "all at
once" in another patch).

> !                "(ov)@+"))
[...]
> !                  "(qaohv)~&@%+"))

Yay!  Magic!

> !   (let (prefix op-ch voice-ch names name op voice)
>       (setq prefix (erc-parse-prefix))
> !     (setq op-ch (cdr (assq ?o prefix))
> !         voice-ch (cdr (assq ?v prefix)))

this should have been

      (let* ((prefix (erc-parse-prefix))
             (op-ch (cdr (assq ?o prefix)))
             (voice-ch (cdr (assq ?v prefix)))
             names name op voice)

Which is both cleaner and faster.

So when you change such code, you can take advantage of the change to
try and reduce occurrences of those "let-without-init followed by setq".

> --- 4763,4820 ----
>             (if (rassq (elt item 0) prefix)
>                 (cond ((= (length item) 1)
>                        (setq updatep nil))
> +                     ((eq (elt item 0) voice-ch)
> +                      (setq name (substring item 1)
> +                            op 'off
> +                            voice 'on
> +                            halfop 'off
> +                            admin 'off
> +                            owner 'off))
> +                     ((eq (elt item 0) hop-ch)
> +                      (setq name (substring item 1)
> +                            op 'off
> +                            voice 'off
> +                            halfop 'on
> +                            admin 'off
> +                            owner 'off))
>                       ((eq (elt item 0) op-ch)
>                        (setq name (substring item 1)
>                              op 'on
> !                            voice 'off
> !                            halfop 'off
> !                            admin 'off
> !                            owner 'off))
> !                     ((eq (elt item 0) adm-ch)
> !                      (setq name (substring item 1)
> !                            op 'off
> !                            voice 'off
> !                            halfop 'off
> !                            admin 'on
> !                            owner 'off))
> !                     ((eq (elt item 0) own-ch)
>                        (setq name (substring item 1)
>                              op 'off
> !                            voice 'off
> !                            halfop 'off
> !                            admin 'off
> !                            owner 'on))
>                       (t (setq name (substring item 1)
>                                op 'off
> !                              voice 'off
> !                              halfop 'off
> !                              admin 'off
> !                              owner 'off)))

This also makes it sound like those op/voice/admin/owner are mutually
exclusive and should be combined into a single element.

Otherwise, please simplify the code with:

   (setq op 'off voice 'off halfop 'off admin 'off owner 'off)
   (cond
    ((eq (elt item 0) voice-ch)
     (setq name (substring item 1)
           voice 'on))
    [...])

> +           (when (and voice
> +                      (not (eq (erc-channel-user-voice cuser) voice)))
> +             (setq changed t)
> +             (setf (erc-channel-user-voice cuser)
> +                   (cond ((eq voice 'on) t)
> +                         ((eq voice 'off) nil)
> +                         (t voice))))

Won't this cause `changed' to "always" be set to t, since
(erc-channel-user-voice cuser) will never be `on' or `off' and hence
never be equal to `voice'?

Also, instead of using on/off and converting them from&to nil/t, maybe
it would be simpler to use nil/t plus a special value
(e.g. `:unspecified') for the case where the value is simply
not provided.


        Stefan