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

Previous Next

Full log

View this message in rfc822 format

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

Previous Next

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