bug#7215: 24.0.50; Minibuffer path completion mispositions point

bug#7215: 24.0.50; Minibuffer path completion mispositions point

[Tim Van Holder]

Recipe:

$ mkdir directory
$ cd directory
$ mkdir subdir
$ cd subdir
$ emacs -Q
  C-x C-f

At this point, the minibuffer will contain (with * indicating point):

  Find file: .../directory/subdir/*

Now delete most of "directory", leaving the minibuffer as:

  Find file: .../dir*/subdir/

Now press [tab]. This correctly completes "dir" back to "directory", but
also moves point backwards, resulting in:

  Find file: .../di*rectory/subdir/

The amount point moves seems to be relative to the place it should end
up at (it will always be after the "di" regardless of whether the
completion was for "di", "dir" or "direc"). It also seems to be related
to the size of the subsequent path component; doing the same tab
completion for the portion of the path before "directory" makes point
jump much further back.

For the last path component (subdir in this case) it _looks_ fine,
resulting in

  Find file: .../directory/subdir*/

But simply adding a single letter after the trailing '/' shows that this
is just the one case where it looks like it's doing the right thing;
trying to complete "subdir" from "sub" then yields:

  Find file: .../directory/subdi*r/x


In GNU Emacs 24.0.50.2 (i686-pc-linux-gnu, GTK+ Version 2.8.20)
 of 2010-10-14 on leeloo
Windowing system distributor `The Cygwin/X Project', version 11.0.10503000
configured using `configure  '--with-x''

bug#7215: 24.0.50; Minibuffer path completion mispositions point

[Stephen Berman]

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

On Thu, 14 Oct 2010 15:22:16 +0200 Tim Van Holder <tim.vanholder@gmail.com> wrote:

> Recipe:
>
> $ mkdir directory
> $ cd directory
> $ mkdir subdir
> $ cd subdir
> $ emacs -Q
>   C-x C-f
>
> At this point, the minibuffer will contain (with * indicating point):
>
>   Find file: .../directory/subdir/*
>
> Now delete most of "directory", leaving the minibuffer as:
>
>   Find file: .../dir*/subdir/
>
> Now press [tab]. This correctly completes "dir" back to "directory", but
> also moves point backwards, resulting in:
>
>   Find file: .../di*rectory/subdir/
>
> The amount point moves seems to be relative to the place it should end
> up at (it will always be after the "di" regardless of whether the
> completion was for "di", "dir" or "direc"). It also seems to be related
> to the size of the subsequent path component; doing the same tab
> completion for the portion of the path before "directory" makes point
> jump much further back.
>
> For the last path component (subdir in this case) it _looks_ fine,
> resulting in
>
>   Find file: .../directory/subdir*/
>
> But simply adding a single letter after the trailing '/' shows that this
> is just the one case where it looks like it's doing the right thing;
> trying to complete "subdir" from "sub" then yields:
>
>   Find file: .../directory/subdi*r/x

The following patch appears to fix this problem (that is, it leaves
point before the `/' following the current path component; note that in
Emacs 23.1, path completion leaves point after the `/'):


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: completion patch --]
[-- Type: text/x-patch, Size: 906 bytes --]

*** /data/steve/bzr/emacs/trunk/lisp/minibuffer.el	2010-10-08 13:54:31.000000000 +0200
--- /data/steve/bzr/emacs/quickfixes/lisp/minibuffer.el	2010-10-15 15:46:18.000000000 +0200
***************
*** 574,580 ****
            ;; Insert in minibuffer the chars we got.
            (completion--replace beg end completion))
  	;; Move point to its completion-mandated destination.
! 	(forward-char (- comp-pos (length completion)))
  
          (if (not (or unchanged completed))
  	   ;; The case of the string changed, but that's all.  We're not sure
--- 574,580 ----
            ;; Insert in minibuffer the chars we got.
            (completion--replace beg end completion))
  	;; Move point to its completion-mandated destination.
! 	;; (forward-char (- comp-pos (length completion)))
  
          (if (not (or unchanged completed))
  	   ;; The case of the string changed, but that's all.  We're not sure

[-- Attachment #3: Type: text/plain, Size: 332 bytes --]


Clearly, the movement calculation doesn't DTRT for path completion.  I
tried the patch with other types of completion, e.g. function and
variable names, and it also seemed fine.  But the movement is obviously
intended, so there must be cases I have overlooked where it is needed;
maybe for certain completion styles?

Steve Berman

bug#7215: 24.0.50; Minibuffer path completion mispositions point

[Stefan Monnier]

> Now delete most of "directory", leaving the minibuffer as:

>   Find file: .../dir*/subdir/

> Now press [tab]. This correctly completes "dir" back to "directory", but
> also moves point backwards, resulting in:

>   Find file: .../di*rectory/subdir/

I've installed the patch below into the emacs-23 branch.  This should
fix the problem.  Please confirm,


        Stefan
        
        
=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2010-10-03 19:50:14 +0000
+++ lisp/minibuffer.el	2010-10-17 17:26:12 +0000
@@ -498,10 +498,11 @@
       (setq suffix-len (1+ suffix-len)))
     (unless (zerop suffix-len)
       (setq end (- end suffix-len))
-      (setq newtext (substring newtext 0 (- suffix-len)))))
+      (setq newtext (substring newtext 0 (- suffix-len))))
   (goto-char beg)
   (insert newtext)
-  (delete-region (point) (+ (point) (- end beg))))
+    (delete-region (point) (+ (point) (- end beg)))
+    (forward-char suffix-len)))
 
 (defun completion--do-completion (&optional try-completion-function)
   "Do the completion and return a summary of what happened.

bug#7215: 24.0.50; Minibuffer path completion mispositions point

[Tim Van Holder]

On 17 October 2010 19:31, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> I've installed the patch below into the emacs-23 branch.  This should
> fix the problem.  Please confirm,

I'm on trunk, not the emacs-23 branch.
However, I (manually) applied the same change to trunk and the problem
did indeed go away.

Does mark matter in the minibuffer? If so, perhaps it should be
forward-char-nomark?

Note: the diff you posted seems to ignore whitespace, which made it
slightly harder to twig that you were extending the let, because the
goto-char and indent didn't change their indentation.

bug#7215: 24.0.50; Minibuffer path completion mispositions point

[Stefan Monnier]

>> I've installed the patch below into the emacs-23 branch.  This should
>> fix the problem.  Please confirm,

> I'm on trunk, not the emacs-23 branch.

Yes, but the bug was there as well.  It'll get merged into trunk in due time.

> However, I (manually) applied the same change to trunk and the problem
> did indeed go away.

Thank you for confirming.

> Does mark matter in the minibuffer?

Yes.

> If so, perhaps it should be forward-char-nomark?

forward-char is a low-level function which does not touch the mark.
forward-char-nomark OTOH is a command (only defined if you load
pc-select.el) which does affect the mark.  So `forward-char' is what we
want here.

> Note: the diff you posted seems to ignore whitespace, which made it
> slightly harder to twig that you were extending the let, because the
> goto-char and indent didn't change their indentation.

It's a tradeoff: OT1H you don't see that the indentation changed, but
OTOH you do see that this part of the code has not been modified.


        Stefan

bug#7215: 24.0.50; Minibuffer path completion mispositions point

[Stefan Monnier]

>> I'm on trunk, not the emacs-23 branch.
>> However, I (manually) applied the same change to trunk and the problem
>> did indeed go away.
> Thank you for confirming.

Closing, then,


        Stefan