From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.bugs Subject: bug#17755: 24.3; ERC user mode support Date: Tue, 17 Jun 2014 12:03:52 -0400 Message-ID: References: <87wqcnag57.fsf@localhost.i-did-not-set--mail-host-address--so-tickle-me> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1403030174 28576 80.91.229.3 (17 Jun 2014 18:36:14 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 17 Jun 2014 18:36:14 +0000 (UTC) Cc: Lawrence Mitchell , Michael Olson , 17755@debbugs.gnu.org, mlang@delysid.org, Diane Murray , Alex Schroeder , Julien Danjou , Francis Litterio , Jorgen Schaefer To: Kelvin White Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Tue Jun 17 20:36:03 2014 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1WwyEj-00049X-GR for geb-bug-gnu-emacs@m.gmane.org; Tue, 17 Jun 2014 20:36:01 +0200 Original-Received: from localhost ([::1]:52309 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WwxW0-0007U1-3J for geb-bug-gnu-emacs@m.gmane.org; Tue, 17 Jun 2014 13:49:48 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:60115) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wwvsl-00017M-TU for bug-gnu-emacs@gnu.org; Tue, 17 Jun 2014 12:05:19 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wwvsd-00040H-LO for bug-gnu-emacs@gnu.org; Tue, 17 Jun 2014 12:05:11 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:59667) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wwvsd-000404-Ix for bug-gnu-emacs@gnu.org; Tue, 17 Jun 2014 12:05:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.80) (envelope-from ) id 1Wwvsd-0005nL-1F for bug-gnu-emacs@gnu.org; Tue, 17 Jun 2014 12:05:03 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 17 Jun 2014 16:05:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 17755 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 17755-submit@debbugs.gnu.org id=B17755.140302104322185 (code B ref 17755); Tue, 17 Jun 2014 16:05:02 +0000 Original-Received: (at 17755) by debbugs.gnu.org; 17 Jun 2014 16:04:03 +0000 Original-Received: from localhost ([127.0.0.1]:50816 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1Wwvre-0005lf-Rc for submit@debbugs.gnu.org; Tue, 17 Jun 2014 12:04:03 -0400 Original-Received: from ironport2-out.teksavvy.com ([206.248.154.181]:55836) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1Wwvrb-0005l2-Gi for 17755@debbugs.gnu.org; Tue, 17 Jun 2014 12:04:00 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: ArUGAIDvNVNLd+D9/2dsb2JhbABZgwaDSsA9gRcXdIIlAQEBAQIBViMFCwsuAQUSFBgNJIgECNIZF456BxIBhCUEqRmBaoNMIYEuBxc X-IPAS-Result: ArUGAIDvNVNLd+D9/2dsb2JhbABZgwaDSsA9gRcXdIIlAQEBAQIBViMFCwsuAQUSFBgNJIgECNIZF456BxIBhCUEqRmBaoNMIYEuBxc X-IronPort-AV: E=Sophos;i="4.97,753,1389762000"; d="scan'208";a="67112251" Original-Received: from 75-119-224-253.dsl.teksavvy.com (HELO pastel.home) ([75.119.224.253]) by ironport2-out.teksavvy.com with ESMTP/TLS/ADH-AES256-SHA; 17 Jun 2014 12:03:52 -0400 Original-Received: by pastel.home (Postfix, from userid 20848) id A1BEF60D53; Tue, 17 Jun 2014 12:03:52 -0400 (EDT) In-Reply-To: (Kelvin White's message of "Wed, 11 Jun 2014 07:45:12 -0400") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.4.50 (gnu/linux) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.15 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 140.186.70.43 X-Mailman-Approved-At: Tue, 17 Jun 2014 13:39:54 -0400 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:90487 Archived-At: > 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