GNU bug report logs - #76261
[PATCH] Add a mentions buffer feature to rcirc

Previous Next

Full log

Message #31 received at 76261 <at> debbugs.gnu.org (full text, mbox):

From: Tassilo Horn <tsdh <at> gnu.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 76261 <at> debbugs.gnu.org
Subject: Re: bug#76261: [PATCH] Add a mentions buffer feature to rcirc
Date: Fri, 21 Feb 2025 10:09:41 +0100

Philip Kaludercic <philipk <at> posteo.net> writes:

Hi Philip,

>> So now I'm really, really done and would welcome a review. :-)
>
> So this is the patch in question, right:
>
>> From e9961ff1ffdc4bb39c1dff7df58f5c7cef5cf448 Mon Sep 17 00:00:00 2001
>> From: Tassilo Horn <tsdh <at> gnu.org>
>> Date: Thu, 13 Feb 2025 10:39:31 +0100
>> Subject: [PATCH] Add a mentions buffer feature to rcirc
>
>> If the feature is enabled by activating rcirc-log-mentions-mode in a
>> buffer or rcirc-global-log-mentions-mode, a buffer
>
> (Not crucial, this just seems like a roundabout way to say what you
> are trying to say... Not that I am any better at it ^^)

:-)

Yeah, that seems about the right version.  (I've already pulled from
master again so the hashes are different but the commit message suggests
it's the latest version).

>> rcirc-mentions-buffer-name is created and used for logging mentions of
>> your nick or a keyword in rcirc-keywords.  The log lines link to the
>> original position in the actual channel buffer where the mention
>> occured.
>
>> * lisp/net/rcirc.el (rcirc-mentions-buffer-name): New defcustom.
>
> I think it is more conventional to refer to this as "user option".

Alright.  I'll change that.

>> (rcirc-log-mentions-mode, rcirc-global-log-mentions-mode): New
>> minor-modes.
>> (rcirc-log-mentions-mode-map): New keymap for rcirc-log-mentions-mode.
>> (rcirc-switch-to-mentions-buffer): New command bound in
>> rcirc-log-mentions-mode-map.
>
> You should add 'these-kinds-of' quotations for elisp symbols.

Fine.

>> (rcirc-mode): Enable rcirc-log-mentions-mode in the buffer if
>> rcirc-global-log-mentions-mode is activated.
>> (rcirc-mentions-buffer-mode): New major mode for the mentions buffer.
>> (rcirc-mentions-next, rcirc-mentions-prev): New movement commands in the
>> mentions buffer.
>> (rcirc--get-last-message-bounds): New helper function.
>> (rcirc-record-activity): Call rcirc-update-mentions-buffer if
>> rcirc-log-mentions-mode is activated.
>> (rcirc-update-mentions-buffer): New function.
>
>> foo
>
> ?
>
>> fix

Haha, sqashing artifacts. :-)

>> ---
>>  lisp/net/rcirc.el | 140 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 140 insertions(+)
>
>> diff --git a/lisp/net/rcirc.el b/lisp/net/rcirc.el
>> index 3f81672182b..61e867377fe 100644
>> --- a/lisp/net/rcirc.el
>> +++ b/lisp/net/rcirc.el
>> @@ -1482,6 +1482,43 @@ does not confuse the pairing."
>>        (narrow-to-region rcirc-prompt-start-marker (point-max))
>>        (funcall fallback char))))
>  
>> +(defcustom rcirc-mentions-buffer-name "*rcirc mentions*"
>> +  "The name of the mentions buffer.
>> +Also see `rcirc-log-mentions-mode' and `rcirc-global-log-mentions-mode'."
>
> I don't think you need to mention both here, but I would go into some
> more detail than just "see also" here.

Alright.

>> +  :type 'string
>> +  :version "31.1")
>> +
>> +(defvar-keymap rcirc-log-mentions-mode-map
>> +  :doc "The keymap of `rcirc-log-mentions-mode'."
>> +  "C-c C-." #'rcirc-switch-to-mentions-buffer)
>> +
>> +(define-minor-mode rcirc-log-mentions-mode
>> +  "Minor mode to log mentions in the current channel in a separate buffer.
>> +The name of this buffer is defined by `rcirc-mentions-buffer-name'.  In
>> +this buffer, mentions of your nick or a keyword in `rcirc-keywords' will
>> +be logged linking to the original message in the respective channel
>> +buffer.
>> +
>> +Also see `rcirc-global-log-mentions-mode' to enable/disable
>> +`rcirc-log-mentions-mode' in all rcirc buffers."
>> +  :lighter " Mentions"
>> +  :interactive (rcirc-mode)
>> +  :keymap rcirc-log-mentions-mode-map)
>
> The last :keymap should be superfluous, right?

I guess I just cargo-culted.

>> +(defun rcirc-update-mentions-buffer (type)
>
> This should be documented with a proper docstring!

And/or probably be a "private" function, right?

>> +(defun rcirc-switch-to-mentions-buffer ()
>> +  "Switch to the mentions buffer if it exists.
>> +See `rcirc-log-mentions-mode' and `rcirc-mentions-buffer-name'."
>> +  (interactive)
>> +  (when-let* ((buf (get-buffer rcirc-mentions-buffer-name)))
>> +    (if-let* ((win (get-buffer-window buf)))
>> +        (select-window win)
>
> Could you use `display-buffer-same-window' here?

Wouldn't that show the mentions buffer in the current/selected window?
The point here is to just select-window the window already showing the
mentians buffer if there's any.

>> +      (switch-to-buffer buf))))
>> +
>>  (defvar rcirc-activity-functions nil
>>    "Hook to be run when there is channel activity.
>  
>> @@ -2478,6 +2614,10 @@ activity.  Only run if the buffer is not visible and
>>    (with-current-buffer buffer
>>      (let ((old-activity rcirc-activity)
>>            (old-types rcirc-activity-types))
>> +      (when (and rcirc-log-mentions-mode
>> +                 (memq type '(nick keyword))
>> +                 (not (eq buffer rcirc-server-buffer)))
>> +        (rcirc-update-mentions-buffer type))
>>        (when (and (not (get-buffer-window (current-buffer) t))
>>                   (not (and rcirc-track-ignore-server-buffer-flag
>>                             (eq rcirc-server-buffer (current-buffer)))))
>> -- 
>> 2.45.3
>
> Most of the code looks just fine, my main question is if we really
> want to add this directly to rcirc or if it would be better to have it
> as a extension on ELPA?

My personal preference would be to have it in core, just because it's
simpler to implement, maintain, and document.  And it doesn't feel like
a big burden for the rest of rcirc.  I'd need one or two additional
hooks for implementing it as a package, I think.

While looking at it, I'm not sure it's right that currently the feature
obeys rcirc-ignore-buffer-activity-flag and rcirc-dim-nicks.  I mean, a
mention is a mention and given that the mentions buffer is "for later
reference" and doesn't disturb you in any way, it's probably better to
log those, too, in contrast to the activity mode-line indicator.

Bye,
Tassilo

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.