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

Fix legit select path find #984

Merged
merged 4 commits into from
Sep 5, 2023
Merged

Conversation

Sasanidas
Copy link
Member

No description provided.

Comment on lines 327 to 329
(format nil "~a~a"
(lem-core/commands/project:find-root (buffer-filename))
file)))))
Copy link
Member

Choose a reason for hiding this comment

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

Using merge-pathnames seems to be a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added 👍

@@ -320,7 +320,13 @@ Notes:
(define-command peek-legit-select () ()
(alexandria:when-let ((file (get-matched-file)))
(quit)
(alexandria:when-let ((buffer (find-file-buffer file)))
(alexandria:when-let
((buffer (or (and (uiop:file-exists-p file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

(I don't really know the context for this fix)

In legit, pressing Enter in the legit window calls this function to visit the file at point. There was a file listing, the file should exist. So if the file doesn't exist anymore, we maybe should display a message to the user (instead of doing nothing)? (and refresh the legit window?)

Also I thought the %filename slot of a buffer had the full path, not requiring to get the project root?

exple:

[ ]  %FILENAME       = "/home/vince/lisp-projects/lem/legit.lisp"

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, but seems like the path is indeed not correct:

2023-08-22.13-14-35.mp4

Maybe the buffer doesn't have the information about the absolute path? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't reproduce. Do you try to visit an unstaged file?

  • start from directory mode
  • move cursor on an "unstaged changes" line
  • press Enter

? this works for me, I visit the file at point. I can stage it with "s" too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I did just that 🤔

@cxxxr
Copy link
Member

cxxxr commented Aug 30, 2023

What is the state of this?

@vindarel
Copy link
Collaborator

vindarel commented Sep 1, 2023

What is the state of this?

I myself can't reproduce, I have yet to try more,

and I have an open improvement suggestion in my comment:

if the file doesn't exist anymore, we maybe should display a message to the user (instead of doing nothing)?

@Sasanidas
Copy link
Member Author

I added a message when the file cannot be found 👍

((buffer (or (and (uiop:file-exists-p file)
(find-file-buffer file))
(find-file-buffer
(merge-pathnames
(lem-core/commands/project:find-root (buffer-filename))
file)))))
(switch-to-buffer buffer))))
(switch-to-buffer buffer)
(message "File ~a doesn't exist." file))))
Copy link
Member

Choose a reason for hiding this comment

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

Elsewhere, we use editor-error to display errors.
Is it possible to do that here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 👍

@cxxxr
Copy link
Member

cxxxr commented Sep 5, 2023

Thank you!

@cxxxr cxxxr merged commit 3059b04 into lem-project:main Sep 5, 2023
1 check passed
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

Successfully merging this pull request may close these issues.

3 participants