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.

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

Tide-lv-message shows too few fixes. #466

Open
6 of 7 tasks
phuhl opened this issue May 25, 2023 · 5 comments
Open
6 of 7 tasks

Tide-lv-message shows too few fixes. #466

phuhl opened this issue May 25, 2023 · 5 comments

Comments

@phuhl
Copy link
Contributor

phuhl commented May 25, 2023

When submitting issues to this project, please include the following information.

Checklist

  • I have searched both open and closed issues and cannot find a duplicate.
  • I can reproduce the problem with the latest version of the relevant packages.
  • The problem still occurs after I issued M-x tide-restart-server in the buffer where I had the problem.
  • I verified that the version and the configuration file path reported by M-x tide-verify-setup are correct.
  • If tide is reporting an error or warning I think should not be reported, I can run tsc (and tslint, if applicable) without the error or warning I'm seeing in tide.
  • If tide is not reporting an error or warning I think should be reported, tsc (or tslint, if applicable) reports the error or warning I was expecting to see.
    • ish. It reports the error, just not the fix.
  • I am positive the problem does not belong to typescript-mode or tsserver.

Relevant Version Numbers

  • Tide: 20230428.1846
  • TypeScript: 5.0.2
  • Emacs: GNU Emacs 29.0.90 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.37, cairo version 1.17.8) of 2023-04-26

(The list above is minimal. Make sure to include any other version numbers relevant to your report.)

Steps to Reproduce the Bug

I have this code:

export const MainAppContainer = () => {
  useEffect(() => {}, []);
  return null
};

useEffect is not imported. Tide correctly identifies the issue and produces the correct warning/error. I run tide-fix. Tide only displays this:

image

I checked TS-Server and debugged into tide. TS-server reports two fixes. This patch within tide.el shows that this is clearly a display issue, probably caused by tide-lv-message:

(defun tide-popup-select-item (prompt list)
  (let ((hints (-map-indexed
                (lambda (i item)
                  (concat (propertize (char-to-string (nth i tide-alphabets)) 'face 'tide-choice-face)
                          "  "
                          item))
                list)))
+   (message (mapconcat 'identity hints "\n"))
    (unwind-protect
        (progn
          (tide-lv-message (mapconcat 'identity hints "\n"))
          (let ((selected (read-char-choice prompt (-take (length list) tide-alphabets))))
            (nth (-find-index (lambda (char) (eql selected char)) tide-alphabets) list)))
        (tide-lv-delete-window))))

The modified version prints out this:

a  Add missing function declaration ’useEffect’
s  Update import from "react"

Also, using the "s"-key applies the fix.

Expected Behavior

All fixes should be shown by tide-lv

Actual Behavior

Only n-1 (?) fixes are shown.

@phuhl
Copy link
Contributor Author

phuhl commented May 25, 2023

My current quick fix to make tide function for me currently is this:

(defun tide-popup-select-item (prompt list)
  (let ((hints (-map-indexed
                (lambda (i item)
                  (concat (propertize (char-to-string (nth i tide-alphabets)) 'face 'tide-choice-face)
                          "  "
                          item))
                list)))
    (unwind-protect
        (progn
+         (tide-lv-message (concat (mapconcat 'identity hints "\n") "\ntest"))
-         (tide-lv-message (mapconcat 'identity hints "\n") )
          (let ((selected (read-char-choice prompt (-take (length list) tide-alphabets))))
            (nth (-find-index (lambda (char) (eql selected char)) tide-alphabets) list)))
        (tide-lv-delete-window))))

For some reason, just adding "\n" at the end does not suffice.

@ananthakumaran
Copy link
Owner

ananthakumaran commented May 25, 2023

I did a quick check to see if I can reproduce the issue, unfortunately, I can't. Can you confirm if this is not related to emacs version or other plugins? From your comments, it seems like up to tide-lv-message, everything is good. tide-lv file has not been touched after the initial import from the hydra project, I would need more help from you to debug.

Try to debug around (set (make-local-variable 'window-min-height) n-lines). Check if n-lines is different between the cases.

@phuhl
Copy link
Contributor Author

phuhl commented May 25, 2023

Can you confirm if this is not related to emacs version or other plugins?

I can't guarantee it, I guess. But I wouldn't know what could be responsible. I have a feeling it could be a braking change in Emacs 29, though.

I printed n-lines and it gives me 1 (in the unmodified version of tide). Might this be the issue? We have two lines for the suggestions and one for the "Select fix:" for a total of two \ns.

@phuhl
Copy link
Contributor Author

phuhl commented May 25, 2023

Ok, I played around a bit more with n-lines. Turns out the value of n-lines does not seem to do anything on my system. Even setting it to 10 did not fix the issue. Only when the string has an additional line (and not only a \n at the end but actual content in the last line the window is bigger.

Changing the fit-window-to-buffer call to this though: (fit-window-to-buffer nil nil (+ 2 n-lines)) did work.

@phuhl
Copy link
Contributor Author

phuhl commented May 25, 2023

From Emacs 29 changelog:

+++
*** 'display-buffer' provides more options for using an existing window.
The display buffer action functions 'display-buffer-use-some-window' and
'display-buffer-use-least-recent-window' now honor the action alist
entry 'window-min-height' as well as the entries listed below to make
the display of several buffers in a row more amenable.

Not sure why this would have the given effect, but it seems that behavior around window-min-height has been changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants