> 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