GNU bug report logs - #64428
[PATCH] Fix flymake mode line scrolling with pixel-scroll-precision-mode

Previous Next

Package: emacs;

Reported by: sbaugh <at> catern.com

Date: Sun, 2 Jul 2023 21:52:02 UTC

Severity: normal

Tags: patch

Done: Eli Zaretskii <eliz <at> gnu.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 64428 in the body.
You can then email your comments to 64428 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#64428; Package emacs. (Sun, 02 Jul 2023 21:52:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to sbaugh <at> catern.com:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Sun, 02 Jul 2023 21:52:02 GMT) Full text and rfc822 format available.

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

From: sbaugh <at> catern.com
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fix flymake mode line scrolling with
 pixel-scroll-precision-mode
Date: Sun, 02 Jul 2023 21:51:01 +0000 (UTC)
[Message part 1 (text/plain, inline)]
Tags: patch


This patch is based on the patch in bug#64425 (since I noticed this
unrelated issue while working on that, and they touch the same code).



When pixel-scroll-precision-mode is enabled, scrolling the mouse wheel
will yield wheel-{up,down} events.  flymake now binds the new events
in addition to the old mouse-wheel-{up,down}-event.

* lisp/progmodes/flymake.el:(flymake--mode-line-counter-scroll-prev,
flymake--mode-line-counter-scroll-next,
flymake--mode-line-counter-map): Add.
(flymake--mode-line-counter): Use new keymap and include 'type as a
property in the mode-line.


In GNU Emacs 29.0.92 (build 8, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw3d scroll bars) of 2023-07-02 built on earth
Repository revision: c361966508e2da159b5e65c37dff7f78e87b3445
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: NixOS 23.05 (Stoat)

Configured using:
 'configure --with-x-toolkit=lucid --with-tree-sitter --with-xinput2'

[0001-Fix-flymake-mode-line-scrolling-with-pixel-scroll-pr.patch (text/patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64428; Package emacs. (Thu, 06 Jul 2023 07:38:01 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: sbaugh <at> catern.com
Cc: 64428 <at> debbugs.gnu.org
Subject: Re: bug#64428: [PATCH] Fix flymake mode line scrolling with
 pixel-scroll-precision-mode
Date: Thu, 06 Jul 2023 10:37:07 +0300
> From: sbaugh <at> catern.com
> Date: Sun, 02 Jul 2023 21:51:01 +0000 (UTC)
> 
> This patch is based on the patch in bug#64425 (since I noticed this
> unrelated issue while working on that, and they touch the same code).
> 
> 
> 
> When pixel-scroll-precision-mode is enabled, scrolling the mouse wheel
> will yield wheel-{up,down} events.  flymake now binds the new events
> in addition to the old mouse-wheel-{up,down}-event.
> 
> * lisp/progmodes/flymake.el:(flymake--mode-line-counter-scroll-prev,
> flymake--mode-line-counter-scroll-next,
> flymake--mode-line-counter-map): Add.
> (flymake--mode-line-counter): Use new keymap and include 'type as a
> property in the mode-line.

Thanks.  Any reason you couldn't simply add more events to the
existing code?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64428; Package emacs. (Thu, 06 Jul 2023 13:17:02 GMT) Full text and rfc822 format available.

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: sbaugh <at> catern.com, 64428 <at> debbugs.gnu.org
Subject: Re: bug#64428: [PATCH] Fix flymake mode line scrolling with
 pixel-scroll-precision-mode
Date: Thu, 06 Jul 2023 09:16:41 -0400
Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: sbaugh <at> catern.com
>> Date: Sun, 02 Jul 2023 21:51:01 +0000 (UTC)
>> 
>> This patch is based on the patch in bug#64425 (since I noticed this
>> unrelated issue while working on that, and they touch the same code).
>> 
>> 
>> 
>> When pixel-scroll-precision-mode is enabled, scrolling the mouse wheel
>> will yield wheel-{up,down} events.  flymake now binds the new events
>> in addition to the old mouse-wheel-{up,down}-event.
>> 
>> * lisp/progmodes/flymake.el:(flymake--mode-line-counter-scroll-prev,
>> flymake--mode-line-counter-scroll-next,
>> flymake--mode-line-counter-map): Add.
>> (flymake--mode-line-counter): Use new keymap and include 'type as a
>> property in the mode-line.
>
> Thanks.  Any reason you couldn't simply add more events to the
> existing code?

Two reasons:

1. I initially did that but it made the code uglier.  Also, this code is
run every time the mode-line is updated, and adding more of that seems
bad.

2. This makes describe-key work better and makes it possible for users
to configure the scroll direction or add more bindings for different
things.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64428; Package emacs. (Thu, 06 Jul 2023 14:05:02 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>, João Távora
 <joaotavora <at> gmail.com>
Cc: sbaugh <at> catern.com, 64428 <at> debbugs.gnu.org
Subject: Re: bug#64428: [PATCH] Fix flymake mode line scrolling with
 pixel-scroll-precision-mode
Date: Thu, 06 Jul 2023 17:04:39 +0300
> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: sbaugh <at> catern.com,  64428 <at> debbugs.gnu.org
> Date: Thu, 06 Jul 2023 09:16:41 -0400
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> * lisp/progmodes/flymake.el:(flymake--mode-line-counter-scroll-prev,
> >> flymake--mode-line-counter-scroll-next,
> >> flymake--mode-line-counter-map): Add.
> >> (flymake--mode-line-counter): Use new keymap and include 'type as a
> >> property in the mode-line.
> >
> > Thanks.  Any reason you couldn't simply add more events to the
> > existing code?
> 
> Two reasons:
> 
> 1. I initially did that but it made the code uglier.  Also, this code is
> run every time the mode-line is updated, and adding more of that seems
> bad.
> 
> 2. This makes describe-key work better and makes it possible for users
> to configure the scroll direction or add more bindings for different
> things.

This means your changeset is actually two different loosely-related
changes.  The one that refactors the original code needs approval from
João (CC'ed).  Once João is happy with that refactoring, I'm okay with
adding the extra events to it.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64428; Package emacs. (Fri, 07 Jul 2023 11:39:02 GMT) Full text and rfc822 format available.

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

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Spencer Baugh <sbaugh <at> janestreet.com>, 64428 <at> debbugs.gnu.org,
 sbaugh <at> catern.com
Subject: Re: bug#64428: [PATCH] Fix flymake mode line scrolling with
 pixel-scroll-precision-mode
Date: Fri, 7 Jul 2023 12:37:44 +0100
On Thu, Jul 6, 2023 at 3:04 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Spencer Baugh <sbaugh <at> janestreet.com>
> > Cc: sbaugh <at> catern.com,  64428 <at> debbugs.gnu.org
> > Date: Thu, 06 Jul 2023 09:16:41 -0400
> >
> > Eli Zaretskii <eliz <at> gnu.org> writes:
> >
> > >> * lisp/progmodes/flymake.el:(flymake--mode-line-counter-scroll-prev,
> > >> flymake--mode-line-counter-scroll-next,
> > >> flymake--mode-line-counter-map): Add.
> > >> (flymake--mode-line-counter): Use new keymap and include 'type as a
> > >> property in the mode-line.
> > >
> > > Thanks.  Any reason you couldn't simply add more events to the
> > > existing code?
> >
> > Two reasons:
> >
> > 1. I initially did that but it made the code uglier.  Also, this code is
> > run every time the mode-line is updated, and adding more of that seems
> > bad.
> >
> > 2. This makes describe-key work better and makes it possible for users
> > to configure the scroll direction or add more bindings for different
> > things.
>
> This means your changeset is actually two different loosely-related
> changes.  The one that refactors the original code needs approval from
> João (CC'ed).  Once João is happy with that refactoring, I'm okay with
> adding the extra events to it.

Hi Eli, Spencer,

I've reviewed the code.  I understand Spencer's rationale for the
small refactoring and I'm OK with it.

However I didn't test.  I'd say it's better for this new capability to
go to master, though with some minimal testing (say, at least in tty
as well as graphical mode) it shouldn't be really dangerous for
emacs-29 either.

João




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64428; Package emacs. (Fri, 07 Jul 2023 11:48:02 GMT) Full text and rfc822 format available.

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

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Spencer Baugh <sbaugh <at> janestreet.com>, 64428 <at> debbugs.gnu.org,
 sbaugh <at> catern.com
Subject: Re: bug#64428: [PATCH] Fix flymake mode line scrolling with
 pixel-scroll-precision-mode
Date: Fri, 7 Jul 2023 12:47:10 +0100
On second thought, here are some comments that I think should be
improved in Spencer's patch:

> @@ -1479,21 +1505,8 @@ flymake--mode-line-counter
>                               ((eq type :warning) "warnings")
>                               ((eq type :note) "notes")
>                               (t (format "%s diagnostics" type))))
> -         keymap
> -         ,(let ((map (make-sparse-keymap)))
> -            (define-key map (vector 'mode-line
> -                                    mouse-wheel-down-event)
> -              (lambda (event)
> -                (interactive "e")
> -                (with-selected-window (posn-window (event-start event))
> -                  (flymake-goto-prev-error 1 (list type) t))))
> -            (define-key map (vector 'mode-line
> -                                    mouse-wheel-up-event)
> -              (lambda (event)
> -                (interactive "e")
> -                (with-selected-window (posn-window (event-start event))
> -                  (flymake-goto-next-error 1 (list type) t))))
> -            map))))))
> +         type ,type

Spencer, here you are recording the value of the `type` in a `type`
text-property of the affected text.  Generally, though this rule
isn't enforced or always followed (at least by me), it's better
to give these package-specific properties some longer
package-specific name like `flymake--diagnostic-type`.  This will
prevent any clashes if the less-qualified `type` is ever defined
to mean something else as a text-property.

> +  (interactive "e")
> +  (let* ((posn-string (posn-string (event-start event)))
> +         (type (get-text-property (cdr posn-string) 'type (car posn-string))))
> +    (with-selected-window (posn-window (event-start event))
> +      (flymake-goto-prev-error 1 (list type) t))))

And here, you could consider saving the value of (event-start event)
by adding another early binding to that `let*`, maybe call it `estart`.
This is much less important than the first comment though.

João




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64428; Package emacs. (Fri, 07 Jul 2023 12:13:02 GMT) Full text and rfc822 format available.

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

From: sbaugh <at> catern.com
To: João Távora <joaotavora <at> gmail.com>
Cc: Spencer Baugh <sbaugh <at> janestreet.com>, Eli Zaretskii <eliz <at> gnu.org>,
 64428 <at> debbugs.gnu.org
Subject: Re: bug#64428: [PATCH] Fix flymake mode line scrolling with
 pixel-scroll-precision-mode
Date: Fri, 07 Jul 2023 12:12:32 +0000 (UTC)
[Message part 1 (text/plain, inline)]
João Távora <joaotavora <at> gmail.com> writes:

> On second thought, here are some comments that I think should be
> improved in Spencer's patch:
>
>> @@ -1479,21 +1505,8 @@ flymake--mode-line-counter
>>                               ((eq type :warning) "warnings")
>>                               ((eq type :note) "notes")
>>                               (t (format "%s diagnostics" type))))
>> -         keymap
>> -         ,(let ((map (make-sparse-keymap)))
>> -            (define-key map (vector 'mode-line
>> -                                    mouse-wheel-down-event)
>> -              (lambda (event)
>> -                (interactive "e")
>> -                (with-selected-window (posn-window (event-start event))
>> -                  (flymake-goto-prev-error 1 (list type) t))))
>> -            (define-key map (vector 'mode-line
>> -                                    mouse-wheel-up-event)
>> -              (lambda (event)
>> -                (interactive "e")
>> -                (with-selected-window (posn-window (event-start event))
>> -                  (flymake-goto-next-error 1 (list type) t))))
>> -            map))))))
>> +         type ,type
>
> Spencer, here you are recording the value of the `type` in a `type`
> text-property of the affected text.  Generally, though this rule
> isn't enforced or always followed (at least by me), it's better
> to give these package-specific properties some longer
> package-specific name like `flymake--diagnostic-type`.  This will
> prevent any clashes if the less-qualified `type` is ever defined
> to mean something else as a text-property.
>
>> +  (interactive "e")
>> +  (let* ((posn-string (posn-string (event-start event)))
>> +         (type (get-text-property (cdr posn-string) 'type (car posn-string))))
>> +    (with-selected-window (posn-window (event-start event))
>> +      (flymake-goto-prev-error 1 (list type) t))))
>
> And here, you could consider saving the value of (event-start event)
> by adding another early binding to that `let*`, maybe call it `estart`.
> This is much less important than the first comment though.
>
> João

Fixed.

I have tested in both graphical and tty Emacs.

[0001-Fix-flymake-mode-line-scrolling-with-pixel-scroll-pr.patch (text/x-patch, inline)]
From c369c3b99ef1e5f9eab29f99a9d4f354352ef05b Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> catern.com>
Date: Sun, 2 Jul 2023 17:49:23 -0400
Subject: [PATCH] Fix flymake mode line scrolling with
 pixel-scroll-precision-mode

When pixel-scroll-precision-mode is enabled, scrolling the mouse wheel
will yield wheel-{up,down} events.  flymake now binds the new events
in addition to the old mouse-wheel-{up,down}-event.

* lisp/progmodes/flymake.el:(flymake--mode-line-counter-scroll-prev,
flymake--mode-line-counter-scroll-next,
flymake--mode-line-counter-map): Add.
(flymake--mode-line-counter): Use new keymap and include
flymake--diagnostic-type as a property in the mode-line.
---
 lisp/progmodes/flymake.el | 47 ++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index 47dc32f9245..17154b646d7 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -1449,6 +1449,36 @@ flymake--mode-line-exception
 (defun flymake--mode-line-counters ()
   (when (flymake-running-backends) flymake-mode-line-counter-format))
 
+(defun flymake--mode-line-counter-scroll-prev (event)
+  (interactive "e")
+  (let* ((event-start (event-start event))
+         (posn-string (posn-string event-start))
+         (type (get-text-property
+                (cdr posn-string) 'flymake--diagnostic-type (car posn-string))))
+    (with-selected-window (posn-window event-start)
+      (flymake-goto-prev-error 1 (list type) t))))
+
+(defun flymake--mode-line-counter-scroll-next (event)
+  (interactive "e")
+  (let* ((event-start (event-start event))
+         (posn-string (posn-string event-start))
+         (type (get-text-property
+                (cdr posn-string) 'flymake--diagnostic-type (car posn-string))))
+    (with-selected-window (posn-window event-start)
+      (flymake-goto-next-error 1 (list type) t))))
+
+(defvar flymake--mode-line-counter-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map (vector 'mode-line mouse-wheel-down-event)
+                #'flymake--mode-line-counter-scroll-prev)
+    (define-key map [mode-line wheel-down]
+                #'flymake--mode-line-counter-scroll-prev)
+    (define-key map (vector 'mode-line mouse-wheel-up-event)
+                #'flymake--mode-line-counter-scroll-next)
+    (define-key map [mode-line wheel-up]
+                #'flymake--mode-line-counter-scroll-next)
+    map))
+
 (defun flymake--mode-line-counter (type &optional no-space)
   "Compute number of diagnostics in buffer with TYPE's severity.
 TYPE is usually keyword `:error', `:warning' or `:note'."
@@ -1479,21 +1509,8 @@ flymake--mode-line-counter
                              ((eq type :warning) "warnings")
                              ((eq type :note) "notes")
                              (t (format "%s diagnostics" type))))
-         keymap
-         ,(let ((map (make-sparse-keymap)))
-            (define-key map (vector 'mode-line
-                                    mouse-wheel-down-event)
-              (lambda (event)
-                (interactive "e")
-                (with-selected-window (posn-window (event-start event))
-                  (flymake-goto-prev-error 1 (list type) t))))
-            (define-key map (vector 'mode-line
-                                    mouse-wheel-up-event)
-              (lambda (event)
-                (interactive "e")
-                (with-selected-window (posn-window (event-start event))
-                  (flymake-goto-next-error 1 (list type) t))))
-            map))))))
+         flymake--diagnostic-type ,type
+         keymap ,flymake--mode-line-counter-map)))))
 
 ;;; Per-buffer diagnostic listing
 
-- 
2.41.0


Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64428; Package emacs. (Fri, 07 Jul 2023 12:54:01 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: João Távora <joaotavora <at> gmail.com>
Cc: sbaugh <at> janestreet.com, 64428 <at> debbugs.gnu.org, sbaugh <at> catern.com
Subject: Re: bug#64428: [PATCH] Fix flymake mode line scrolling with
 pixel-scroll-precision-mode
Date: Fri, 07 Jul 2023 15:52:59 +0300
> From: João Távora <joaotavora <at> gmail.com>
> Date: Fri, 7 Jul 2023 12:37:44 +0100
> Cc: Spencer Baugh <sbaugh <at> janestreet.com>, sbaugh <at> catern.com, 64428 <at> debbugs.gnu.org
> 
> I've reviewed the code.  I understand Spencer's rationale for the
> small refactoring and I'm OK with it.
> 
> However I didn't test.  I'd say it's better for this new capability to
> go to master, though with some minimal testing (say, at least in tty
> as well as graphical mode) it shouldn't be really dangerous for
> emacs-29 either.

Thanks.  It will definitely go to master, not to emacs-29.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64428; Package emacs. (Thu, 13 Jul 2023 05:57:01 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: joaotavora <at> gmail.com, sbaugh <at> catern.com
Cc: sbaugh <at> janestreet.com, 64428 <at> debbugs.gnu.org
Subject: Re: bug#64428: [PATCH] Fix flymake mode line scrolling with
 pixel-scroll-precision-mode
Date: Thu, 13 Jul 2023 08:56:44 +0300
> From: sbaugh <at> catern.com
> Date: Fri, 07 Jul 2023 12:12:32 +0000 (UTC)
> Cc: Eli Zaretskii <eliz <at> gnu.org>, Spencer Baugh <sbaugh <at> janestreet.com>,
> 	64428 <at> debbugs.gnu.org
> 
> João Távora <joaotavora <at> gmail.com> writes:
> 
> > On second thought, here are some comments that I think should be
> > improved in Spencer's patch:
> >
> >> @@ -1479,21 +1505,8 @@ flymake--mode-line-counter
> >>                               ((eq type :warning) "warnings")
> >>                               ((eq type :note) "notes")
> >>                               (t (format "%s diagnostics" type))))
> >> -         keymap
> >> -         ,(let ((map (make-sparse-keymap)))
> >> -            (define-key map (vector 'mode-line
> >> -                                    mouse-wheel-down-event)
> >> -              (lambda (event)
> >> -                (interactive "e")
> >> -                (with-selected-window (posn-window (event-start event))
> >> -                  (flymake-goto-prev-error 1 (list type) t))))
> >> -            (define-key map (vector 'mode-line
> >> -                                    mouse-wheel-up-event)
> >> -              (lambda (event)
> >> -                (interactive "e")
> >> -                (with-selected-window (posn-window (event-start event))
> >> -                  (flymake-goto-next-error 1 (list type) t))))
> >> -            map))))))
> >> +         type ,type
> >
> > Spencer, here you are recording the value of the `type` in a `type`
> > text-property of the affected text.  Generally, though this rule
> > isn't enforced or always followed (at least by me), it's better
> > to give these package-specific properties some longer
> > package-specific name like `flymake--diagnostic-type`.  This will
> > prevent any clashes if the less-qualified `type` is ever defined
> > to mean something else as a text-property.
> >
> >> +  (interactive "e")
> >> +  (let* ((posn-string (posn-string (event-start event)))
> >> +         (type (get-text-property (cdr posn-string) 'type (car posn-string))))
> >> +    (with-selected-window (posn-window (event-start event))
> >> +      (flymake-goto-prev-error 1 (list type) t))))
> >
> > And here, you could consider saving the value of (event-start event)
> > by adding another early binding to that `let*`, maybe call it `estart`.
> > This is much less important than the first comment though.
> >
> > João
> 
> Fixed.
> 
> I have tested in both graphical and tty Emacs.

Thanks.  João, is this good to go, from your POV?

I admit I consider it a bit strange to have commands that are
"internal" and don't have a doc string:

> +(defun flymake--mode-line-counter-scroll-prev (event)
> +  (interactive "e")
> +  (let* ((event-start (event-start event))
> +         (posn-string (posn-string event-start))
> +         (type (get-text-property
> +                (cdr posn-string) 'flymake--diagnostic-type (car posn-string))))
> +    (with-selected-window (posn-window event-start)
> +      (flymake-goto-prev-error 1 (list type) t))))
> +
> +(defun flymake--mode-line-counter-scroll-next (event)
> +  (interactive "e")
> +  (let* ((event-start (event-start event))
> +         (posn-string (posn-string event-start))
> +         (type (get-text-property
> +                (cdr posn-string) 'flymake--diagnostic-type (car posn-string))))
> +    (with-selected-window (posn-window event-start)
> +      (flymake-goto-next-error 1 (list type) t))))

Any reasons not to make them public and add doc strings?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#64428; Package emacs. (Thu, 13 Jul 2023 08:13:01 GMT) Full text and rfc822 format available.

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

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: sbaugh <at> catern.com, 64428 <at> debbugs.gnu.org,
 Spencer Baugh <sbaugh <at> janestreet.com>
Subject: Re: bug#64428: [PATCH] Fix flymake mode line scrolling with
 pixel-scroll-precision-mode
Date: Thu, 13 Jul 2023 09:12:33 +0100
[Message part 1 (text/plain, inline)]
On Thu, Jul 13, 2023, 06:56 Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: sbaugh <at> catern.com
> > Date: Fri, 07 Jul 2023 12:12:32 +0000 (UTC)
> > Cc: Eli Zaretskii <eliz <at> gnu.org>, Spencer Baugh <sbaugh <at> janestreet.com>,
> >       64428 <at> debbugs.gnu.org
> >
> > João Távora <joaotavora <at> gmail.com> writes:
> >
> > > On second thought, here are some comments that I think should be
> > > improved in Spencer's patch:
> > >
> > >> @@ -1479,21 +1505,8 @@ flymake--mode-line-counter
> > >>                               ((eq type :warning) "warnings")
> > >>                               ((eq type :note) "notes")
> > >>                               (t (format "%s diagnostics" type))))
> > >> -         keymap
> > >> -         ,(let ((map (make-sparse-keymap)))
> > >> -            (define-key map (vector 'mode-line
> > >> -                                    mouse-wheel-down-event)
> > >> -              (lambda (event)
> > >> -                (interactive "e")
> > >> -                (with-selected-window (posn-window (event-start
> event))
> > >> -                  (flymake-goto-prev-error 1 (list type) t))))
> > >> -            (define-key map (vector 'mode-line
> > >> -                                    mouse-wheel-up-event)
> > >> -              (lambda (event)
> > >> -                (interactive "e")
> > >> -                (with-selected-window (posn-window (event-start
> event))
> > >> -                  (flymake-goto-next-error 1 (list type) t))))
> > >> -            map))))))
> > >> +         type ,type
> > >
> > > Spencer, here you are recording the value of the `type` in a `type`
> > > text-property of the affected text.  Generally, though this rule
> > > isn't enforced or always followed (at least by me), it's better
> > > to give these package-specific properties some longer
> > > package-specific name like `flymake--diagnostic-type`.  This will
> > > prevent any clashes if the less-qualified `type` is ever defined
> > > to mean something else as a text-property.
> > >
> > >> +  (interactive "e")
> > >> +  (let* ((posn-string (posn-string (event-start event)))
> > >> +         (type (get-text-property (cdr posn-string) 'type (car
> posn-string))))
> > >> +    (with-selected-window (posn-window (event-start event))
> > >> +      (flymake-goto-prev-error 1 (list type) t))))
> > >
> > > And here, you could consider saving the value of (event-start event)
> > > by adding another early binding to that `let*`, maybe call it `estart`.
> > > This is much less important than the first comment though.
> > >
> > > João
> >
> > Fixed.
> >
> > I have tested in both graphical and tty Emacs.
>
> Thanks.  João, is this good to go, from your POV?
>
> I admit I consider it a bit strange to have commands that are
> "internal" and don't have a doc string:
>
> > +(defun flymake--mode-line-counter-scroll-prev (event)
> > +  (interactive "e")
> > +  (let* ((event-start (event-start event))
> > +         (posn-string (posn-string event-start))
> > +         (type (get-text-property
> > +                (cdr posn-string) 'flymake--diagnostic-type (car
> posn-string))))
> > +    (with-selected-window (posn-window event-start)
> > +      (flymake-goto-prev-error 1 (list type) t))))
> > +
> > +(defun flymake--mode-line-counter-scroll-next (event)
> > +  (interactive "e")
> > +  (let* ((event-start (event-start event))
> > +         (posn-string (posn-string event-start))
> > +         (type (get-text-property
> > +                (cdr posn-string) 'flymake--diagnostic-type (car
> posn-string))))
> > +    (with-selected-window (posn-window event-start)
> > +      (flymake-goto-next-error 1 (list type) t))))
>
> Any reasons not to make them public and add doc strings?
>

If you can give a reasonable example of sonething the general public would
want to do with them, ok.  Until then, I think public interfaces should be
kept small.

Yes, it's good for pushing.

João

>
[Message part 2 (text/html, inline)]

Reply sent to Eli Zaretskii <eliz <at> gnu.org>:
You have taken responsibility. (Thu, 13 Jul 2023 14:01:01 GMT) Full text and rfc822 format available.

Notification sent to sbaugh <at> catern.com:
bug acknowledged by developer. (Thu, 13 Jul 2023 14:01:02 GMT) Full text and rfc822 format available.

Message #37 received at 64428-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: João Távora <joaotavora <at> gmail.com>
Cc: sbaugh <at> catern.com, 64428-done <at> debbugs.gnu.org, sbaugh <at> janestreet.com
Subject: Re: bug#64428: [PATCH] Fix flymake mode line scrolling with
 pixel-scroll-precision-mode
Date: Thu, 13 Jul 2023 17:00:43 +0300
> From: João Távora <joaotavora <at> gmail.com>
> Date: Thu, 13 Jul 2023 09:12:33 +0100
> Cc: sbaugh <at> catern.com, Spencer Baugh <sbaugh <at> janestreet.com>, 64428 <at> debbugs.gnu.org
> 
>  Any reasons not to make them public and add doc strings?
> 
> If you can give a reasonable example of sonething the general public would want to do with them, ok. 
> Until then, I think public interfaces should be kept small.
> 
> Yes, it's good for pushing.

Thanks, installed on master, and closing the bug.

Spencer, please in the future try to follow closer our conventions of
formatting commit log messages.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Fri, 11 Aug 2023 11:24:07 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 6 days ago.

Previous Next


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