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

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

[Martin Marshall]

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

Dear Emacs Maintainers,

I noticed the following item in the Emacs TODO file:

> ** Improve the "code snippets" support
> Consolidate skeleton.el, tempo.el, and expand.el (any other?) and then
> advertise/use/improve it.

To that end, here's a patch which allows using expand.el's
`expand-jump-to-next-slot' ("C-x a n") and
`expand-jump-to-previous-slot' ("C-x a p") commands with all
skeletons.

In the current Emacs release, an expanded skeleton adds the locations
of `@' symbols to `skeleton-positions' list.  One could theoretically
convert these positions to markers and write commands for navigating
to the locations.  Fortunately, expand.el already implements this
behavior.  The only problem is that it's limited to skeletons being
expanded as abbrevs.  Skeletons invoked by a keybinding, menu entry,
or "M-x" can't use expand.el's jumping commands.

This patch changes that by updating `define-skeleton', so that
skeleton commands will update the list of markers in `expand-pos'
whenever called outside of `expand-abbrev'.

What do you think?

-- 
Best regards,
Martin Marshall

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Make jump commands usable for all skeletons --]
[-- Type: text/x-diff, Size: 4385 bytes --]

From f5dbc75b7d8c06ebed6ad0dae6c06b6957d7852b Mon Sep 17 00:00:00 2001
From: Martin Marshall <law@martinmarshall.com>
Date: Mon, 15 Jan 2024 15:09:01 -0500
Subject: [PATCH] Make jump commands usable for all skeletons

* etc/NEWS: Announce availability of jump commands for all skeletons
* lisp/expand.el (expand-in-progress-p, expand-abbrev-hook): Add global
variable to indicate when the 'expand-abbrev-hook' function is running.
(expand-skeleton-end-hook, skeleton-end-hook): Remove the
'expand-skeleton-end-hook' function and move its code to 'define-skeleton'.
* lisp/skeleton.el (expand): Require the expand.el library.
(define-skeleton): Populate 'expand-pos' for use by expand.el's jump commands,
or if 'expand-abbrev-hook' is runnning, copy the positions for handling by
that function.
---
 etc/NEWS         | 13 +++++++++++++
 lisp/expand.el   | 14 +++++---------
 lisp/skeleton.el | 11 ++++++++++-
 3 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index bce33f96aee..ef30d4e9219 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -147,6 +147,19 @@ compositing manager, Emacs will now redisplay such a frame even though
 'frame-visible-p' returns nil or 'icon' for it.  This can happen, for
 example, as part of preview for iconified frames.
 
+** Consolidation of tempo.el, skeleton.el, and expand.el
+
+---
+*** All skeleton templates can now use expand.el "jump" commands.
+Previously, skeleton templates invoked by "M-x", a keybinding, or a
+menu entry, could not use expand.el's jump commands
+('expand-jump-to-next-slot' and 'expand-jump-to-previous-slot').  A
+skeleton could only use expand.el navigation if added as an abbrev
+using 'expand-add-abbrev' and only when invoked using such an abbrev.
+
+The expand.el jump commands now work on all skeletons, regardless of
+invocation method.
+
 ---
 ** New user option 'menu-bar-close-window'.
 When non-nil, selecting "Close" from the "File" menu or clicking
diff --git a/lisp/expand.el b/lisp/expand.el
index f32ab101224..ff2ae226d5f 100644
--- a/lisp/expand.el
+++ b/lisp/expand.el
@@ -286,6 +286,9 @@ expand-add-abbrevs
 
 (defvar expand-list nil "Temporary variable used by the Expand package.")
 
+(defvar expand-in-progress-p nil
+  "If non-nil, `expand-abbrev-hook' is expanding an abbrev.")
+
 (defvar-local expand-pos nil
   "If non-nil, store a vector with position markers defined by the last expansion.")
 
@@ -326,6 +329,7 @@ expand-abbrev-hook
 See `expand-add-abbrevs'.  Value is non-nil if expansion was done."
   ;; Expand only at the end of a line if we are near a word that has
   ;; an abbrev built from expand-add-abbrev.
+  (setq expand-in-progress-p t)
   (if (and (eolp)
 	   (not (expand-in-literal)))
       (let ((p (point)))
@@ -348,6 +352,7 @@ expand-abbrev-hook
 		    (setq expand-index 0
 			  expand-pos (expand-list-to-markers expand-list)
 			  expand-list nil)))
+              (setq expand-in-progress-p nil)
 	      (run-hooks 'expand-expand-hook)
 	      t)
 	  nil))
@@ -473,15 +478,6 @@ expand-list-to-markers
 	    loop (1- loop)))
     v))
 
-;; integration with skeleton.el
-;; 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)))
-
-(add-hook 'skeleton-end-hook (function expand-skeleton-end-hook))
-
 (provide 'expand)
 
 (run-hooks 'expand-load-hook)
diff --git a/lisp/skeleton.el b/lisp/skeleton.el
index 89cb11b0fe2..24d6ef15e74 100644
--- a/lisp/skeleton.el
+++ b/lisp/skeleton.el
@@ -31,6 +31,8 @@
 
 ;;; Code:
 
+(require 'expand)
+
 (eval-when-compile (require 'cl-lib))
 
 ;; page 1:	statement skeleton language definition & interpreter
@@ -139,7 +141,14 @@ define-skeleton
 This is a way of overriding the use of a highlighted region.")
        (interactive "*P\nP")
        (atomic-change-group
-         (skeleton-proxy-new ',skeleton str arg)))))
+         (skeleton-proxy-new ',skeleton str arg))
+       (if expand-in-progress-p
+           ;; `expand-abbrev-hook' will set the markers in this case.
+           (setq expand-list skeleton-positions)
+         (setq expand-index 0
+	       expand-pos (expand-list-to-markers skeleton-positions)
+               expand-list nil))
+       t)))
 
 ;;;###autoload
 (defun skeleton-proxy-new (skeleton &optional str arg)
-- 
2.39.2

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

[Eli Zaretskii]

> From: Martin Marshall <law@martinmarshall.com>
> Date: Mon, 15 Jan 2024 15:45:23 -0500
> 
> Dear Emacs Maintainers,
> 
> I noticed the following item in the Emacs TODO file:
> 
> > ** Improve the "code snippets" support
> > Consolidate skeleton.el, tempo.el, and expand.el (any other?) and then
> > advertise/use/improve it.
> 
> To that end, here's a patch which allows using expand.el's
> `expand-jump-to-next-slot' ("C-x a n") and
> `expand-jump-to-previous-slot' ("C-x a p") commands with all
> skeletons.
> 
> In the current Emacs release, an expanded skeleton adds the locations
> of `@' symbols to `skeleton-positions' list.  One could theoretically
> convert these positions to markers and write commands for navigating
> to the locations.  Fortunately, expand.el already implements this
> behavior.  The only problem is that it's limited to skeletons being
> expanded as abbrevs.  Skeletons invoked by a keybinding, menu entry,
> or "M-x" can't use expand.el's jumping commands.
> 
> This patch changes that by updating `define-skeleton', so that
> skeleton commands will update the list of markers in `expand-pos'
> whenever called outside of `expand-abbrev'.
> 
> What do you think?

Martin,

Is this patch still relevant, or you intend to resolve this while
consolidating the related packages, perhaps based on yasnippet?

Thanks.

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

[Martin Marshall]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Martin Marshall <law@martinmarshall.com>
>> Date: Mon, 15 Jan 2024 15:45:23 -0500
>> 
>> Dear Emacs Maintainers,
>> 
>> I noticed the following item in the Emacs TODO file:
>> 
>> > ** Improve the "code snippets" support
>> > Consolidate skeleton.el, tempo.el, and expand.el (any other?) and then
>> > advertise/use/improve it.
>> 
>> To that end, here's a patch which allows using expand.el's
>> `expand-jump-to-next-slot' ("C-x a n") and
>> `expand-jump-to-previous-slot' ("C-x a p") commands with all
>> skeletons.
>> 
>> In the current Emacs release, an expanded skeleton adds the locations
>> of `@' symbols to `skeleton-positions' list.  One could theoretically
>> convert these positions to markers and write commands for navigating
>> to the locations.  Fortunately, expand.el already implements this
>> behavior.  The only problem is that it's limited to skeletons being
>> expanded as abbrevs.  Skeletons invoked by a keybinding, menu entry,
>> or "M-x" can't use expand.el's jumping commands.
>> 
>> This patch changes that by updating `define-skeleton', so that
>> skeleton commands will update the list of markers in `expand-pos'
>> whenever called outside of `expand-abbrev'.
>> 
>> What do you think?
>
> Martin,
>
> Is this patch still relevant, or you intend to resolve this while
> consolidating the related packages, perhaps based on yasnippet?

I think it's still relevant.

As I understand it, a goal for the snippet-engine project is to include
it in core and implement skeleton.el, expand.el, and tempo.el on top of
it.

When completed, that will make this patch irrelevant, but I don't know
how long that process will take.

-- 
Best regards,
Martin Marshall

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

[Eli Zaretskii]

> From: Martin Marshall <law@martinmarshall.com>
> Cc: 68487@debbugs.gnu.org
> Date: Sat, 27 Jan 2024 13:27:31 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Is this patch still relevant, or you intend to resolve this while
> > consolidating the related packages, perhaps based on yasnippet?
> 
> I think it's still relevant.
> 
> As I understand it, a goal for the snippet-engine project is to include
> it in core and implement skeleton.el, expand.el, and tempo.el on top of
> it.
> 
> When completed, that will make this patch irrelevant, but I don't know
> how long that process will take.

OK.  But after applying the patch on the master branch, I get this
while byte-compiling:

  In toplevel form:
  expand.el:91:2: Error: Eager macro-expansion failure: (error "Eager macro-expansion skipped due to cycle:
      => (load \"expand.el\") => (macroexpand-all (define-skeleton expand-c-for-skeleton  )) => (macroexpand (define-skeleton  )) => (load \"skeleton.el\") => (load \"expand.el\")")

  In toplevel form:
  skeleton.el:34:11: Error: Eager macro-expansion failure: (error "Eager macro-expansion skipped due to cycle:
      => (load \"expand.el\") => (macroexpand-all (define-skeleton expand-c-for-skeleton  )) => (macroexpand (define-skeleton  )) => (load \"skeleton.el\") => (load \"expand.el\")")
  Makefile:335: recipe for target `skeleton.elc' failed
  make[3]: *** [skeleton.elc] Error 1
  make[3]: *** Waiting for unfinished jobs....
  Makefile:335: recipe for target `expand.elc' failed

Could you please DTRT to avoid these errors?

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

[Martin Marshall]

Eli Zaretskii <eliz@gnu.org> writes:
> OK.  But after applying the patch on the master branch, I get this
> while byte-compiling:
>
>   In toplevel form:
>   expand.el:91:2: Error: Eager macro-expansion failure: (error "Eager macro-expansion skipped due to cycle:
>       => (load \"expand.el\") => (macroexpand-all (define-skeleton expand-c-for-skeleton  )) => (macroexpand (define-skeleton  )) => (load \"skeleton.el\") => (load \"expand.el\")")
>
>   In toplevel form:
>   skeleton.el:34:11: Error: Eager macro-expansion failure: (error "Eager macro-expansion skipped due to cycle:
>       => (load \"expand.el\") => (macroexpand-all (define-skeleton expand-c-for-skeleton  )) => (macroexpand (define-skeleton  )) => (load \"skeleton.el\") => (load \"expand.el\")")
>   Makefile:335: recipe for target `skeleton.elc' failed
>   make[3]: *** [skeleton.elc] Error 1
>   make[3]: *** Waiting for unfinished jobs....
>   Makefile:335: recipe for target `expand.elc' failed
>
> Could you please DTRT to avoid these errors?

Sorry, that was due to my incorrect assumption that byte-compiling with
"C-c C-f" would be equivalent to a full recompile.

I can fix it by deleting the sample skeleton (`expand-c-for-skeleton')
from expand.el.  But even though it's just a sample template, there
might be people using it.

Another option would be to join expand.el and skeleton.el into a single
file, perhaps calling it "expand-skeleton.el".

What do you think is best?

-- 
Best regards,
Martin Marshall

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

[Eli Zaretskii]

> From: Martin Marshall <law@martinmarshall.com>
> Cc: 68487@debbugs.gnu.org
> Date: Sat, 27 Jan 2024 16:48:52 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > OK.  But after applying the patch on the master branch, I get this
> > while byte-compiling:
> >
> >   In toplevel form:
> >   expand.el:91:2: Error: Eager macro-expansion failure: (error "Eager macro-expansion skipped due to cycle:
> >       => (load \"expand.el\") => (macroexpand-all (define-skeleton expand-c-for-skeleton  )) => (macroexpand (define-skeleton  )) => (load \"skeleton.el\") => (load \"expand.el\")")
> >
> >   In toplevel form:
> >   skeleton.el:34:11: Error: Eager macro-expansion failure: (error "Eager macro-expansion skipped due to cycle:
> >       => (load \"expand.el\") => (macroexpand-all (define-skeleton expand-c-for-skeleton  )) => (macroexpand (define-skeleton  )) => (load \"skeleton.el\") => (load \"expand.el\")")
> >   Makefile:335: recipe for target `skeleton.elc' failed
> >   make[3]: *** [skeleton.elc] Error 1
> >   make[3]: *** Waiting for unfinished jobs....
> >   Makefile:335: recipe for target `expand.elc' failed
> >
> > Could you please DTRT to avoid these errors?
> 
> Sorry, that was due to my incorrect assumption that byte-compiling with
> "C-c C-f" would be equivalent to a full recompile.
> 
> I can fix it by deleting the sample skeleton (`expand-c-for-skeleton')
> from expand.el.  But even though it's just a sample template, there
> might be people using it.
> 
> Another option would be to join expand.el and skeleton.el into a single
> file, perhaps calling it "expand-skeleton.el".
> 
> What do you think is best?

Neither alternative sounds attractive, TBH.

Stefan, what are our facilities to avoid mutual recursion like that?
Put the common stuff on a separate file, perhaps?

Thanks.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > OK.  But after applying the patch on the master branch, I get this
>> > while byte-compiling:
>> >
>> >   In toplevel form:
>> >   expand.el:91:2: Error: Eager macro-expansion failure: (error "Eager macro-expansion skipped due to cycle:
>> >       => (load \"expand.el\") => (macroexpand-all (define-skeleton expand-c-for-skeleton  )) => (macroexpand (define-skeleton  )) => (load \"skeleton.el\") => (load \"expand.el\")")

Why does loading `skeleton.el` cause a load of `expand.el`?
I can't reproduce it here and I can't see any mention of "expand" in
`skeleton.el` that would explain it.

> Stefan, what are our facilities to avoid mutual recursion like that?

It all depends on the specifics.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Martin Marshall <law@martinmarshall.com>,  68487@debbugs.gnu.org
> Date: Sun, 28 Jan 2024 13:47:34 -0500
> 
> >> > OK.  But after applying the patch on the master branch, I get this
> >> > while byte-compiling:
> >> >
> >> >   In toplevel form:
> >> >   expand.el:91:2: Error: Eager macro-expansion failure: (error "Eager macro-expansion skipped due to cycle:
> >> >       => (load \"expand.el\") => (macroexpand-all (define-skeleton expand-c-for-skeleton  )) => (macroexpand (define-skeleton  )) => (load \"skeleton.el\") => (load \"expand.el\")")
> 
> Why does loading `skeleton.el` cause a load of `expand.el`?
> I can't reproduce it here and I can't see any mention of "expand" in
> `skeleton.el` that would explain it.

You need to apply the patch in

   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=68487#5 

Sorry for not making it clear.

> > Stefan, what are our facilities to avoid mutual recursion like that?
> 
> It all depends on the specifics.

TIA for any advice.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> diff --git a/lisp/skeleton.el b/lisp/skeleton.el
> index 89cb11b0fe2..24d6ef15e74 100644
> --- a/lisp/skeleton.el
> +++ b/lisp/skeleton.el
> @@ -31,6 +31,8 @@
>  
>  ;;; Code:
>  
> +(require 'expand)
> +
>  (eval-when-compile (require 'cl-lib))
>  
>  ;; page 1:	statement skeleton language definition & interpreter
> @@ -139,7 +141,14 @@ define-skeleton
>  This is a way of overriding the use of a highlighted region.")
>         (interactive "*P\nP")
>         (atomic-change-group
> -         (skeleton-proxy-new ',skeleton str arg)))))
> +         (skeleton-proxy-new ',skeleton str arg))
> +       (if expand-in-progress-p
> +           ;; `expand-abbrev-hook' will set the markers in this case.
> +           (setq expand-list skeleton-positions)
> +         (setq expand-index 0
> +	       expand-pos (expand-list-to-markers skeleton-positions)
> +               expand-list nil))
> +       t)))
>  
>  ;;;###autoload
>  (defun skeleton-proxy-new (skeleton &optional str arg)

I don't think we want such a tight dependency between `skeleton.el` and
`expand.el` [ Partly to avoid the kind of circular dependencies you
just found yourself in, but also more generally.  ]

My suggestion would be to move that code to the `skeleton-end-hook`, but
I see that's where the code started, so I'm obviously missing something:
what make you decide to move the code out of the `skeleton-end-hook` and
into `define-skeleton`?


        Stefan

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

[Martin Marshall]

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

>> diff --git a/lisp/skeleton.el b/lisp/skeleton.el
>> index 89cb11b0fe2..24d6ef15e74 100644
>> --- a/lisp/skeleton.el
>> +++ b/lisp/skeleton.el
>> @@ -31,6 +31,8 @@
>>  
>>  ;;; Code:
>>  
>> +(require 'expand)
>> +
>>  (eval-when-compile (require 'cl-lib))
>>  
>>  ;; page 1:	statement skeleton language definition & interpreter
>> @@ -139,7 +141,14 @@ define-skeleton
>>  This is a way of overriding the use of a highlighted region.")
>>         (interactive "*P\nP")
>>         (atomic-change-group
>> -         (skeleton-proxy-new ',skeleton str arg)))))
>> +         (skeleton-proxy-new ',skeleton str arg))
>> +       (if expand-in-progress-p
>> +           ;; `expand-abbrev-hook' will set the markers in this case.
>> +           (setq expand-list skeleton-positions)
>> +         (setq expand-index 0
>> +	       expand-pos (expand-list-to-markers skeleton-positions)
>> +               expand-list nil))
>> +       t)))
>>  
>>  ;;;###autoload
>>  (defun skeleton-proxy-new (skeleton &optional str arg)
>
> I don't think we want such a tight dependency between `skeleton.el` and
> `expand.el` [ Partly to avoid the kind of circular dependencies you
> just found yourself in, but also more generally.  ]
>
> My suggestion would be to move that code to the `skeleton-end-hook`, but
> I see that's where the code started, so I'm obviously missing something:
> what make you decide to move the code out of the `skeleton-end-hook` and
> into `define-skeleton`?
>
>
>         Stefan

Sorry for my late response.

I had moved that code to the skeleton command definition because I
thought I'd have to account for adding skeleton positions to
`expand-pos' before expand.el had loaded.  But since it turns out that
expand.el loads skeleton.el anyway, it seems to makes more sense to
follow your suggestion and put this change back on `skeleton-end-hook'.

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.

-- 
Best regards,
Martin Marshall

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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`?


        Stefan

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

[Martin Marshall]

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

>> 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 think `skeleton-positions` was intended as a building block for users
(or package authors) to create jumping capability of their own.  For
example, an emacswiki article[1] proposes one way of doing this.  I'd
want to avoid renaming `skeleton-positions` or changing the value it
receives, since that would probably break such configurations.

[1] https://www.emacswiki.org/emacs/SkeletonMode#h5o-15

-- 
Best regards,
Martin Marshall

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> 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

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

[Martin Marshall]

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))
 

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> 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.

It's because it internally did that, tho I don't know why it did that
internally since my patch seems to show that it's simpler not to.

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

Should I install it, so it's kept separate from the changes you add
on top (mostly for readability of the patches)?

> What do you think?

I find the patch a bit hard to read, maybe for lack of a separate
description of the intended changes, or maybe because it does too much
in a single step.

I have one question, tho:

>  (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))

Hmm... but this `expand-do-expansion` doesn't actually "do" any
expansion any more, right?

>  (defun expand-skeleton-end-hook ()
> -  (if skeleton-positions
> -      (setq expand-list skeleton-positions)))
> +  (when skeleton-positions
> +    (setq expand-list skeleton-positions)
> +    (expand-do-expansion)))

Here if you read the code out loud it doesn't make sense to call 
`expand-do-expansion` since skeleton has already "done the expansion".


        Stefan

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

[Eli Zaretskii]

Ping!  Martin, can you please respond to Stefan's comments, so we
could move forward with this issue?

> Cc: 68487@debbugs.gnu.org
> Date: Sat, 02 Mar 2024 23:07:18 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> >> 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.
> 
> It's because it internally did that, tho I don't know why it did that
> internally since my patch seems to show that it's simpler not to.
> 
> > Changing `expand-abbrev-hook` to expect `expand-list` to actually be a
> > list (as you did with your patch) makes sense to me.
> 
> Should I install it, so it's kept separate from the changes you add
> on top (mostly for readability of the patches)?
> 
> > What do you think?
> 
> I find the patch a bit hard to read, maybe for lack of a separate
> description of the intended changes, or maybe because it does too much
> in a single step.
> 
> I have one question, tho:
> 
> >  (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))
> 
> Hmm... but this `expand-do-expansion` doesn't actually "do" any
> expansion any more, right?
> 
> >  (defun expand-skeleton-end-hook ()
> > -  (if skeleton-positions
> > -      (setq expand-list skeleton-positions)))
> > +  (when skeleton-positions
> > +    (setq expand-list skeleton-positions)
> > +    (expand-do-expansion)))
> 
> Here if you read the code out loud it doesn't make sense to call 
> `expand-do-expansion` since skeleton has already "done the expansion".
> 
> 
>         Stefan
> 
> 
> 
> 
> 

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

[martin]

Eli Zaretskii <eliz@gnu.org> writes:

> Ping!  Martin, can you please respond to Stefan's comments, so we
> could move forward with this issue?

Sorry for the long delay in responding.  I'll try to get to this by the
end of the weekend.


>> Cc: 68487@debbugs.gnu.org
>> Date: Sat, 02 Mar 2024 23:07:18 -0500
>> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> >> 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.
>> 
>> It's because it internally did that, tho I don't know why it did that
>> internally since my patch seems to show that it's simpler not to.
>> 
>> > Changing `expand-abbrev-hook` to expect `expand-list` to actually be a
>> > list (as you did with your patch) makes sense to me.
>> 
>> Should I install it, so it's kept separate from the changes you add
>> on top (mostly for readability of the patches)?
>> 
>> > What do you think?
>> 
>> I find the patch a bit hard to read, maybe for lack of a separate
>> description of the intended changes, or maybe because it does too much
>> in a single step.
>> 
>> I have one question, tho:
>> 
>> >  (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))
>> 
>> Hmm... but this `expand-do-expansion` doesn't actually "do" any
>> expansion any more, right?
>> 
>> >  (defun expand-skeleton-end-hook ()
>> > -  (if skeleton-positions
>> > -      (setq expand-list skeleton-positions)))
>> > +  (when skeleton-positions
>> > +    (setq expand-list skeleton-positions)
>> > +    (expand-do-expansion)))
>> 
>> Here if you read the code out loud it doesn't make sense to call 
>> `expand-do-expansion` since skeleton has already "done the expansion".
>> 
>> 
>>         Stefan
>> 
>> 
>> 
>> 
>> 

-- 
Best regards,
Martin Marshall

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

[Eli Zaretskii]

Any progress there?

> From: martin <law@martinmarshall.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  68487@debbugs.gnu.org
> Date: Thu, 21 Mar 2024 20:05:04 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Ping!  Martin, can you please respond to Stefan's comments, so we
> > could move forward with this issue?
> 
> Sorry for the long delay in responding.  I'll try to get to this by the
> end of the weekend.
> 
> 
> >> Cc: 68487@debbugs.gnu.org
> >> Date: Sat, 02 Mar 2024 23:07:18 -0500
> >> From:  Stefan Monnier via "Bug reports for GNU Emacs,
> >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >> 
> >> >> 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.
> >> 
> >> It's because it internally did that, tho I don't know why it did that
> >> internally since my patch seems to show that it's simpler not to.
> >> 
> >> > Changing `expand-abbrev-hook` to expect `expand-list` to actually be a
> >> > list (as you did with your patch) makes sense to me.
> >> 
> >> Should I install it, so it's kept separate from the changes you add
> >> on top (mostly for readability of the patches)?
> >> 
> >> > What do you think?
> >> 
> >> I find the patch a bit hard to read, maybe for lack of a separate
> >> description of the intended changes, or maybe because it does too much
> >> in a single step.
> >> 
> >> I have one question, tho:
> >> 
> >> >  (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))
> >> 
> >> Hmm... but this `expand-do-expansion` doesn't actually "do" any
> >> expansion any more, right?
> >> 
> >> >  (defun expand-skeleton-end-hook ()
> >> > -  (if skeleton-positions
> >> > -      (setq expand-list skeleton-positions)))
> >> > +  (when skeleton-positions
> >> > +    (setq expand-list skeleton-positions)
> >> > +    (expand-do-expansion)))
> >> 
> >> Here if you read the code out loud it doesn't make sense to call 
> >> `expand-do-expansion` since skeleton has already "done the expansion".
> >> 
> >> 
> >>         Stefan
> >> 
> >> 
> >> 
> >> 
> >> 
> 
> -- 
> Best regards,
> Martin Marshall
> 

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

[Eli Zaretskii]

Ping!  Martin, did you have time to make any progress in this matter?

> Cc: 68487@debbugs.gnu.org, monnier@iro.umontreal.ca
> Date: Sat, 06 Apr 2024 11:56:53 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Any progress there?
> 
> > From: martin <law@martinmarshall.com>
> > Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  68487@debbugs.gnu.org
> > Date: Thu, 21 Mar 2024 20:05:04 -0400
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > > Ping!  Martin, can you please respond to Stefan's comments, so we
> > > could move forward with this issue?
> > 
> > Sorry for the long delay in responding.  I'll try to get to this by the
> > end of the weekend.
> > 
> > 
> > >> Cc: 68487@debbugs.gnu.org
> > >> Date: Sat, 02 Mar 2024 23:07:18 -0500
> > >> From:  Stefan Monnier via "Bug reports for GNU Emacs,
> > >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > >> 
> > >> >> 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.
> > >> 
> > >> It's because it internally did that, tho I don't know why it did that
> > >> internally since my patch seems to show that it's simpler not to.
> > >> 
> > >> > Changing `expand-abbrev-hook` to expect `expand-list` to actually be a
> > >> > list (as you did with your patch) makes sense to me.
> > >> 
> > >> Should I install it, so it's kept separate from the changes you add
> > >> on top (mostly for readability of the patches)?
> > >> 
> > >> > What do you think?
> > >> 
> > >> I find the patch a bit hard to read, maybe for lack of a separate
> > >> description of the intended changes, or maybe because it does too much
> > >> in a single step.
> > >> 
> > >> I have one question, tho:
> > >> 
> > >> >  (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))
> > >> 
> > >> Hmm... but this `expand-do-expansion` doesn't actually "do" any
> > >> expansion any more, right?
> > >> 
> > >> >  (defun expand-skeleton-end-hook ()
> > >> > -  (if skeleton-positions
> > >> > -      (setq expand-list skeleton-positions)))
> > >> > +  (when skeleton-positions
> > >> > +    (setq expand-list skeleton-positions)
> > >> > +    (expand-do-expansion)))
> > >> 
> > >> Here if you read the code out loud it doesn't make sense to call 
> > >> `expand-do-expansion` since skeleton has already "done the expansion".
> > >> 
> > >> 
> > >>         Stefan
> > >> 
> > >> 
> > >> 
> > >> 
> > >> 
> > 
> > -- 
> > Best regards,
> > Martin Marshall
> > 
> 
> 
> 
> 

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

[Eli Zaretskii]

Ping! Ping!  Martin, could you please respond?

> Cc: 68487@debbugs.gnu.org, monnier@iro.umontreal.ca
> Date: Thu, 18 Apr 2024 11:58:40 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Ping!  Martin, did you have time to make any progress in this matter?
> 
> > Cc: 68487@debbugs.gnu.org, monnier@iro.umontreal.ca
> > Date: Sat, 06 Apr 2024 11:56:53 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > 
> > Any progress there?
> > 
> > > From: martin <law@martinmarshall.com>
> > > Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  68487@debbugs.gnu.org
> > > Date: Thu, 21 Mar 2024 20:05:04 -0400
> > > 
> > > Eli Zaretskii <eliz@gnu.org> writes:
> > > 
> > > > Ping!  Martin, can you please respond to Stefan's comments, so we
> > > > could move forward with this issue?
> > > 
> > > Sorry for the long delay in responding.  I'll try to get to this by the
> > > end of the weekend.
> > > 
> > > 
> > > >> Cc: 68487@debbugs.gnu.org
> > > >> Date: Sat, 02 Mar 2024 23:07:18 -0500
> > > >> From:  Stefan Monnier via "Bug reports for GNU Emacs,
> > > >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > > >> 
> > > >> >> 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.
> > > >> 
> > > >> It's because it internally did that, tho I don't know why it did that
> > > >> internally since my patch seems to show that it's simpler not to.
> > > >> 
> > > >> > Changing `expand-abbrev-hook` to expect `expand-list` to actually be a
> > > >> > list (as you did with your patch) makes sense to me.
> > > >> 
> > > >> Should I install it, so it's kept separate from the changes you add
> > > >> on top (mostly for readability of the patches)?
> > > >> 
> > > >> > What do you think?
> > > >> 
> > > >> I find the patch a bit hard to read, maybe for lack of a separate
> > > >> description of the intended changes, or maybe because it does too much
> > > >> in a single step.
> > > >> 
> > > >> I have one question, tho:
> > > >> 
> > > >> >  (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))
> > > >> 
> > > >> Hmm... but this `expand-do-expansion` doesn't actually "do" any
> > > >> expansion any more, right?
> > > >> 
> > > >> >  (defun expand-skeleton-end-hook ()
> > > >> > -  (if skeleton-positions
> > > >> > -      (setq expand-list skeleton-positions)))
> > > >> > +  (when skeleton-positions
> > > >> > +    (setq expand-list skeleton-positions)
> > > >> > +    (expand-do-expansion)))
> > > >> 
> > > >> Here if you read the code out loud it doesn't make sense to call 
> > > >> `expand-do-expansion` since skeleton has already "done the expansion".
> > > >> 
> > > >> 
> > > >>         Stefan
> > > >> 
> > > >> 
> > > >> 
> > > >> 
> > > >> 
> > > 
> > > -- 
> > > Best regards,
> > > Martin Marshall
> > > 
> > 
> > 
> > 
> > 
> 
> 
> 
>