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

From: Stefan Monnier <[email-masked]>
Date: Wed, 07 Feb 2024 12:13:11 -0500

>>> There's one problem though.  The autoloaded keybindings for
>>> `expand-jump-to-next-slot' and `expand-jump-to-previous-slot' won't work
>>> the first time they're called on an expanded skeleton, unless the user
>>> has previously loaded expand.el.
>>
>> Hmm... this suggests we should try and "merge" `expand-list/pos` and
>> `skeleton-positions`?
>
> My thinking was just to initialize `expand-list/pos/index` in skeleton.el, so
> that a skeleton-command could populate `expand-pos` with locations from
> `skeleton-positions` even before expand.el has loaded.

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.

But the docstring of `expand-list` needs to be (re)written for that, first.

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.  IOW my reading of the code suggests the
code would work just as well with the patch below.

        Stefan

diff --git a/lisp/expand.el b/lisp/expand.el
index f32ab101224..714cc5fc11a 100644
--- a/lisp/expand.el
+++ b/lisp/expand.el
@@ -337,17 +337,12 @@ expand-abbrev-hook
 	    (progn
 	      ;; 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))
 	      (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)))
+		  (indent-region p expand-point nil))
 	      (run-hooks 'expand-expand-hook)
 	      t)
 	  nil))
@@ -359,12 +354,16 @@ expand-do-expansion
 	 (text (aref vect 0))
 	 (position (aref vect 1))
 	 (jump-args (aref vect 2))
-	 (hook (aref vect 3)))
+	 (hook (aref vect 3))
+         (startpos (point)))
     (cond (text
 	   (insert text)
 	   (setq expand-point (point))))
     (if jump-args
-        (funcall #'expand-build-list (car jump-args) (cdr jump-args)))
+        (setq expand-list (nreverse
+                           (mapcar (lambda (offset)
+                                     (+ startpos -1 offset))
+                                   (cdr jump-args)))))
     (if position
 	(backward-char position))
     (if hook
@@ -415,28 +414,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