> 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.

A user mode prefix is referring to a symbol prefixed to your nickname to
display to other users that you have a certain user mode.
Take "&nickname" for instance. The "&" is the user mode prefix showing you
have +a (admin) user mode.

> > ***************
> > *** 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?

My first approach was to change the erc-channel-user struct to use a list
of modes,
eliminating some of the args, but it seemed to cause issues elsewhere.
 I'll revisit this again soon.

> > + (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.

Indeed, the callers do rely on the value being t in this case. rather than
just non-nil

> > + (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.

Good idea. I have updated this so each of these two faces inherits from the
appropriate nick faces.
By default the prefix will be the same color unless these are changed
individually.

> > + (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?

Sure, I've updated the patch to add help-echo props

> 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)?

These are actually not mutually exclusive. A user could have all of these
modes enabled, but it isn't typical.
In that case we only want to display the most valuable. If a user has +vo
we want to display @.
If a user has +oa we display &. etc. Glad you noticed this though, this
should check in reverse order.

> >   (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.

Ok

> 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).

Good point, I'll clean that up in another patch.

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

>Yay!  Magic!

;D these are the default user modes

> !   (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".

This has been updated, thanks.

> 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))
>    [...])

Yes, I agree. I have simplified it.

> > +           (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.

Sure, I will look at revising this in a separate patch. I tried to keep
this as simple
as possible but I have noticed things like this and others that could be
simplified
and cleaned up a bit.

Attached is the new patch cleaned up per your suggestions.

Thanks