-
-
Notifications
You must be signed in to change notification settings - Fork 445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refine flycheck-display-errors lifecycle #1972
base: master
Are you sure you want to change the base?
Refine flycheck-display-errors lifecycle #1972
Conversation
I think I see the error; however, I don't think this fix is right: 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? |
I see your point. The problem is, there's only |
e56ff12
to
bb5f055
Compare
Ok I've introduced a |
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 (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))))) |
Thanks a lot! I'm traveling this month but I can get to this early September. I hope that's OK. |
Take your time! |
@cpitclaudel have you had a chance to test this out? |
cfa2aae
to
01eaef9
Compare
01eaef9
to
f470739
Compare
f470739
to
ddbd8e3
Compare
@cpitclaudel ping again |
@cpitclaudel have you had a chance to take a look at this yet? |
Can you rebase this, so we test it with our latest CI? Thanks! cc @bbatsov for the review, another maintainer has the push permission. |
I'd also suggest squashing the related commits together for the review. |
ddbd8e3
to
a18db30
Compare
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. |
It's replacement for Cask that's actively maintained. |
@@ -3329,6 +3341,7 @@ current syntax check." | |||
(flycheck-stop)) | |||
(flycheck-delete-all-overlays) | |||
(flycheck-clear-errors) | |||
(flycheck-clear-displayed-error-messages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason as #1972 (comment). Clearing the errors data structures and displayed messages have to be distinct in order to prevent flickering.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this to be customizable? Why would users want a different behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is merged. |
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. |
@bbatsov have I addressed your questions? |
@wyuenho Can you rebase on |
:type '(choice (const :tag "Clear displayed error messages" | ||
flycheck-clear-displayed-error-messages) | ||
(function :tag "Clear displayed errors function")) | ||
:package-version '(flycheck . "33") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be 34, now that 33 is out.
Before we can merge this we also need:
|
…isplay-error-at-point a chance to clear errors
…oint so it can clear the displayed error
a18db30
to
a3ea819
Compare
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use when
here instead?
"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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not do a string comparison instead?
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a docstring here.
I left a bit of extra feedback. You'll also need to rebase this on top of |
@wyuenho Friendly ping :-) |
This PR fixes a 7-8 year-old bug where the
flycheck-display-error-at-point-timer
has been callingflycheck-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:
flycheck-mode
on any elisp buffer.de
andfun
and there's areference to free variable 'fun'
message displayed in the echo area.(defun foo ())
Expectation:
After the error has been fixed, the stale error message should not continue to display in the echo area.