bug#12619: completion-at-point and changing buffer

bug#12619: completion-at-point and changing buffer

[Jorgen Schaefer]

Hello!
In the process of porting our IRC client to the
`completion-at-point-functions' interface, we have noticed the
following behavior:

When using cycle completion (using `completion-cycle-threshold'), the
completion cycle gets aborted if the buffer contents are modified. For
an IRC buffer, this means that the completion cycle terminates when new
text arrives. The same problem should be the case for other
process-related buffers, like shells with regular output or similar.

This is very confusing because the text change is expected and does not
interfere with the completion text at all.

While hunting down this bug, I have found two changes to minibuffer.el
that solve my problem and *seem* not to introduce (major) other
problems.

First, in `completion-at-point', `completion-in-region-mode-predicate'
is set to a function that compares the old start value with the new
using `eq'. This prevents markers to work in this case. Simply
replacing `eq' with `=' means markers work, so the positions used by
completion just move together with text changes.

Second, `completion--cache-all-sorted-completions' adds a function to
`after-change-functions' that cleans up the cache of sorted
completions. I'm not entirely sure why it does so, but my patch adds
that function only if not both of the returned positions are markers.

I'm unsure about possible ramifications of the second change. It might
be that completion needs to terminate on text change that will not be
caught by the other tests, but that I have not thought of. But if not,
or if those cases are minor, these changes would be really useful.


2012-10-10  Jorgen Schaefer  <forcer@forcix.cx>

        * minibuffer.el (completion--cache-all-sorted-completions):
          Don't clean cache on text change if the completion code uses
          markers, as those follow text changes.

        * minibuffer.el (completion-at-point): Compare completion
          positions using `=' instead of `eq' to support markers in
          addition of fixed integers.


-----8<----------8<-----
--- lisp/minibuffer.el  2012-10-06 17:29:15 +0000                               
+++ lisp/minibuffer.el  2012-10-10 19:10:40 +0000                               
@@ -1047,8 +1047,12 @@
         (_     t)))))                                                          
                                                                                
 (defun completion--cache-all-sorted-completions (comps)                        
-  (add-hook 'after-change-functions                                            
-               'completion--flush-all-sorted-completions nil t)                
+  ;; Flush the cache on text change only if the given indicators for           
+  ;; the completion are not markers that move with the change.                 
+  (when (or (not (markerp (nth 1 completion-in-region--data)))                 
+            (not (markerp (nth 2 completion-in-region--data))))                
+    (add-hook 'after-change-functions                                          
+              'completion--flush-all-sorted-completions nil t))                
   (setq completion-all-sorted-completions comps))                              
                                                                                
 (defun completion--flush-all-sorted-completions (&rest _ignore)                
@@ -1883,7 +1887,7 @@
              (completion-in-region-mode-predicate                              
               (lambda ()                                                       
                 ;; We're still in the same completion field.                   
-                (eq (car-safe (funcall hookfun)) start))))                     
+                (= (car-safe (funcall hookfun)) start))))                      
         (completion-in-region start end collection                             
                               (plist-get plist :predicate))))                  
      ;; Maybe completion already happened and the function returned t.         
----->8---------->8-----

Regards,
	-- Jorgen

bug#12619: completion-at-point and changing buffer

[Stefan Monnier]

> When using cycle completion (using `completion-cycle-threshold'), the
> completion cycle gets aborted if the buffer contents are modified. For
> an IRC buffer, this means that the completion cycle terminates when new
> text arrives. The same problem should be the case for other
> process-related buffers, like shells with regular output or similar.

Indeed, good point.

> First, in `completion-at-point', `completion-in-region-mode-predicate'
> is set to a function that compares the old start value with the new
> using `eq'. This prevents markers to work in this case.  Simply
> replacing `eq' with `=' means markers work, so the positions used by
> completion just move together with text changes.

Sounds OK, tho we should make sure that those values can't be nil
or some other non-numeric thingy.

> Second, `completion--cache-all-sorted-completions' adds a function to
> `after-change-functions' that cleans up the cache of sorted
> completions. I'm not entirely sure why it does so, but my patch adds
> that function only if not both of the returned positions are markers.

Hmm...but if the buffer modification happens right in the middle of the
completion text what should we do?  Should we really ignore
this modification?


        Stefan

bug#12619: completion-at-point and changing buffer

[Jorgen Schaefer]

On Wed, 10 Oct 2012 20:37:42 -0400
Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> > First, in `completion-at-point',
> > `completion-in-region-mode-predicate' is set to a function that
> > compares the old start value with the new using `eq'. This prevents
> > markers to work in this case.  Simply replacing `eq' with `=' means
> > markers work, so the positions used by completion just move
> > together with text changes.
> 
> Sounds OK, tho we should make sure that those values can't be nil
> or some other non-numeric thingy.

True, I think some of those values can be nil in some cases (didn't
happen during my testing, though). Pity that neither `eql' nor `equal'
work on markers.

> > Second, `completion--cache-all-sorted-completions' adds a function
> > to `after-change-functions' that cleans up the cache of sorted
> > completions. I'm not entirely sure why it does so, but my patch adds
> > that function only if not both of the returned positions are
> > markers.
> 
> Hmm...but if the buffer modification happens right in the middle of
> the completion text what should we do?  Should we really ignore
> this modification?

Actually, I have no idea what would happen then and couldn't make a
test case to reproduce it easily. :-)

The patch below flushes the cache if the changed text intersects with
the completion text.


--- lisp/minibuffer.el	2012-10-06 17:29:15 +0000
+++ lisp/minibuffer.el	2012-10-11 17:56:42 +0000
@@ -1048,12 +1048,29 @@
 
 (defun completion--cache-all-sorted-completions (comps)
   (add-hook 'after-change-functions
-               'completion--flush-all-sorted-completions nil t)
+            'completion--flush-all-sorted-completions-after-change nil t)
   (setq completion-all-sorted-completions comps))
 
-(defun completion--flush-all-sorted-completions (&rest _ignore)
+(defun completion--flush-all-sorted-completions-after-change (change-start change-end pre-change-length)
+  (let ((start (nth 1 completion-in-region--data))
+        (end (nth 2 completion-in-region--data)))
+    (when (or
+           ;; We don't even have completion data
+           (not start)
+           (not end)
+           ;; Change completely before our completion, but our
+           ;; completion didn't use markers.
+           (and (<= change-end start)
+                (not (and (markerp start)
+                          (markerp end))))
+           ;; Change overlaps our completion, regardless of markers.
+           (not (or (< change-end start)
+                    (< end change-start))))
+      (completion--flush-all-sorted-completions))))
+
+(defun completion--flush-all-sorted-completions ()
   (remove-hook 'after-change-functions
-               'completion--flush-all-sorted-completions t)
+               'completion--flush-all-sorted-completions-after-change t)
   (setq completion-cycling nil)
   (setq completion-all-sorted-completions nil))
 
@@ -1882,8 +1899,12 @@
       (let* ((completion-extra-properties plist)
              (completion-in-region-mode-predicate
               (lambda ()
-                ;; We're still in the same completion field.
-                (eq (car-safe (funcall hookfun)) start))))
+                (let ((old-start (car-safe (funcall hookfun))))
+                  ;; We're still in the same completion field.
+                  (condition-case error
+                      (= old-start start)
+                    (error
+                     (eq old-start start)))))))
         (completion-in-region start end collection
                               (plist-get plist :predicate))))
      ;; Maybe completion already happened and the function returned t.



(I have no idea at what point patches are considered non-trivial for
copyright reasons, but there should be an assignment "for all future
contributions to Emacs" with the FSF.)

Regards,
	-- Jorgen

bug#12619: completion-at-point and changing buffer

[Stefan Monnier]

I've just installed a patch which should solve those problems.
Can you confirm it fixes your case?


        Stefan

bug#12619: completion-at-point and changing buffer

[Jorgen Schaefer]

On Tue, 23 Oct 2012 23:19:21 -0400
Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> I've just installed a patch which should solve those problems.
> Can you confirm it fixes your case?

I'm afraid it doesn't (tested with trunk/110646). The problem is that
`minibuffer-force-complete' uses `field-beginning' and `field-end',
neither of which returns a marker. So even if the original function
uses markers, at this point they are turned into absolute buffer
positions.

Wrapping both calls in `copy-marker' fixes the problem for me.

Regards,
	-- Jorgen

bug#12619: completion-at-point and changing buffer

[Stefan Monnier]

>> I've just installed a patch which should solve those problems.
>> Can you confirm it fixes your case?
> I'm afraid it doesn't (tested with trunk/110646). The problem is that
> `minibuffer-force-complete' uses `field-beginning' and `field-end',
> neither of which returns a marker. So even if the original function
> uses markers, at this point they are turned into absolute buffer
> positions.
> Wrapping both calls in `copy-marker' fixes the problem for me.

You mean the patch below?
I guess you're right, it's still needed for the use of `start' in:

              (lambda () "Cycle through the possible completions."
                (interactive)
                (let ((completion-extra-properties extra-prop))
                  (completion-in-region start (point) table pred)))))

Can you confirm that this patch fixes it?


        Stefan


=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2012-10-24 03:22:21 +0000
+++ lisp/minibuffer.el	2012-10-24 17:47:17 +0000
@@ -1114,7 +1114,7 @@
   ;; FIXME: Need to deal with the extra-size issue here as well.
   ;; FIXME: ~/src/emacs/t<M-TAB>/lisp/minibuffer.el completes to
   ;; ~/src/emacs/trunk/ and throws away lisp/minibuffer.el.
-  (let* ((start (field-beginning))
+  (let* ((start (copy-marker (field-beginning)))
          (end (field-end))
          ;; (md (completion--field-metadata start))
          (all (completion-all-sorted-completions))

bug#12619: completion-at-point and changing buffer

[Jorgen Schaefer]

On Wed, 24 Oct 2012 13:48:33 -0400
Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> You mean the patch below?
> I guess you're right, it's still needed for the use of `start' in:
> 
>               (lambda () "Cycle through the possible completions."
>                 (interactive)
>                 (let ((completion-extra-properties extra-prop))
>                   (completion-in-region start (point) table pred)))))
> 
> Can you confirm that this patch fixes it?

It does indeed. :-)

	-- Jorgen

bug#12619: completion-at-point and changing buffer

[Stefan Monnier]

>> Can you confirm that this patch fixes it?
> It does indeed. :-)

Thanks, installed,


        Stefan