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

add support for relative path for gf #1917

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

Conversation

liuzhishan
Copy link

In evil mode, gf doest work when path under cursor has common ancstor with current buffer file. This seems to be a common situation. For example, suppose I have the following files:

project
└── src
    ├── a
    │   └── a.h
    └── b
        └── b.h

and in b.h, the file a/a.h is included:

// b/b.h
#include "a/a.h"

The gf command was mapped to find-file-at-point. If I want to open a/a.h file using gf in b.h file, it doest not work.

To solve this problem, I add a new function evil-open-file-under-cursor:

  1. Find path under cursor.
  2. If the path is absolute path, then try opening the file using find-file.
  3. If the path is a relative path, find directory of current buffer file, then try every combination of sub directory and path under cursor. If any of them exists, then we found the target, try opening the file.
  4. Otherwise, fallback to evil-find-file-at-point-with-line as the origin logic.

And also I changed the gf key mapping in evil-maps.el:

(define-key evil-normal-state-map "gf" 'evil-open-file-under-cursor)
(define-key evil-normal-state-map "]f" 'evil-open-file-under-cursor)
(define-key evil-normal-state-map "[f" 'evil-open-file-under-cursor)

  • [ x ] I searched the issue tracker and this hasn't been PRed before.
  • [ x ] My changes are not on the do-not-PR list for this project.
  • [ x ] My commits conform to the git conventions.
  • [ x ] Any relevant issues or PRs have been linked to.

@tomdl89
Copy link
Member

tomdl89 commented Jun 20, 2024

Thanks for the PR @liuzhishan. I think there will be some work required before we can merge. A few observations:

  • Functions such as evil-find-file-at-point-with-line should be backtick-quoted in docstrings
  • It looks like you're using the dash library. Evil doesn't require dash, so you'll need to find a way without it
  • Your indentation looks wrong on a few lines. e.g. 3738 and 3747
  • if without an else clause looks better as an and or when
  • My inclination would be to let this function take an argument of the filename, and put the thing-at-point code in the interactive block.
  • When let-binding something to nil, it's usually more idiomatic to just write foo than (foo nil)
  • let* is not necessary when you only have one binding let will do

I'll take a look at the code in more detail once these things are ironed out. I hope that helps.

@liuzhishan
Copy link
Author

@tomdl89 thanks for the detailed reply. I'll fix the code and update the pr.

@liuzhishan
Copy link
Author

liuzhishan commented Jun 22, 2024

Thanks for the PR @liuzhishan. I think there will be some work required before we can merge. A few observations:

  • Functions such as evil-find-file-at-point-with-line should be backtick-quoted in docstrings
  • It looks like you're using the dash library. Evil doesn't require dash, so you'll need to find a way without it
  • Your indentation looks wrong on a few lines. e.g. 3738 and 3747
  • if without an else clause looks better as an and or when
  • My inclination would be to let this function take an argument of the filename, and put the thing-at-point code in the interactive block.
  • When let-binding something to nil, it's usually more idiomatic to just write foo than (foo nil)
  • let* is not necessary when you only have one binding let will do

I'll take a look at the code in more detail once these things are ironed out. I hope that helps.

Hi, I have update the code, please have a look if there are any other problem.

Copy link
Member

@tomdl89 tomdl89 left a comment

Choose a reason for hiding this comment

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

I've added a load more comments. If this is an enjoyable educational process, then by all means keep going. Equally though, if this is turning out to be more work than you had hoped for, feel free to drop this PR, make an issue, and I'll take a look when I get the time. Cheers

@@ -3727,28 +3727,25 @@ Supports positions in the following formats: \"path:line path(line)\",
(when column-number
(move-to-column (1- column-number)))))))

(evil-define-command evil-open-file-under-cursor ()
(evil-define-command evil-open-file-under-cursor (file-path)
Copy link
Member

Choose a reason for hiding this comment

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

I think this name doesn't make sense now it takes an argument. I think something like evil-goto-file (in line with the mnemonic mentioned in vim's docs) would be better.

(evil-find-file-at-point-with-line))))))
directly. Otherwise, fallback to `evil-find-file-at-point-with-line`."
(interactive
(let ((file-path (thing-at-point 'filename t)))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to let bind things, only to immediately use them. You can just have (list (thing-at-point 'filename t))

(if (file-name-absolute-p file-path)
(find-file file-path)

(if (and (stringp file-path)
Copy link
Member

Choose a reason for hiding this comment

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

An if inside an if should almost always be a cond (or similar)

(find-file file-path)

(if (and (stringp file-path)
(string-match-p "/" file-path))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure on this, but have you checked if this check would work in windows? I believe they use backslashes over there.

(if (and (stringp file-path)
(string-match-p "/" file-path))
(let ((dirname (locate-dominating-file default-directory file-path)))
(if (file-in-directory-p dirname (doom-project-root))
Copy link
Member

Choose a reason for hiding this comment

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

doom-project-root is part of doom, not emacs or evil, so you'll need to find a way without it.

(string-match-p "/" file-path))
(let ((dirname (locate-dominating-file default-directory file-path)))
(if (file-in-directory-p dirname (doom-project-root))
(find-file (doom-path dirname file-path))
Copy link
Member

Choose a reason for hiding this comment

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

likewise with doom-path, it's not part of emacs or evil.

(if (file-in-directory-p dirname (doom-project-root))
(find-file (doom-path dirname file-path))
(evil-find-file-at-point-with-line)))
(evil-find-file-at-point-with-line))))
Copy link
Member

Choose a reason for hiding this comment

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

I get that this works, but not sure I like how it will echo "no line, no column" when it visits via this way.

Copy link
Member

Choose a reason for hiding this comment

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

Have you taken a look at evil-find-file-at-point-visual? I think it could be done away with by adding visual-state handling logic into this function.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I haven't tried your function yet (because it requires functions I don't have) but your diagram of accessing a/a.h from b/b.h doesn't seem to tally with the vim's documentation on path (by default, anyway). So I'm not sure if your diagram is wrong and your functionality is right, or the other way round, but that's worth checking too.

@liuzhishan
Copy link
Author

I've added a load more comments. If this is an enjoyable educational process, then by all means keep going. Equally though, if this is turning out to be more work than you had hoped for, feel free to drop this PR, make an issue, and I'll take a look when I get the time. Cheers

Hi, I think this is a enjoyable learning process. I use doom emacs, and got used to use some doom function. I'll rm the doom related code, and fix other problems.

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.

None yet

2 participants