Apologies, I just realized I didn't include the whole patch in the last
submission.
Here is the entire diff.


On Wed, Jun 18, 2014 at 10:40 AM, Kelvin White <kelvin.white77@gmail.com>
wrote:

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