bug#70996: project-find-file defaults

bug#70996: project-find-file defaults

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

(thing-at-point 'filename) in project-find-file is a very useful feature
that often helps to use a string under point as a partial file name
to match a project file name as substring.  So it's like 'M-.'
that navigates by file names instead of identifiers.

But the problem is that in this case it drops the current file name
as the default value that is also useful in many cases.

Fortunately, the minibuffer supports a list of default values,
like in the following patch:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: project-find-file-at-point.patch --]
[-- Type: text/x-diff, Size: 2389 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index a95d1267dd2..5e6516b3b64 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1077,8 +1130,9 @@ project-find-file
          (dirs (list root))
          (project-files-relative-names t))
     (project-find-file-in
-     (or (thing-at-point 'filename)
-         (and buffer-file-name (project--find-default-from buffer-file-name pr)))
+     (delq nil (list (and buffer-file-name (project--find-default-from
+                                            buffer-file-name pr))
+                     (thing-at-point 'filename)))
      dirs pr include-all)))
 
 ;;;###autoload
@@ -1100,8 +1154,9 @@ project-or-external-find-file
                 (project-external-roots pr)))
          (project-file-history-behavior t))
     (project-find-file-in
-     (or (thing-at-point 'filename)
-         (and buffer-file-name (project--find-default-from buffer-file-name pr)))
+     (delq nil (list (and buffer-file-name (project--find-default-from
+                                            buffer-file-name pr))
+                     (thing-at-point 'filename)))
      dirs pr include-all)))
 
 (defcustom project-read-file-name-function #'project--read-file-cpd-relative
@@ -1163,11 +1218,14 @@ project--read-file-cpd-relative
                          (setq all-files
                                (delete common-parent-directory all-files))
                          t))
-         (mb-default (if (and common-parent-directory
-                              mb-default
-                              (file-name-absolute-p mb-default))
-                         (file-relative-name mb-default common-parent-directory)
-                       mb-default))
+         (mb-default (mapcar (lambda (mb-default)
+                               (if (and common-parent-directory
+                                        mb-default
+                                        (file-name-absolute-p mb-default))
+                                   (file-relative-name
+                                    mb-default common-parent-directory)
+                                 mb-default))
+                             (ensure-list mb-default)))
          (substrings (mapcar (lambda (s) (substring s cpd-length)) all-files))
          (_ (when included-cpd
               (setq substrings (cons "./" substrings))))

bug#70996: project-find-file defaults

[Juri Linkov]

close 70996 30.0.50
thanks

> (thing-at-point 'filename) in project-find-file is a very useful feature
> that often helps to use a string under point as a partial file name
> to match a project file name as substring.  So it's like 'M-.'
> that navigates by file names instead of identifiers.
>
> But the problem is that in this case it drops the current file name
> as the default value that is also useful in many cases.
>
> Fortunately, the minibuffer supports a list of default values,
> like in the following patch:

With no objections this is pushed to master.

bug#70996: project-find-file defaults

[Dmitry Gutov]

Hi Juri,

On 17/05/2024 09:36, Juri Linkov wrote:
> But the problem is that in this case it drops the current file name
> as the default value that is also useful in many cases.
> 
> Fortunately, the minibuffer supports a list of default values,
> like in the following patch:

This seems like a good idea, except it reverses the priority: 
previously, if file-at-point was present, it would be the default, not 
the current file name.

So how about this?

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 8a8b4fc33d6..b855dcd4546 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1080,9 +1080,9 @@ project-find-file
           (dirs (list root))
           (project-files-relative-names t))
      (project-find-file-in
-     (delq nil (list (and buffer-file-name (project--find-default-from
-                                            buffer-file-name pr))
-                     (thing-at-point 'filename)))
+     (delq nil (list (thing-at-point 'filename)
+                     (and buffer-file-name (project--find-default-from
+                                            buffer-file-name pr))))
       dirs pr include-all)))

  ;;;###autoload
@@ -1104,9 +1104,9 @@ project-or-external-find-file
                  (project-external-roots pr)))
           (project-file-history-behavior t))
      (project-find-file-in
-     (delq nil (list (and buffer-file-name (project--find-default-from
-                                            buffer-file-name pr))
-                     (thing-at-point 'filename)))
+     (delq nil (list (thing-at-point 'filename)
+                     (and buffer-file-name (project--find-default-from
+                                            buffer-file-name pr))))
       dirs pr include-all)))

  (defcustom project-read-file-name-function 
#'project--read-file-cpd-relative

bug#70996: project-find-file defaults

[Juri Linkov]

>> But the problem is that in this case it drops the current file name
>> as the default value that is also useful in many cases.
>> Fortunately, the minibuffer supports a list of default values,
>> like in the following patch:
>
> This seems like a good idea, except it reverses the priority: previously,
> if file-at-point was present, it would be the default, not the current file
> name.

I mentioned in the log message that this change was intentional.

> So how about this?

The reason of this change to make the first item of the M-n list
more deterministic:

1. when there is no thing-at-point, then the first item will be
   buffer-file-name;

2. and also when there is a thing-at-point, the first item
   will remain buffer-file-name.

Otherwise, it was too unpredictable: after typing 'M-n RET'
to use buffer-file-name, it often did a wrong thing
when point happened to stay in a thing-at-point.

bug#70996: project-find-file defaults

[Dmitry Gutov]

On 09/06/2024 19:51, Juri Linkov wrote:
>>> But the problem is that in this case it drops the current file name
>>> as the default value that is also useful in many cases.
>>> Fortunately, the minibuffer supports a list of default values,
>>> like in the following patch:
>>
>> This seems like a good idea, except it reverses the priority: previously,
>> if file-at-point was present, it would be the default, not the current file
>> name.
> 
> I mentioned in the log message that this change was intentional.
> 
>> So how about this?
> 
> The reason of this change to make the first item of the M-n list
> more deterministic:

Okay, now I see that line, thanks.

> 1. when there is no thing-at-point, then the first item will be
>     buffer-file-name;
> 
> 2. and also when there is a thing-at-point, the first item
>     will remain buffer-file-name.
> 
> Otherwise, it was too unpredictable: after typing 'M-n RET'
> to use buffer-file-name, it often did a wrong thing
> when point happened to stay in a thing-at-point.

Okay, but I'm not sure predictability must be the overriding principle.

If 10 people use the thing-at-point default, for example, and only 2 use 
the buffer-file-name default (or, say, the number of users is the same, 
but the frequency is higher for the latter), we'd be forcing a lot of 
people to press C-n to jump over the default they don't use.

What's the main usage scenario for the buffer-file-name default? I 
recall Spencer describing his workflow, but that seems only useful when 
you have a lot of branches, checked out specifically into worktrees or 
similar, and switch between them often (while explicitly staying in the 
"same" file during a switch). Do you do something similar?

bug#70996: project-find-file defaults

[Juri Linkov]

>> The reason of this change to make the first item of the M-n list
>> more deterministic:
>
> Okay, now I see that line, thanks.
>
>> 1. when there is no thing-at-point, then the first item will be
>>     buffer-file-name;
>> 2. and also when there is a thing-at-point, the first item
>>     will remain buffer-file-name.
>> Otherwise, it was too unpredictable: after typing 'M-n RET'
>> to use buffer-file-name, it often did a wrong thing
>> when point happened to stay in a thing-at-point.
>
> Okay, but I'm not sure predictability must be the overriding principle.
>
> If 10 people use the thing-at-point default, for example, and only 2 use
> the buffer-file-name default (or, say, the number of users is the same, but
> the frequency is higher for the latter), we'd be forcing a lot of people to
> press C-n to jump over the default they don't use.
>
> What's the main usage scenario for the buffer-file-name default? I recall
> Spencer describing his workflow, but that seems only useful when you have
> a lot of branches, checked out specifically into worktrees or similar, and
> switch between them often (while explicitly staying in the "same" file
> during a switch). Do you do something similar?

I recall Spencer mentioned that 'C-x p f M-n' is the quickest way getting
a file name relative to the project root for using it for external references.
And I use the same case very often too.  (I mean the case of
'(project--find-default-from buffer-file-name pr)').

OTOH, thing-at-point is too specific to a programming language,
and can be used to navigate source code by placing point on
an "include" directive with relative a file name, then
'C-x p f M-n M-n' will help to find the referenced file.
This is a poor man's way for source code navigation, since 'M-.'
doesn't support navigation by project relative file names.

bug#70996: project-find-file defaults

[Dmitry Gutov]

On 11/06/2024 19:58, Juri Linkov wrote:
>> What's the main usage scenario for the buffer-file-name default? I recall
>> Spencer describing his workflow, but that seems only useful when you have
>> a lot of branches, checked out specifically into worktrees or similar, and
>> switch between them often (while explicitly staying in the "same" file
>> during a switch). Do you do something similar?
> I recall Spencer mentioned that 'C-x p f M-n' is the quickest way getting
> a file name relative to the project root for using it for external references.
> And I use the same case very often too.  (I mean the case of
> '(project--find-default-from buffer-file-name pr)').

Hmm, that sounds good and useful, but it's also, like, a secondary 
purpose - using a command's history while aborting the command itself.

To copy the relative file name, you also need to select it, right? 
Doesn't that mean that you will look at the minibuffer first?

Or do you do like C-x p f M-n C-h C-w automatically?

> OTOH, thing-at-point is too specific to a programming language,
> and can be used to navigate source code by placing point on
> an "include" directive with relative a file name, then
> 'C-x p f M-n M-n' will help to find the referenced file.
> This is a poor man's way for source code navigation, since 'M-.'
> doesn't support navigation by project relative file names.

Sometimes it does (certain backends), but indeed this is a way to visit 
an included file, for example. And here the core behavior (find-file) 
does get executed.

bug#70996: project-find-file defaults

[Spencer Baugh]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 09/06/2024 19:51, Juri Linkov wrote:
>>>> But the problem is that in this case it drops the current file name
>>>> as the default value that is also useful in many cases.
>>>> Fortunately, the minibuffer supports a list of default values,
>>>> like in the following patch:
>>>
>>> This seems like a good idea, except it reverses the priority: previously,
>>> if file-at-point was present, it would be the default, not the current file
>>> name.
>> I mentioned in the log message that this change was intentional.
>> 
>>> So how about this?
>> The reason of this change to make the first item of the M-n list
>> more deterministic:
>
> Okay, now I see that line, thanks.
>
>> 1. when there is no thing-at-point, then the first item will be
>>     buffer-file-name;
>> 2. and also when there is a thing-at-point, the first item
>>     will remain buffer-file-name.
>> Otherwise, it was too unpredictable: after typing 'M-n RET'
>> to use buffer-file-name, it often did a wrong thing
>> when point happened to stay in a thing-at-point.
>
> Okay, but I'm not sure predictability must be the overriding principle.
>
> If 10 people use the thing-at-point default, for example, and only 2
> use the buffer-file-name default (or, say, the number of users is the
> same, but the frequency is higher for the latter), we'd be forcing a
> lot of people to press C-n to jump over the default they don't use.

It seems to me that we can have the best of both worlds if we match the
behavior of find-file, and use something like
(run-hook-with-args-until-success 'file-name-at-point-functions) rather
than (thing-at-point 'file-name).

The default of file-name-at-point-functions is
ffap-guess-file-name-at-point, which by default only returns a filename
if that file name actually exists in the filesystem.

The old behavior of (thing-at-point 'file-name) often got in the way,
since it would pick up any random string at point, even if it wasn't
referring to an actual file name.

Instead we can be like find-file and have:

(delq nil (list
  (run-hook-with-args-until-success 'file-name-at-point-functions)
  buffer-file-name))

So the file name at point *does* take precedence over
buffer-file-name... but the file name at point is only present when it's
actually useful - that is, when the file exists.

This is especially useful now that there is ffap-in-project by default,
so ffap-guess-file-name-at-point will pick up relative file names in the
project root.

I personally never use the file-name-at-point behavior of
project-find-file, but I'm happy with it being higher-precedence because
it will match find-file - as long as it also matches find-file in only
including filenames of existing files.

> What's the main usage scenario for the buffer-file-name default? I
> recall Spencer describing his workflow, but that seems only useful
> when you have a lot of branches, checked out specifically into
> worktrees or similar, and switch between them often (while explicitly
> staying in the "same" file during a switch). Do you do something
> similar?

For me, two use cases:

1. Copy project-relative filename:
   C-x p f M-n C-a C-k

2. Switch to the same file in another project:
   C-x p p [type project name] f M-n RET

About half of my use for 2 is switching between my emacs-29 and trunk
git worktrees.  (The other half is switching between checkouts of
branches in Jane Street's internal monorepo)

bug#70996: project-find-file defaults

[Dmitry Gutov]

On 12/06/2024 03:11, Spencer Baugh wrote:

> It seems to me that we can have the best of both worlds if we match the
> behavior of find-file, and use something like
> (run-hook-with-args-until-success 'file-name-at-point-functions) rather
> than (thing-at-point 'file-name).
> 
> The default of file-name-at-point-functions is
> ffap-guess-file-name-at-point, which by default only returns a filename
> if that file name actually exists in the filesystem.
> 
> The old behavior of (thing-at-point 'file-name) often got in the way,
> since it would pick up any random string at point, even if it wasn't
> referring to an actual file name.
> 
> Instead we can be like find-file and have:
> 
> (delq nil (list
>    (run-hook-with-args-until-success 'file-name-at-point-functions)
>    buffer-file-name))

That's an interesting suggestion, but could we rely on 
file-name-at-point-functions acting correctly for any project?

As luck would have it, ffap-guess-file-name-at-point seems to work fine 
on relative file names inside a directory, and even file names without 
extensions, but it could miss them in some odd directory structures.

> So the file name at point *does* take precedence over
> buffer-file-name... but the file name at point is only present when it's
> actually useful - that is, when the file exists.

Sounds good to me.

> This is especially useful now that there is ffap-in-project by default,
> so ffap-guess-file-name-at-point will pick up relative file names in the
> project root.

This one won't pick up base file names inside some directory. Or just 
names relative to a subdirectory.

> I personally never use the file-name-at-point behavior of
> project-find-file, but I'm happy with it being higher-precedence because
> it will match find-file - as long as it also matches find-file in only
> including filenames of existing files.

In case you agree with my concerns above (I'm happy to be convinced 
otherwise), we could try to do something like

   (completion-try-completion (thing-at-point 'filename)
                              TABLE nil LEN)

Unfortunately, we currently call thing-at-point earlier than 
project-files is called. So some rethinking would have to be done (a 
breaking change to project-find-file-in, apparently).

>> What's the main usage scenario for the buffer-file-name default? I
>> recall Spencer describing his workflow, but that seems only useful
>> when you have a lot of branches, checked out specifically into
>> worktrees or similar, and switch between them often (while explicitly
>> staying in the "same" file during a switch). Do you do something
>> similar?
> 
> For me, two use cases:
> 
> 1. Copy project-relative filename:
>     C-x p f M-n C-a C-k
> 
> 2. Switch to the same file in another project:
>     C-x p p [type project name] f M-n RET
> 
> About half of my use for 2 is switching between my emacs-29 and trunk
> git worktrees.  (The other half is switching between checkouts of
> branches in Jane Street's internal monorepo)

I guess my question was whether you do it frequently enough that the 
change in the order would made a big difference. But the answer you gave 
above is even better.

bug#70996: project-find-file defaults

[Dmitry Gutov]

On 12/06/2024 16:52, Dmitry Gutov wrote:
> 
> In case you agree with my concerns above (I'm happy to be convinced 
> otherwise), we could try to do something like
> 
>    (completion-try-completion (thing-at-point 'filename)
>                               TABLE nil LEN)
> 
> Unfortunately, we currently call thing-at-point earlier than 
> project-files is called. So some rethinking would have to be done (a 
> breaking change to project-find-file-in, apparently).

Actually, we could do the check inside project-find-file-in.

The tricky part is that project--file-completion-table (which adds the 
metadata property to the table, allowing for laxer matching due to the 
entry in completion-category-defaults) is called later - in 
project--read-file-cpd-relative and project--read-file-absolute.

And one reason for that is project--read-file-cpd-relative mapcar's the 
list through 'substring'. Which requires a list as input, and IIRC so 
the previous version first called 'all-completions'. Which wasn't free, 
especially with larger lists - but perhaps since the completion table is 
functional we could get away with returning a copy of the list when the 
prefix is empty...

Or maybe project--read-file-cpd-relative could use something like 
completion-table-subvert, which seems like it can avoid extra copying...

Alternatively, we'd change the calling convention even more and pass 
down a function that returns the list of "future history". And takes the 
completion table as argument, I suppose.

bug#70996: project-find-file defaults

[Juri Linkov]

>> It seems to me that we can have the best of both worlds if we match the
>> behavior of find-file, and use something like
>> (run-hook-with-args-until-success 'file-name-at-point-functions) rather
>> than (thing-at-point 'file-name).
>> The default of file-name-at-point-functions is
>> ffap-guess-file-name-at-point, which by default only returns a filename
>> if that file name actually exists in the filesystem.
>> The old behavior of (thing-at-point 'file-name) often got in the way,
>> since it would pick up any random string at point, even if it wasn't
>> referring to an actual file name.
>> Instead we can be like find-file and have:
>> (delq nil (list
>>    (run-hook-with-args-until-success 'file-name-at-point-functions)
>>    buffer-file-name))
>
> That's an interesting suggestion, but could we rely on
> file-name-at-point-functions acting correctly for any project?
>
> As luck would have it, ffap-guess-file-name-at-point seems to work fine on
> relative file names inside a directory, and even file names without
> extensions, but it could miss them in some odd directory structures.

I tried file-name-at-point-functions with ffap-guess-file-name-at-point,
but it fails to pick a file name when it's not relative neither
to default-directory nor to the project root.  It's easy to blame
the design of such languages, but this won't solve the problem.

So I see no way to reliable pick a file name.  Therefore the need
to fall back to (thing-at-point 'filename).

>> I personally never use the file-name-at-point behavior of
>> project-find-file, but I'm happy with it being higher-precedence because
>> it will match find-file - as long as it also matches find-file in only
>> including filenames of existing files.
>
> In case you agree with my concerns above (I'm happy to be convinced
> otherwise), we could try to do something like
>
>   (completion-try-completion (thing-at-point 'filename)
>                              TABLE nil LEN)

Unfortunately, this might get a false positive on a short word under point
that with high likelihood will match a file name with a short substring.

bug#70996: project-find-file defaults

[Dmitry Gutov]

On 14/06/2024 20:00, Juri Linkov wrote:
>>> I personally never use the file-name-at-point behavior of
>>> project-find-file, but I'm happy with it being higher-precedence because
>>> it will match find-file - as long as it also matches find-file in only
>>> including filenames of existing files.
>> In case you agree with my concerns above (I'm happy to be convinced
>> otherwise), we could try to do something like
>>
>>    (completion-try-completion (thing-at-point 'filename)
>>                               TABLE nil LEN)
> Unfortunately, this might get a false positive on a short word under point
> that with high likelihood will match a file name with a short substring.

It might. But we could set up the styles (or some other way to do the 
matching) so that it would only match full file name segments.