Refine flycheck-display-errors lifecycle #1972

wyuenho · 2022-08-15T19:44:33Z

This PR fixes a 7-8 year-old bug where the flycheck-display-error-at-point-timer has been calling flycheck-display-errors-function with error overlays marked for deletion, so even when an error has been fixed, this particular code path will still display the fixed error in the echo area.

Reproduction:

Install flycheck, do not configure it at all.
Turn on flycheck-mode on any elisp buffer.
Make a syntax error such as
```
(de |fun foo ())
```
Observe flycheck has highlighted de and fun and there's a reference to free variable 'fun' message displayed in the echo area.
Press backspace to fix the error so it becomes (defun foo ())
Observe the highlighting on is gone on the symbol, but the message is being displayed in the echo area.

Expectation:

After the error has been fixed, the stale error message should not continue to display in the echo area.

cpitclaudel · 2022-08-16T05:31:06Z

I think I see the error; however, I don't think this fix is right: (flycheck-delete-marked-overlays) is delayed to avoid flickering, and indeed this patch reintroduces flickering when a change does not fix an error (the overlays disappear and subsequently reappear).

Additionally, I'm not positive that not showing the error is the right thing to do: if I fix one error but another one persists (until the checker returns), shouldn't we still display that error?

wyuenho · 2022-08-16T11:32:31Z

I see your point. The problem is, there's only flycheck-display-errors-function, but not a flycheck-clear-displayed-errors-function, so all of the flycheck-display-errors-function in the world such as flycheck-postip, flycheck-posframe, flycheck-inline etc will ever only be able to display errors but not clear them when the error was fixed.....

wyuenho · 2022-08-16T14:10:23Z

Ok I've introduced a flycheck-clear-displayed-errors-function variable. Let me know what you think. flycheck-display-error-at-point will now call it if there are no errors to display.

wyuenho · 2022-08-20T20:20:26Z

After playing around with this implementation for a while, I'm quite satisfied with it. An additional benefits to this simple API is now I can finally get the error message to show with flycheck-inline when I hover on the flycheck error without dealing with the perennially broken tooltip-mode.

  (defun flycheck-show-help-function (msg)
    (when flycheck-mode
      (if (null msg)
          (flycheck-clear-displayed-errors)
        (pcase-let* ((`(,frame ,x . ,y) (mouse-position))
                     (win (window-at x y frame))
                     (`(,body-left ,body-top ,@_) (window-body-edges win))
                     (col (max 1 (- x body-left (or display-line-numbers-width 0))))
                     (row (- y body-top)))
          (with-current-buffer (window-buffer win)
            (save-excursion
              (goto-char (point-min))
              (forward-line (1- (+ (line-number-at-pos (window-start win)) row)))
              (move-to-column (1- col))
              (when-let (errors (flycheck-overlay-errors-at (point)))
                (flycheck-display-errors errors))))))))

  (add-hook 'flycheck-mode-hook
            (lambda ()
              (when (display-mouse-p)
                (if flycheck-mode
                    (setq-local show-help-function 'flycheck-show-help-function)
                  (kill-local-variable 'show-help-function)))))

cpitclaudel · 2022-08-25T20:36:35Z

Thanks a lot! I'm traveling this month but I can get to this early September. I hope that's OK.

wyuenho · 2022-08-26T00:10:19Z

Take your time!

wyuenho · 2022-09-24T11:49:02Z

@cpitclaudel have you had a chance to test this out?

wyuenho · 2022-12-15T20:53:16Z

@cpitclaudel ping again

wyuenho · 2023-02-25T20:27:09Z

@cpitclaudel have you had a chance to take a look at this yet?

jcs090218 · 2023-02-25T21:41:01Z

Can you rebase this, so we test it with our latest CI? Thanks!

cc @bbatsov for the review, another maintainer has the push permission.

bbatsov · 2023-02-25T21:55:25Z

I'd also suggest squashing the related commits together for the review.

wyuenho · 2023-02-26T15:06:15Z

Rebased. I'm not sure what Eask is and why it is failing to download packages and breaking the tests.

I'll squash after review so you have a history to look at.

bbatsov · 2023-02-26T15:50:43Z

Rebased. I'm not sure what Eask is and why it is failing to download packages and breaking the tests.

It's replacement for Cask that's actively maintained.

bbatsov · 2023-02-26T15:53:03Z

flycheck.el

@@ -3329,6 +3341,7 @@ current syntax check."
 (flycheck-stop))
 (flycheck-delete-all-overlays)
 (flycheck-clear-errors)
+ (flycheck-clear-displayed-error-messages)


Isn't this something that flycheck-clear-errors should normally be doing? I find it weird to have to clear the errors and the displayed error messages separately.

Same reason as #1972 (comment). Clearing the errors data structures and displayed messages have to be distinct in order to prevent flickering.

bbatsov · 2023-02-26T15:54:08Z

flycheck.el

@@ -5493,6 +5506,11 @@ non-nil."
 (when flycheck-display-errors-function
 (funcall flycheck-display-errors-function errors)))

+(defun flycheck-clear-displayed-errors ()
+ "Clear errors using `flycheck-clear-displayed-errors-function'."
+ (when flycheck-clear-displayed-errors-function


Do we really need this to be customizable? Why would users want a different behaviour?

Not all error messages are displayed the same way. There are many packages that adjusts how the messages are displayed, some use childframes, others use overlays. Displaying and clearing messages have to be a symmetric operation operating on the same data type in order to be reliable.

See #1972 (comment)

Also, see a usage example here to enable a config such as the above

jcs090218 · 2023-02-26T18:11:24Z

The CI is failing due to the upstream changes, I've opened the PR for the fix, see #2007.

@bbatsov Can you review and merge this? Thanks!

Sorry for the inconvenience! 😓

bbatsov · 2023-02-26T21:06:15Z

The fix is merged.

cpitclaudel · 2023-02-27T23:20:33Z

This is a subtle part of flycheck. I'm very thankful for the changes and I'm sorry I haven't gotten around to reviewing them :'( Hopefully @bbatsov and @jcs090218 can test / review and report.

wyuenho · 2023-04-10T10:37:49Z

@bbatsov have I addressed your questions?

bbatsov · 2024-02-04T19:11:16Z

@wyuenho Can you rebase on master, so I can see what's the current state of the CI?

bbatsov · 2024-02-04T19:11:42Z

flycheck.el

+ :type '(choice (const :tag "Clear displayed error messages"
+ flycheck-clear-displayed-error-messages)
+ (function :tag "Clear displayed errors function"))
+ :package-version '(flycheck . "33")


This should be 34, now that 33 is out.

bbatsov · 2024-02-04T19:12:24Z

Before we can merge this we also need:

a changelog entry
some entry in the user docs, so people would actual find out about this new configuration variable

…isplay-error-at-point a chance to clear errors

…oint so it can clear the displayed error

bbatsov · 2024-02-10T10:51:00Z

flycheck.el

@@ -5605,6 +5613,14 @@ Hide the error buffer if there is no error under point."
 (save-selected-window
 (quit-window nil window)))))

+(defun flycheck-clear-displayed-error-messages ()
+ "Clear error messages displayed by `flycheck-display-error-messages'."
+ (unless (null flycheck--last-displayed-message)


Maybe use when here instead?

bbatsov · 2024-02-10T10:51:19Z

flycheck.el

+ "Clear error messages displayed by `flycheck-display-error-messages'."
+ (unless (null flycheck--last-displayed-message)
+ (if (and (stringp flycheck--last-displayed-message)
+ (equal (current-message) flycheck--last-displayed-message))


Why not do a string comparison instead?

bbatsov · 2024-02-10T10:52:00Z

flycheck.el

@@ -5561,6 +5565,8 @@ and if the echo area is not occupied by minibuffer input."
 "Flycheck error messages"
 "Major mode for extended error messages.")

+(defvar flycheck--last-displayed-message nil)


I'd add a docstring here.

bbatsov · 2024-02-10T10:53:08Z

I left a bit of extra feedback. You'll also need to rebase this on top of master, as I was doing a lot of changes lately.

bbatsov · 2024-02-24T07:19:05Z

@wyuenho Friendly ping :-)

wyuenho force-pushed the delete-marked-overlays-before-displaying-errors branch from e56ff12 to bb5f055 Compare August 16, 2022 14:08

wyuenho mentioned this pull request Aug 16, 2022

Use flycheck-clear-displayed-errors-function flycheck/flycheck-inline#22

Open

wyuenho changed the title ~~Delete overlays marked for deletion before finding errors from existing overlays~~ Refine flycheck-display-errors lifecycle Aug 20, 2022

wyuenho force-pushed the delete-marked-overlays-before-displaying-errors branch 2 times, most recently from cfa2aae to 01eaef9 Compare November 4, 2022 20:59

wyuenho force-pushed the delete-marked-overlays-before-displaying-errors branch from 01eaef9 to f470739 Compare November 21, 2022 17:36

wyuenho force-pushed the delete-marked-overlays-before-displaying-errors branch from f470739 to ddbd8e3 Compare December 15, 2022 20:31

wyuenho force-pushed the delete-marked-overlays-before-displaying-errors branch from ddbd8e3 to a18db30 Compare February 26, 2023 14:50

bbatsov reviewed Feb 26, 2023

View reviewed changes

wyuenho requested a review from bbatsov April 10, 2023 10:38

bbatsov reviewed Feb 4, 2024

View reviewed changes

wyuenho added 7 commits February 4, 2024 19:49

Introduce flycheck-clear-displayed-errors-function to give flycheck-d…

d311234

…isplay-error-at-point a chance to clear errors

clear displayed errors at teardown as well

4d2a09f

delete dead code

34b789b

move flycheck-clear-displayed-error-messages to flycheck-clear

96ec3c2

run flycheck-display-error-at-point even when there's no overlay at p…

6604038

…oint so it can clear the displayed error

no need to check buffer-modified-tick

265e18b

don't wash away message not displayed by flycheck

a3ea819

wyuenho force-pushed the delete-marked-overlays-before-displaying-errors branch from a18db30 to a3ea819 Compare February 4, 2024 19:49

bbatsov reviewed Feb 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine flycheck-display-errors lifecycle #1972

Refine flycheck-display-errors lifecycle #1972

wyuenho commented Aug 15, 2022

cpitclaudel commented Aug 16, 2022

wyuenho commented Aug 16, 2022 •

edited

wyuenho commented Aug 16, 2022

wyuenho commented Aug 20, 2022 •

edited

cpitclaudel commented Aug 25, 2022

wyuenho commented Aug 26, 2022

wyuenho commented Sep 24, 2022

wyuenho commented Dec 15, 2022

wyuenho commented Feb 25, 2023

jcs090218 commented Feb 25, 2023

bbatsov commented Feb 25, 2023

wyuenho commented Feb 26, 2023

bbatsov commented Feb 26, 2023

bbatsov Feb 26, 2023

wyuenho Feb 27, 2023

bbatsov Feb 26, 2023

wyuenho Feb 27, 2023 •

edited

wyuenho Feb 27, 2023 •

edited

jcs090218 commented Feb 26, 2023

bbatsov commented Feb 26, 2023

cpitclaudel commented Feb 27, 2023

wyuenho commented Apr 10, 2023

bbatsov commented Feb 4, 2024

bbatsov Feb 4, 2024

bbatsov commented Feb 4, 2024

bbatsov Feb 10, 2024

bbatsov Feb 10, 2024

bbatsov Feb 10, 2024

bbatsov commented Feb 10, 2024

bbatsov commented Feb 24, 2024

Refine flycheck-display-errors lifecycle #1972

Are you sure you want to change the base?

Refine flycheck-display-errors lifecycle #1972

Conversation

wyuenho commented Aug 15, 2022

cpitclaudel commented Aug 16, 2022

wyuenho commented Aug 16, 2022 • edited

wyuenho commented Aug 16, 2022

wyuenho commented Aug 20, 2022 • edited

cpitclaudel commented Aug 25, 2022

wyuenho commented Aug 26, 2022

wyuenho commented Sep 24, 2022

wyuenho commented Dec 15, 2022

wyuenho commented Feb 25, 2023

jcs090218 commented Feb 25, 2023

bbatsov commented Feb 25, 2023

wyuenho commented Feb 26, 2023

bbatsov commented Feb 26, 2023

bbatsov Feb 26, 2023

Choose a reason for hiding this comment

wyuenho Feb 27, 2023

Choose a reason for hiding this comment

bbatsov Feb 26, 2023

Choose a reason for hiding this comment

wyuenho Feb 27, 2023 • edited

Choose a reason for hiding this comment

wyuenho Feb 27, 2023 • edited

Choose a reason for hiding this comment

jcs090218 commented Feb 26, 2023

bbatsov commented Feb 26, 2023

cpitclaudel commented Feb 27, 2023

wyuenho commented Apr 10, 2023

bbatsov commented Feb 4, 2024

bbatsov Feb 4, 2024

Choose a reason for hiding this comment

bbatsov commented Feb 4, 2024

bbatsov Feb 10, 2024

Choose a reason for hiding this comment

bbatsov Feb 10, 2024

Choose a reason for hiding this comment

bbatsov Feb 10, 2024

Choose a reason for hiding this comment

bbatsov commented Feb 10, 2024

bbatsov commented Feb 24, 2024

wyuenho commented Aug 16, 2022 •

edited

wyuenho commented Aug 20, 2022 •

edited

wyuenho Feb 27, 2023 •

edited

wyuenho Feb 27, 2023 •

edited