make project--find-in-file generic, add interactive filename to project-find-file

make project--find-in-file generic, add interactive filename to project-find-file

[Stephen Leake]

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

Any objections to the attached patch?

Having a "filename" parameter for `project-find-file' is useful with
comments like:

 // see file "fancy_hack.c" for more details

Put point on "fancy_hack.c", M-x project-find-file (or your favorite key
binding), and the file comes up.


Making `project--find-file-in' generic allows my ede and ada-mode
backends to override the default implementation.


The change in xref just provides a missing doc string for a function
that is used by the default project--find-file-in.

-- 
-- Stephe

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

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 85f3907..becccbb 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -154,6 +154,15 @@ project--find-in-directory
     vc-directory-exclusion-list)
    grep-find-ignored-files))
 
+(cl-defgeneric project--find-file-in (filename dirs project)
+  "Complete FILENAME (a string or nil) in directories DIRS; visit the file.
+If non-nil, DIRS is a list of absolute directories; it should be some
+subset of the project roots and external roots. If nil, the backend
+uses all the directories it knows about.
+PROJECT is used to find the project ignores and other project meta-data."
+  ;; dispatch on PROJECT
+  )
+
 (defgroup project-vc nil
   "Project implementation using the VC package."
   :version "25.1"
@@ -312,31 +321,34 @@ project--find-regexp-in
     (xref--show-xrefs xrefs nil)))
 
 ;;;###autoload
-(defun project-find-file ()
-  "Visit a file in the current project's roots.
-
-This is like `find-file', but it limits the file-name completion
-candidates to the files within the current project roots."
-  (interactive)
+(defun project-find-file (filename)
+  "Visit FILENAME in the current project's roots.
+FILENAME defaults to the filename at point (nil if none
+recognized).
+FILENAME is completed with the files within the current project
+roots."
+  (interactive (list (when current-prefix-arg (thing-at-point 'filename))))
   (let* ((pr (project-current t))
          (dirs (project-roots pr)))
-    (project--find-file-in dirs pr)))
+    (project--find-file-in filename dirs pr)))
 
 ;;;###autoload
-(defun project-or-external-find-file ()
-  "Visit a file in the current project's roots or external roots.
-
-This is like `find-file', but it limits the file-name completion
-candidates to the files within the current project roots and external roots."
-  (interactive)
+(defun project-or-external-find-file (filename)
+  "Visit FILENAME in the current project's roots or external roots.
+FILENAME defaults to the filename at point (nil if none
+recognized).
+FILENAME is completed with the files within the current project
+roots and external roots."
+  (interactive (list (when current-prefix-arg (thing-at-point 'filename))))
   (let* ((pr (project-current t))
          (dirs (append
                 (project-roots pr)
                 (project-external-roots pr))))
-    (project--find-file-in dirs pr)))
+    (project--find-file-in filename dirs pr)))
 
-;; FIXME: Uniquely abbreviate the roots?
-(defun project--find-file-in (dirs project)
+  ;; FIXME: Uniquely abbreviate the roots?
+(cl-defmethod project--find-file-in (filename dirs project)
+  "Default implementation using `find-program'."
   (require 'xref)
   (let* ((all-files
           (cl-mapcan
@@ -357,7 +369,7 @@ project--find-file-in
                    (t
                     (complete-with-action action all-files string pred))))))
     (find-file
-     (completing-read "Find file: " table nil t))))
+     (completing-read "Find file: " table nil t filename))))
 
 (provide 'project)
 ;;; project.el ends here
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 267853d..ad7d6bb 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -948,6 +948,9 @@ xref--rgrep-command
    (xref--find-ignores-arguments ignores dir)))
 
 (defun xref--find-ignores-arguments (ignores dir)
+  "Convert IGNORES and DIR to a list of arguments for 'find'.
+IGNORES is a list of glob patterns.  DIR is an absolute
+directory, used as the root of the ignore globs."
   ;; `shell-quote-argument' quotes the tilde as well.
   (cl-assert (not (string-match-p "\\`~" dir)))
   (when ignores

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/28/2016 04:04 AM, Stephen Leake wrote:
> Any objections to the attached patch?

A couple:

- IIRC, using INITIAL-INPUT argument is semi-deprecated. Using DEFAULT 
is preferable, and then you don't need to make that value depend on 
current-prefix-arg. We can always set the default input, and if the user 
wants to use it, they will press M-n. And I see no good reason to thread 
it through the command's function arguments. Why not do it like in the 
patch at the bottom?

- More importantly, the new generic function should not do too much. It 
should return a completion table, which can dispatch to ede, ada-mode, 
or any other facility. I believe we've went over this a couple of times 
already.

The xref docstring looks good, thanks.

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 85f3907..03ead98 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -357,7 +357,8 @@ project--find-file-in
                     (t
                      (complete-with-action action all-files string 
pred))))))
      (find-file
-     (completing-read "Find file: " table nil t))))
+     (completing-read "Find file: " table nil t nil nil
+                      (thing-at-point 'filename)))))

  (provide 'project)
  ;;; project.el ends here

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stephen Leake]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 01/28/2016 04:04 AM, Stephen Leake wrote:
>> Any objections to the attached patch?
>
> A couple:
>
> - IIRC, using INITIAL-INPUT argument is semi-deprecated. Using DEFAULT
> is preferable, 

I was not aware of M-n (more below).

> and then you don't need to make that value depend on
> current-prefix-arg. 

Actually, I did not intend to make it depend on current-prefix-arg, as
the doc string does not mention that; I forgot to fix that when I copied
from my current code.

On the other hand, it makes sense to give the user some control over
whether the file at point is used.

In any case, we can call `thing-at-point' in the
completing-read call for either INITIAL-INPUT or DEFAULT.

> We can always set the default input, and if the user wants to use it,
> they will press M-n. 

I was not aware of that use of M-n; that is a good alternative to
current-prefix-arg.

However, the default still does not show anywhere;
we should add it to the prompt.

> And I see no good reason to thread it through the command's function
> arguments. Why not do it like in the patch at the bottom?

Because there are also times when calling `project-find-file' from lisp
with a filename makes sense. But that code can call
project--find-file-in, instead of project-find-file.

> - More importantly, the new generic function should not do too much.
> It should return a completion table, which can dispatch to ede,
> ada-mode, or any other facility. I believe we've went over this a
> couple of times already.

So instead of computing `table' in project--find-file-in, it would
call (project--file-completion-table dirs project). The default
implementation of `project--file-completion-table' would be the code
using `find-program'.

I'll give that a try.

-- 
-- Stephe

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/28/2016 01:11 PM, Stephen Leake wrote:

>> - IIRC, using INITIAL-INPUT argument is semi-deprecated. Using DEFAULT
>> is preferable,
>
> I was not aware of M-n (more below).

Hmm, the default value actually has one noticeable drawback here: if you 
just press RET, completing-read will accept it, and won't check whether 
the completion table includes the given value. Then, find-file will be 
called with this raw (not expanded) value.

Stefan, would you say INITIAL-INPUT is better for this use case?

>> and then you don't need to make that value depend on
>> current-prefix-arg.
>
> Actually, I did not intend to make it depend on current-prefix-arg, as
> the doc string does not mention that; I forgot to fix that when I copied
> from my current code.
>
> On the other hand, it makes sense to give the user some control over
> whether the file at point is used.

Indeed.

> However, the default still does not show anywhere;
> we should add it to the prompt.

Agree. If we end up using the default.

>> And I see no good reason to thread it through the command's function
>> arguments. Why not do it like in the patch at the bottom?
>
> Because there are also times when calling `project-find-file' from lisp
> with a filename makes sense.

In a user-defined command? Maybe.

> But that code can call
> project--find-file-in, instead of project-find-file.

It'll have to become a public function, though, if we're really 
considering this use case.

>> - More importantly, the new generic function should not do too much.
>> It should return a completion table, which can dispatch to ede,
>> ada-mode, or any other facility. I believe we've went over this a
>> couple of times already.
>
> So instead of computing `table' in project--find-file-in, it would
> call (project--file-completion-table dirs project). The default
> implementation of `project--file-completion-table' would be the code
> using `find-program'.

Pretty much, except it should have a public name (no double-dash), and 
the project should probably be the first argument.

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stephen Leake]

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

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> On 01/28/2016 04:04 AM, Stephen Leake wrote:
>>> Any objections to the attached patch?
>>
>
>> - More importantly, the new generic function should not do too much.
>> It should return a completion table, which can dispatch to ede,
>> ada-mode, or any other facility. I believe we've went over this a
>> couple of times already.
>
> So instead of computing `table' in project--find-file-in, it would
> call (project--file-completion-table dirs project). The default
> implementation of `project--file-completion-table' would be the code
> using `find-program'.
>
> I'll give that a try.

That works nicely; new patch attached.

-- 
-- Stephe

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

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 85f3907..41dc810 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -154,6 +154,14 @@ project--find-in-directory
     vc-directory-exclusion-list)
    grep-find-ignored-files))
 
+(cl-defgeneric project--file-completion-table (_project _dirs)
+  "Return a completion table for files in directories DIRS in PROJECT.
+If non-nil, DIRS is a list of absolute directories; it should be some
+subset of the project roots and external roots. If nil, the backend
+uses all the directories it knows about.
+PROJECT is used to find the project ignores and other project meta-data."
+  )
+
 (defgroup project-vc nil
   "Project implementation using the VC package."
   :version "25.1"
@@ -313,51 +321,57 @@ project--find-regexp-in
 
 ;;;###autoload
 (defun project-find-file ()
-  "Visit a file in the current project's roots.
-
-This is like `find-file', but it limits the file-name completion
-candidates to the files within the current project roots."
+  "Visit a file (with completion) in the current project's roots.
+The completion default is the filename at point, if one is
+recognized."
   (interactive)
   (let* ((pr (project-current t))
          (dirs (project-roots pr)))
-    (project--find-file-in dirs pr)))
+    (project--find-file-in (thing-at-point 'filename) dirs pr)))
 
 ;;;###autoload
 (defun project-or-external-find-file ()
   "Visit a file in the current project's roots or external roots.
-
-This is like `find-file', but it limits the file-name completion
-candidates to the files within the current project roots and external roots."
+The completion default is the filename at point, if one is
+recognized."
   (interactive)
   (let* ((pr (project-current t))
          (dirs (append
                 (project-roots pr)
                 (project-external-roots pr))))
-    (project--find-file-in dirs pr)))
+    (project--find-file-in (thing-at-point 'filename) dirs pr)))
 
 ;; FIXME: Uniquely abbreviate the roots?
-(defun project--find-file-in (dirs project)
+(cl-defmethod project--file-completion-table (project dirs)
+  "Default implementation using `find-program'."
   (require 'xref)
-  (let* ((all-files
-          (cl-mapcan
-           (lambda (dir)
-             (let ((command
-                    (format "%s %s %s -type f -print0"
-                            find-program
-                            dir
-                            (xref--find-ignores-arguments
-                             (project-ignores project dir)
-                             (expand-file-name dir)))))
-               (split-string (shell-command-to-string command) "\0" t)))
-           dirs))
-         (table (lambda (string pred action)
-                  (cond
-                   ((eq action 'metadata)
-                    '(metadata . ((category . project-file))))
-                   (t
-                    (complete-with-action action all-files string pred))))))
-    (find-file
-     (completing-read "Find file: " table nil t))))
+  (let ((all-files
+	 (cl-mapcan
+	  (lambda (dir)
+	    (let ((command
+		   (format "%s %s %s -type f -print0"
+			   find-program
+			   dir
+			   (xref--find-ignores-arguments
+			    (project-ignores project dir)
+			    (expand-file-name dir)))))
+	      (split-string (shell-command-to-string command) "\0" t)))
+	  dirs)))
+    (lambda (string pred action)
+      (cond
+       ((eq action 'metadata)
+	'(metadata . ((category . project-file))))
+       (t
+	(complete-with-action action all-files string pred))))
+    ))
+
+(defun project--find-file-in (filename dirs project)
+  "Complete FILENAME in DIRS in PROJECT, visit the file."
+  (find-file
+   (completing-read
+    (format "Find file (%s): " filename)
+    (project--file-completion-table project dirs)
+    nil t nil nil filename)))
 
 (provide 'project)
 ;;; project.el ends here
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 267853d..2fd7297 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -854,6 +854,7 @@ xref-etags-mode
 (declare-function semantic-symref-find-references-by-name "semantic/symref")
 (declare-function semantic-find-file-noselect "semantic/fw")
 (declare-function grep-expand-template "grep")
+(defvar ede-minor-mode) ;; ede.el
 
 (defun xref-collect-references (symbol dir)
   "Collect references to SYMBOL inside DIR.
@@ -948,6 +949,9 @@ xref--rgrep-command
    (xref--find-ignores-arguments ignores dir)))
 
 (defun xref--find-ignores-arguments (ignores dir)
+  "Convert IGNORES and DIR to a list of arguments for 'find'.
+IGNORES is a list of glob patterns.  DIR is an absolute
+directory, used as the root of the ignore globs."
   ;; `shell-quote-argument' quotes the tilde as well.
   (cl-assert (not (string-match-p "\\`~" dir)))
   (when ignores

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stephen Leake]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>
>> Dmitry Gutov <dgutov@yandex.ru> writes:
>>
>>> On 01/28/2016 04:04 AM, Stephen Leake wrote:
>>>> Any objections to the attached patch?
>>>
>>
>>> - More importantly, the new generic function should not do too much.
>>> It should return a completion table, which can dispatch to ede,
>>> ada-mode, or any other facility. I believe we've went over this a
>>> couple of times already.
>>
>> So instead of computing `table' in project--find-file-in, it would
>> call (project--file-completion-table dirs project). The default
>> implementation of `project--file-completion-table' would be the code
>> using `find-program'.
>>
>> I'll give that a try.
>
> That works nicely; new patch attached.

One change; if there is no filename at point, don't include "(nil)" in
the prompt:

(defun project--find-file-in (filename dirs project)
  "Complete FILENAME in DIRS in PROJECT, visit the file."
  (find-file
   (completing-read
    (if filename
        (format "Find file (%s): " filename)
      "Find file: ")
    (project--file-completion-table project dirs)
    nil t nil nil filename)))

-- 
-- Stephe

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stefan Monnier]

>>> - IIRC, using INITIAL-INPUT argument is semi-deprecated. Using DEFAULT
>>> is preferable,
>> I was not aware of M-n (more below).
> Hmm, the default value actually has one noticeable drawback here: if you
> just press RET, completing-read will accept it, and won't check whether the
> completion table includes the given value.  Then, find-file will be called
> with this raw (not expanded) value.
> Stefan, would you say INITIAL-INPUT is better for this use case?

Details of whether completing-read checks the validity of DEFAULT
against the completion table shouldn't determine whether we use DEFAULT
or INITIAL-INPUT.  After all, if we want it to be checked, we can
perform that check manually.

>> However, the default still does not show anywhere;
>> we should add it to the prompt.
> Agree.  If we end up using the default.

BTW we really should introduce new functions to replace completing-read
(and most others that derive from it) where the DEFAULT argument is made
more prominent (maybe even non-optional) and is automatically added to
the prompt.  Also, this new function could check that DEFAULT is
accepted by the completion tables before returning (or using) it.

This would also allow users to replace this function with one which
handles DEFAULT differently (e.g. inserting it as initial-input but
pre-selected so that delete-selection-mode deletes it in many cases).


        Stefan

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/28/2016 04:14 PM, Stefan Monnier wrote:

> Details of whether completing-read checks the validity of DEFAULT
> against the completion table shouldn't determine whether we use DEFAULT
> or INITIAL-INPUT.  After all, if we want it to be checked, we can
> perform that check manually.

But in the current use case, the "filename at point" will almost always 
_not_ be in the completion table. And it may even have several 
expansions, not just one. Because it's only a part of the file name: 
maybe it's the base name, or maybe it doesn't even have an extension.

Oh! Should we use minibuffer-default-add-function instead?

> This would also allow users to replace this function with one which
> handles DEFAULT differently (e.g. inserting it as initial-input but
> pre-selected so that delete-selection-mode deletes it in many cases).

I'm not sure that delete-selection-mode is the answer: the user might 
have to type some additional characters, to disambiguate the initial-input.

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stefan Monnier]

> But in the current use case, the "filename at point" will almost always
> _not_ be in the completion table. And it may even have several expansions,
> not just one. Because it's only a part of the file name: maybe it's the base
> name, or maybe it doesn't even have an extension.

So, we want to check it only if it's actually returned.  Sounds right.

>> This would also allow users to replace this function with one which
>> handles DEFAULT differently (e.g. inserting it as initial-input but
>> pre-selected so that delete-selection-mode deletes it in many cases).
> I'm not sure that delete-selection-mode is the answer:

It's not an answer, it's an alternative UI (which is the standard in
pretty much every other GUI application).


        Stefan

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/29/2016 12:11 AM, Stefan Monnier wrote:
>> But in the current use case, the "filename at point" will almost always
>> _not_ be in the completion table. And it may even have several expansions,
>> not just one. Because it's only a part of the file name: maybe it's the base
>> name, or maybe it doesn't even have an extension.
>
> So, we want to check it only if it's actually returned.  Sounds right.

I don't follow.

Do you mean returned by completing-read? I'd prefer not to allow the 
user to input it as-is (we call completing-read with REQUIRE-MATCH t).

Or, to put it differently, I'd like to be able to M-x project-find-file, 
then press RET, and:

- If there's just one match for the filename-at-point, see it opened 
(the completion must be expanded first).

- Otherwise, be forced to edit the input, to narrow down the matches, or 
to type something different, if the project has no matching files.

If I pass filename-at-point as DEFAULT, I don't get either of these items.

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stefan Monnier]

>> So, we want to check it only if it's actually returned.  Sounds right.
> I don't follow.
> Do you mean returned by completing-read?

Yes.  The DEFAULT would used in 3 places, basically:
- the prompt
- M-n
- RET
And it'd be checked only in RET (basically, the RET case would behave
similarly to `M-n RET').

> I'd prefer not to allow the user to input it as-is (we call
> completing-read with REQUIRE-MATCH t).

That's what I mean by "check".

> If I pass filename-at-point as DEFAULT, I don't get either of these items.

Not with completing-read, no.  But the new function would do that (as
well as change the calling convention so it'd be in charge of adding
DEFAULT into the prompt).


        Stefan

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/29/2016 12:11 AM, Stefan Monnier wrote:

>>> This would also allow users to replace this function with one which
>>> handles DEFAULT differently (e.g. inserting it as initial-input but
>>> pre-selected so that delete-selection-mode deletes it in many cases).
>> I'm not sure that delete-selection-mode is the answer:
>
> It's not an answer, it's an alternative UI (which is the standard in
> pretty much every other GUI application).

How would the user avoid deleting the default value, though? In other 
GUI applications, I'd click on the input, or press an arrow key. We 
avoid doing the former in Emacs. And probably neither would help anyway, 
because the region follows point in Emacs, instead of disappearing as 
soon as the user moves the cursor without holding Shift, like it happens 
in other applications.

We seem to deal with situations like that by putting the cursor before 
the input, so the user can press C-k. I probably prefer the M-n 
mechanics, though.

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/29/2016 01:44 AM, Stefan Monnier wrote:

>> If I pass filename-at-point as DEFAULT, I don't get either of these items.
>
> Not with completing-read, no.  But the new function would do that (as

Will it make completing-read-function deprecated as well? Unless we can 
implement the new function in terms of the old one, it seems like we'll 
have to deal with lots of moving parts.

For instance, with keeping icomplete-mode working appropriately in both 
cases. That probably means that we can't just use a keymap that makes 
C-m work differently, because icomplete-mode uses its own bindings.

For 25.1, I'd prefer a less-breaking solution than that.

> well as change the calling convention so it'd be in charge of adding
> DEFAULT into the prompt).

Can we still call the argument DEFAULT, if it won't always be defaulted 
to? Similarly, would it still be justified to call it "default" in the 
prompt?

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/28/2016 02:06 PM, Stephen Leake wrote:

> That works nicely; new patch attached.

Please install, with the following adjustments:

> +(cl-defgeneric project--file-completion-table (_project _dirs)

Let's call it project-file-completion-table (no double-dash). A private 
generic function is an oxymoron.

Should this table also include directories, BTW? That shouldn't make a 
difference in performance, and then we could add a project-dired 
command. Or just open Dired from project-find-file if the input is a 
directory (directories are files, too).

> +  "Return a completion table for files in directories DIRS in PROJECT.
> +If non-nil, DIRS is a list of absolute directories; it should be some
> +subset of the project roots and external roots. If nil, the backend
> +uses all the directories it knows about.

Why should we allow DIRS to be nil, and ask all backends to deal with 
that? Nobody passes nil to it. Out default implementation doesn't handle 
nil there.

> +PROJECT is used to find the project ignores and other project meta-data."
> +  )

Why not put the default implementation here? There's no need for the 
separate cl-defmethod declaration.

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stefan Monnier]

>>>> This would also allow users to replace this function with one which
>>>> handles DEFAULT differently (e.g. inserting it as initial-input but
>>>> pre-selected so that delete-selection-mode deletes it in many cases).
>>> I'm not sure that delete-selection-mode is the answer:
>> It's not an answer, it's an alternative UI (which is the standard in
>> pretty much every other GUI application).
> How would the user avoid deleting the default value, though?

Why cares.  The point is that the new functions's interface would allow
someone to implement a different UI, which some users like.

> We seem to deal with situations like that by putting the cursor before the
> input, so the user can press C-k.  I probably prefer the M-n 
> mechanics, though.

I think you missed in the text you quoted above the fact that I said
"allow users to replace this function".  I do not intend this alternative
behavior to be the default.


        Stefan

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stefan Monnier]

>>> If I pass filename-at-point as DEFAULT, I don't get either of these items.
>> Not with completing-read, no.  But the new function would do that (as
> Will it make completing-read-function deprecated as well?

I have no idea.  But I see no reason why the new function couldn't be
implemented on top of the existing ones.


        Stefan

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/29/2016 05:55 AM, Stefan Monnier wrote:

> I think you missed in the text you quoted above the fact that I said
> "allow users to replace this function".  I do not intend this alternative
> behavior to be the default.

Maybe I missed it, yes. You said "introduce new functions", not "new 
customization points", though.

But can't we already do that with completing-read-function? Its contract 
imposes no conditions on the presentation.

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/29/2016 05:57 AM, Stefan Monnier wrote:

> I have no idea.  But I see no reason why the new function couldn't be
> implemented on top of the existing ones.

Either it can be implemented on top of completing-read without digging 
into its internals (which would be great, but I don't see how; 
suggestions welcome), or completing-read-function won't work with the 
code that uses the new snazzy-completing-read function.

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stefan Monnier]

>> I think you missed in the text you quoted above the fact that I said
>> "allow users to replace this function".  I do not intend this alternative
>> behavior to be the default.
> Maybe I missed it, yes. You said "introduce new functions", not "new
> customization points", though.

The main purpose of the new functions would be to improve the handing of
DEFAULT and encourage coders to use it.  IIUC the way we handle default
didn't exist when completing-read was first defined, which is why it's
hidden so far down the list of optional arguments.

It so happens that it would also provide good customization points, yes.

> But can't we already do that with completing-read-function?  Its contract
> imposes no conditions on the presentation.

We can, yes, but completing-read will receive a prompt string which
sometimes includes the default and would then have to try and find it to
remove it from there (if the customized UI wants to put default as
initial input instead).  This can't be done cleanly and reliably currently.
[ The same problem affects minibuffer-electric-default-mode, of course.  ]


        Stefan

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stefan Monnier]

> Either it can be implemented on top of completing-read without digging into
> its internals

As I said "I see no reason why" that wouldn't be the case.

> (which would be great, but I don't see how; suggestions welcome),

What problems do you foresee?


        Stefan

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/29/2016 05:03 PM, Stefan Monnier wrote:

> The main purpose of the new functions would be to improve the handing of
> DEFAULT and encourage coders to use it.  IIUC the way we handle default
> didn't exist when completing-read was first defined, which is why it's
> hidden so far down the list of optional arguments.

The fact that completing-read is expected to return an empty string on 
empty input, irrespective of REQUIRE-MATCH, seems to be another problem.

On the other hand, if we also fix that, the result would turn out to be 
overall less versatile.

> It so happens that it would also provide good customization points, yes.

What customization points, though? Do you expect it to be used with nadvice?

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/29/2016 05:05 PM, Stefan Monnier wrote:

> What problems do you foresee?

The usual lack of imagination?

With your generous encouragement, I wrote this:

(defun completing-read-strict (prompt
                                collection &optional predicate
                                hist default inherit-input-method)
   (let* ((new-prompt (if default
                          (format "%s (default %s): " prompt default)
                        (format "%s: " prompt)))
          (res (completing-read new-prompt
                                collection predicate t
                                nil hist default inherit-input-method))
          try)
     (if (and (equal res default)
              (not (test-completion res collection predicate))
              (not (and (setq try (car (completion-try-completion
                                        res collection predicate (length 
res))))
                        (setq res try)
                        (test-completion res collection predicate))))
         (completing-read (format "%s: " prompt)
                          collection predicate t res hist nil
                          inherit-input-method)
       res)))

I can use it in completing-read-strict to satisfy both points mentioned 
previously. But after trying it out, I'm not sure about the general 
applicability of the DEFAULT semantics here. It just seems awkward:

First, you tell the user that the default is so-and-so. But if they 
choose to trust you (and aren't even surprised that the default isn't a 
proper absolute filename), in the event that the default does not 
correspond to a particular filename, you drop them into the same prompt 
once again, but without the "default" part. And now the input contains 
either:

- The previous "default" which has _some_ matches, and the user has to 
figure that out, because I can't force the completing-read-function to 
show the matches right away (the default one doesn't, it waits for me to 
press TAB or RET; it's easier with icomplete-mode on).

- The previous "default" input which has no matches. The user doesn't 
know about that right away either, so they must feel disoriented in this 
case too. Seeing (no matches) at this step would be better.

- Nothing. Which could make sense if the previous "default" didn't 
actually match anything, but the user must be even more surprised in 
this case ("where did my default go?").

We could filter out the last two cases before calling 
completing-read-strict, and don't pass DEFAULT in at all then, but this 
makes the new function less necessary, and the first case remains confusing.

And as long as completing-read-strict, by itself, does allow the case of 
"default that doesn't match anything", that makes me doubt its semantics.

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stephen Leake]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 01/28/2016 02:06 PM, Stephen Leake wrote:
>
>> That works nicely; new patch attached.
>
> Please install, with the following adjustments:
>
>> +(cl-defgeneric project--file-completion-table (_project _dirs)
>
> Let's call it project-file-completion-table (no double-dash). A
> private generic function is an oxymoron.

Ok.

> Should this table also include directories, BTW? That shouldn't make a
> difference in performance, and then we could add a project-dired
> command. Or just open Dired from project-find-file if the input is a
> directory (directories are files, too).

I have not thought about that at all. I never felt like I wanted to do
it. Part of the point of project-find-file is that it allows me to
_ignore_ the directory structure.

So no, I don't think we want to expand this table to include
directories.

A buffer that lists the project directories might be useful for some
things; again, I've never felt like I need it.

>> +  "Return a completion table for files in directories DIRS in PROJECT.
>> +If non-nil, DIRS is a list of absolute directories; it should be some
>> +subset of the project roots and external roots. If nil, the backend
>> +uses all the directories it knows about.
>
> Why should we allow DIRS to be nil, and ask all backends to deal with
> that? Nobody passes nil to it. Out default implementation doesn't
> handle nil there.

I'll delete that part.

>> +PROJECT is used to find the project ignores and other project meta-data."
>> +  )
>
> Why not put the default implementation here? There's no need for the
> separate cl-defmethod declaration.

I find it cleaner to have a separate cl-defgeneric, so the doc string is
clearly about the generic requirement, and not the default
implementation.

And it leaves the default implementation in the same place in the file,
so the git diff is smaller and more usefull.

Pushed.

--
-- Stephe

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/30/2016 02:55 AM, Stephen Leake wrote:

> I have not thought about that at all. I never felt like I wanted to do
> it.

I think I did, once or twice over the last few weeks. Projectile 
includes such command, so putting directories into the completion table 
might be considered useful. Anyway, no hurry, we're still allowed to 
break things.

 > Part of the point of project-find-file is that it allows me to
 > _ignore_ the directory structure.

And you'd be able to "ignore" it to an extent, when visiting 
directories, by only typing a directory's base name, if it's unique.

I don't think we'll introduce any danger to visit a directory when you 
wanted to visit a file, this way.

> I find it cleaner to have a separate cl-defgeneric, so the doc string is
> clearly about the generic requirement, and not the default
> implementation.

You've got a point, but I somewhat prefer them to be kept together: less 
indirection, easier to follow the most common flow of code.

More importantly, both project-ignores and (actually) 
project-external-roots define their default implementations inline. I 
think it's better to keep to one style.

> And it leaves the default implementation in the same place in the file,
> so the git diff is smaller and more usefull.

I'd just put the cl-defgeneric where the defun was, really.

> Pushed.

Thanks.

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stefan Monnier]

> What customization points, though? Do you expect it to be used with nadvice?

Sure,


        Stefan

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Juri Linkov]

> How would the user avoid deleting the default value, though? In other GUI
> applications, I'd click on the input, or press an arrow key. We avoid doing
> the former in Emacs. And probably neither would help anyway, because the
> region follows point in Emacs, instead of disappearing as soon as the user
> moves the cursor without holding Shift, like it happens in
> other applications.

Never tried Shift-arrows?  The region deactivates as soon as you move
the cursor without holding Shift.  We could use this feature in the
minibuffer either for initial input or if there is a default value
(might require a new defcustom to enable this).

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/31/2016 02:32 AM, Juri Linkov wrote:

> Never tried Shift-arrows?  The region deactivates as soon as you move
> the cursor without holding Shift.  We could use this feature in the
> minibuffer either for initial input or if there is a default value
> (might require a new defcustom to enable this).

I suppose.

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stefan Monnier]

> (defun completing-read-strict (prompt
>                                collection &optional predicate
>                                hist default inherit-input-method)

I think `default' should come earlier, maybe even before `&optional'.

> I can use it in completing-read-strict to satisfy both points mentioned
> previously. But after trying it out, I'm not sure about the general
> applicability of the DEFAULT semantics here. It just seems awkward:

You can probably get a cleaner behavior if you change
minibuffer-complete-and-exit so that it does most of the new work (but
only when called from completing-read-strict).

> And as long as completing-read-strict, by itself, does allow the case of
> "default that doesn't match anything", that makes me doubt its semantics.

We could check `default' directly when we receive it, but I'd rather not
do that:
- it would ruin efforts to make completion tables more lazy.
- it's rarely useful, but in 99% of the cases, we already know that
  the default is a valid value.
But yes, it should probably be considered "bad karma" for the caller to
provide an invalid `default'.


        Stefan

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/31/2016 09:18 AM, Stefan Monnier wrote:
>> (defun completing-read-strict (prompt
>>                                 collection &optional predicate
>>                                 hist default inherit-input-method)
>
> I think `default' should come earlier, maybe even before `&optional'.

We can't always provide a non-nil default value (such as in the case of 
project-find-file), and &optional is a good indicator that the 
parameters after it are null-able. I'd put DEFAULT after PREDICATE, or 
right before it.

> You can probably get a cleaner behavior if you change
> minibuffer-complete-and-exit so that it does most of the new work (but
> only when called from completing-read-strict).

That could improve completing-read-default's behavior, but wouldn't help 
when any other completing-read-function is used. Which was my main concern.

But yes, we can do that (probably on master).

> We could check `default' directly when we receive it, but I'd rather not
> do that:
> - it would ruin efforts to make completion tables more lazy.
> - it's rarely useful, but in 99% of the cases, we already know that
>    the default is a valid value.
> But yes, it should probably be considered "bad karma" for the caller to
> provide an invalid `default'.

Right, I already removed the advance checks on DEFAULT from 
project--completing-read-strict. We could document the "bad karma" 
consideration, and then the main missing thing would be to show the 
completions for DEFAULT, or the "no matches" message, right after the 
user presses RET.

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stefan Monnier]

>>> (defun completing-read-strict (prompt
>>> collection &optional predicate
>>> hist default inherit-input-method)
>> I think `default' should come earlier, maybe even before `&optional'.
> We can't always provide a non-nil default value (such as in the case of
> project-find-file), and &optional is a good indicator that the parameters
> after it are null-able. I'd put DEFAULT after PREDICATE, or right before it.

The idea of putting it before &optional is to force people to try and
think if there'd be a good default.  Lots of packages use
completing-read without passing a default simply because the author
doesn't know about that feature.

>> You can probably get a cleaner behavior if you change
>> minibuffer-complete-and-exit so that it does most of the new work (but
>> only when called from completing-read-strict).
> That could improve completing-read-default's behavior, but wouldn't help
> when any other completing-read-function is used. Which was my main concern.

Other functions might still use minibuffer-complete-and-exit.
And for other functions we should just make sure that the behavior is
still correct (even if not perfect) and that they have enough info to
make the behavior "perfect".

> But yes, we can do that (probably on master).

All this discussion is obviously on master, yes.


        Stefan

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Dmitry Gutov]

On 01/31/2016 05:23 PM, Stefan Monnier wrote:

> The idea of putting it before &optional is to force people to try and
> think if there'd be a good default.  Lots of packages use
> completing-read without passing a default simply because the author
> doesn't know about that feature.

IMHO, that might be a good reason to put DEFAULT right after &optional, 
but not before. We'll mention this argument several times more in the 
docstring, so it's not likely to be forgotten.

On the other hand, lots of GUI applications use the previous input as 
the default, in the absence of better choice. So we can consider making 
HIST non-optional, and use it for the default value when DEFAULT is not 
specified.

> Other functions might still use minibuffer-complete-and-exit.
> And for other functions we should just make sure that the behavior is
> still correct (even if not perfect) and that they have enough info to
> make the behavior "perfect".

I suppose we can add a new global variable, that both 
minibuffer-complete-and-exit and the non-default 
completion-read-function's will look at.

Re: make project--find-in-file generic, add interactive filename to project-find-file

[Stefan Monnier]

> I suppose we can add a new global variable, that both
> minibuffer-complete-and-exit and the non-default
> completion-read-function's will look at.

Right.  Maybe we could even (re)use minibuffer-completion-confirm.


        Stefan