bug#68487: [PATCH] Make jump commands usable for all skeletons

[Martin Marshall <law@martinmarshall.com>]

Finally got around to looking at this again.  

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I'm having trouble understanding the design behind `expand.el`, but IIUC
> `expand-list` is basically the variable through which interaction with
> other things is expected to take place, so I think it's fair to make
> `skeleton.el` set `expand-list` whereas `expand-pos/index` seem like
> internal vars and `skeleton.el` shouldn't touch them.

That sounds right.  But outside code also needs a way to trigger
population of `expand-pos` from `expand-list`.  I tried to do this with
some of the new changes copied below.

> Ideally this should go along with the removal of the use of a vector in
> `expand-list`, which not only is odd given its name but is odd because
> it seems completely useless.

Nothing (at least nothing in Emacs core) stores a vector to
`expand-list`.  So I'm curious why `expand-abbrev-hook` was written to
account for that possibility.

Changing `expand-abbrev-hook` to expect `expand-list` to actually be a
list (as you did with your patch) makes sense to me.

> IOW my reading of the code suggests the code would work just as well
> with the patch below.

Yes, I applied your patch and added more changes to separate
functionality between (a) expansion and (b) populating `expand-pos` with
the marks that the "expand-jump" commands use.

I also removed some more functions that either became obsolete because
of the changes from your patch or were already not being used anywhere.

These changes make expand.el much more compact and easier to understand,
not to mention the improved functionality.

Still a work in progress though.

What do you think?

-- 
Best regards,
Martin Marshall

diff --git a/lisp/expand.el b/lisp/expand.el
index f32ab101224..56329dd9805 100644
--- a/lisp/expand.el
+++ b/lisp/expand.el
@@ -331,60 +331,43 @@ expand-abbrev-hook
       (let ((p (point)))
 	(setq expand-point nil)
 	;; don't expand if the preceding char isn't a word constituent
-	(if (and (eq (char-syntax (preceding-char))
-		     ?w)
-		 (expand-do-expansion))
-	    (progn
-	      ;; expand-point tells us if we have inserted the text
-	      ;; ourself or if it is the hook which has done the job.
-	      (if expand-point
-		  (progn
-		    (if (vectorp expand-list)
-			(expand-build-marks expand-point))
-		    (indent-region p expand-point nil))
-		;; an outside function can set expand-list to a list of
-		;; markers in reverse order.
-		(if (listp expand-list)
-		    (setq expand-index 0
-			  expand-pos (expand-list-to-markers expand-list)
-			  expand-list nil)))
-	      (run-hooks 'expand-expand-hook)
+	(if (eq (char-syntax (preceding-char)) ?w)
+            (progn
+              (delete-char (- (length last-abbrev-text)))
+              (let* ((vect (symbol-value last-abbrev))
+	             (text (aref vect 0))
+	             (position (aref vect 1))
+	             (jump-args (aref vect 2))
+	             (hook (aref vect 3))
+                     (startpos (point)))
+                (cond (text
+	               (insert text)
+	               (setq expand-point (point))))
+                (if jump-args
+                    (setq expand-list (nreverse
+                                       (mapcar (lambda (offset)
+                                                 (+ startpos -1 offset))
+                                               (cdr jump-args)))))
+                (if position
+	            (backward-char position))
+                (if hook
+	            (funcall hook))
+                (if expand-point
+                    (indent-region p expand-point nil))
+                (unless hook
+                  (expand-do-expansion)))
 	      t)
 	  nil))
     nil))
 
 (defun expand-do-expansion ()
-  (delete-char (- (length last-abbrev-text)))
-  (let* ((vect (symbol-value last-abbrev))
-	 (text (aref vect 0))
-	 (position (aref vect 1))
-	 (jump-args (aref vect 2))
-	 (hook (aref vect 3)))
-    (cond (text
-	   (insert text)
-	   (setq expand-point (point))))
-    (if jump-args
-        (funcall #'expand-build-list (car jump-args) (cdr jump-args)))
-    (if position
-	(backward-char position))
-    (if hook
-	(funcall hook))
-    t))
-
-(defun expand-abbrev-from-expand (word)
-  "Test if an abbrev has a hook."
-  (or
-   (and (intern-soft word local-abbrev-table)
-	(symbol-function (intern-soft word local-abbrev-table)))
-   (and (intern-soft word global-abbrev-table)
-	(symbol-function (intern-soft word global-abbrev-table)))))
-
-(defun expand-previous-word ()
-  "Return the previous word."
-  (save-excursion
-    (let ((p (point)))
-      (backward-word 1)
-      (buffer-substring p (point)))))
+  ;; expand-point tells us if we have inserted the text
+  ;; ourself or if it is the hook which has done the job.
+  (if (listp expand-list)
+      (setq expand-index 0
+	    expand-pos (expand-list-to-markers expand-list)
+	    expand-list nil))
+  (run-hooks 'expand-expand-hook))
 
 ;;;###autoload
 (defun expand-jump-to-previous-slot ()
@@ -415,38 +398,6 @@ expand-jump-to-next-slot
 ;;;###autoload (define-key abbrev-map "p" 'expand-jump-to-previous-slot)
 ;;;###autoload (define-key abbrev-map "n" 'expand-jump-to-next-slot)
 
-(defun expand-build-list (len l)
-  "Build a vector of offset positions from the list of positions."
-  (expand-clear-markers)
-  (setq expand-list (vconcat l))
-  (let ((i 0)
-	(lenlist (length expand-list)))
-    (while (< i lenlist)
-      (aset expand-list i (- len (1- (aref expand-list i))))
-      (setq i (1+ i)))))
-
-(defun expand-build-marks (p)
-  "Transform the offsets vector into a marker vector."
-  (if expand-list
-      (progn
-	(setq expand-index 0)
-	(setq expand-pos (make-vector (length expand-list) nil))
-	(let ((i (1- (length expand-list))))
-	  (while (>= i 0)
-	    (aset expand-pos i (copy-marker (- p (aref expand-list i))))
-	    (setq i (1- i))))
-	(setq expand-list nil))))
-
-(defun expand-clear-markers ()
-  "Make the markers point nowhere."
-  (if expand-pos
-      (progn
-    (let ((i (1- (length expand-pos))))
-      (while (>= i 0)
-	(set-marker (aref expand-pos i) nil)
-	(setq i (1- i))))
-    (setq expand-pos nil))))
-
 (defun expand-in-literal ()
   "Test if we are in a comment or in a string."
   (save-excursion
@@ -477,8 +428,9 @@ expand-list-to-markers
 ;; Used in `skeleton-end-hook' to fetch the positions for  @ skeleton tags.
 ;; See `skeleton-insert'.
 (defun expand-skeleton-end-hook ()
-  (if skeleton-positions
-      (setq expand-list skeleton-positions)))
+  (when skeleton-positions
+    (setq expand-list skeleton-positions)
+    (expand-do-expansion)))
 
 (add-hook 'skeleton-end-hook (function expand-skeleton-end-hook))