Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refine flycheck-display-errors lifecycle #1972

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Refine flycheck-display-errors lifecycle #1972

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d311234
Introduce flycheck-clear-displayed-errors-function to give flycheck-d…
wyuenho Aug 15, 2022
4d2a09f
clear displayed errors at teardown as well
wyuenho Aug 17, 2022
34b789b
delete dead code
wyuenho Aug 18, 2022
96ec3c2
move flycheck-clear-displayed-error-messages to flycheck-clear
wyuenho Aug 18, 2022
6604038
run flycheck-display-error-at-point even when there's no overlay at p…
wyuenho Aug 18, 2022
265e18b
no need to check buffer-modified-tick
wyuenho Aug 18, 2022
a3ea819
don't wash away message not displayed by flycheck
wyuenho Aug 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
72 changes: 44 additions & 28 deletions flycheck.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,18 @@ If set to nil, do not display errors at all."
:package-version '(flycheck . "0.13")
:risky t)

(defcustom flycheck-clear-displayed-errors-function #'flycheck-clear-displayed-error-messages
"Function to hide error message displayed by `flycheck-display-errors-function'.

If set to a function, it will be called with no arguments to
clear all displayed errors at point."
:group 'flycheck
:type '(choice (const :tag "Clear displayed error messages"
flycheck-clear-displayed-error-messages)
(function :tag "Clear displayed errors function"))
:package-version '(flycheck . "33")
Copy link
Contributor

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.

:risky t)

(defcustom flycheck-help-echo-function #'flycheck-help-echo-all-error-messages
"Function to compute the contents of the error tooltips.

Expand Down Expand Up @@ -2926,7 +2938,7 @@ Slots:
;; the focus hooks only work on Emacs 24.4 and upwards, but since undefined
;; hooks are perfectly ok we don't need a version guard here. They'll just
;; not work silently.
(post-command-hook . flycheck-maybe-display-error-at-point-soon)
(post-command-hook . flycheck-display-error-at-point-soon)
(focus-in-hook . flycheck-display-error-at-point-soon)
(focus-out-hook . flycheck-cancel-error-display-error-at-point-timer)
(post-command-hook . flycheck-hide-error-buffer)
Expand Down Expand Up @@ -3330,6 +3342,7 @@ current syntax check."
(flycheck-stop))
(flycheck-delete-all-overlays)
(flycheck-clear-errors)
(flycheck-clear-displayed-error-messages)
Copy link
Contributor

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.

Copy link
Contributor Author

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.

(flycheck-error-list-refresh)
(flycheck-hide-error-buffer))

Expand Down Expand Up @@ -5494,6 +5507,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
Copy link
Contributor

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?

Copy link
Contributor Author

@wyuenho wyuenho Feb 27, 2023 •
edited

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)

Copy link
Contributor Author

@wyuenho wyuenho Feb 27, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(funcall flycheck-clear-displayed-errors-function)))

(defvar-local flycheck-display-error-at-point-timer nil
"Timer to automatically show errors.")

Expand All @@ -5503,40 +5521,26 @@ non-nil."
(cancel-timer flycheck-display-error-at-point-timer)
(setq flycheck-display-error-at-point-timer nil)))

(defun flycheck--error-display-tick ()
"Return point and tick counter of current buffer."
(cons (point) (buffer-modified-tick)))

(defvar-local flycheck--last-error-display-tick nil
"Value of `flycheck--error-display-tick' when errors were last displayed.")

(defun flycheck-display-error-at-point ()
"Display all the error messages at point."
"Display all the error messages at point.

If there are no errors, clears the error messages at point."
(interactive)
;; This function runs from a timer, so we must take care to not ignore any
;; errors
(with-demoted-errors "Flycheck error display error: %s"
(flycheck-cancel-error-display-error-at-point-timer)
(setq flycheck--last-error-display-tick (flycheck--error-display-tick))
(when flycheck-mode
(-when-let (errors (flycheck-overlay-errors-at (point)))
(flycheck-display-errors errors)))))
(-if-let (errors (flycheck-overlay-errors-at (point)))
(flycheck-display-errors errors)
(flycheck-clear-displayed-errors)))))

(defun flycheck-display-error-at-point-soon ()
"Display error messages at point, with a delay."
(setq flycheck--last-error-display-tick nil)
(flycheck-maybe-display-error-at-point-soon))

(defun flycheck-maybe-display-error-at-point-soon ()
"Display error message at point with a delay, unless already displayed."
(flycheck-cancel-error-display-error-at-point-timer)
(when (and (not (equal flycheck--last-error-display-tick
(setq flycheck--last-error-display-tick
(flycheck--error-display-tick))))
(flycheck-overlays-at (point)))
(setq flycheck-display-error-at-point-timer
(run-at-time flycheck-display-errors-delay nil
'flycheck-display-error-at-point))))
(setq flycheck-display-error-at-point-timer
(run-at-time flycheck-display-errors-delay nil
'flycheck-display-error-at-point)))


;;; Functions to display errors
Expand All @@ -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)
Copy link
Contributor

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.


(defun flycheck-display-error-messages (errors)
"Display the messages of ERRORS.

Expand All @@ -5574,15 +5580,17 @@ information.
In the latter case, show messages in the buffer denoted by
variable `flycheck-error-message-buffer'."
(when (and errors (flycheck-may-use-echo-area-p))
(let ((message (flycheck-help-echo-all-error-messages errors)))
(display-message-or-buffer
message flycheck-error-message-buffer 'not-this-window)
(let* ((message (flycheck-help-echo-all-error-messages errors))
(retval (display-message-or-buffer
message flycheck-error-message-buffer 'not-this-window)))
;; We cannot rely on `display-message-or-buffer' returning the right
;; window. See URL `https://github.com/flycheck/flycheck/issues/1643'.
(-when-let (buf (get-buffer flycheck-error-message-buffer))
(with-current-buffer buf
(unless (derived-mode-p 'flycheck-error-message-mode)
(flycheck-error-message-mode)))))))
(flycheck-error-message-mode))))
(setq flycheck--last-displayed-message retval)
retval)))

(defun flycheck-display-error-messages-unless-error-list (errors)
"Show messages of ERRORS unless the error list is visible.
Expand All @@ -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)
Copy link
Contributor

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?

(if (and (stringp flycheck--last-displayed-message)
(equal (current-message) flycheck--last-displayed-message))
Copy link
Contributor

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?

(message nil)
(flycheck-hide-error-buffer))))


;;; Working with errors
(defun flycheck-copy-errors-as-kill (pos &optional formatter)
Expand Down