> 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.
> > + (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.
erc-patch.diff