GNU bug report logs - #68487
[PATCH] Make jump commands usable for all skeletons

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 68487 in the body.
You can then email your comments to 68487 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Martin Marshall <law <at> martinmarshall.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Make jump commands usable for all skeletons
Date: Mon, 15 Jan 2024 15:45:23 -0500

[Message part 1 (text/plain, inline)]

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

[0001-Make-jump-commands-usable-for-all-skeletons.patch (text/x-diff, attachment)]

Message #8 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Martin Marshall <law <at> martinmarshall.com>
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Sat, 27 Jan 2024 11:13:00 +0200

> From: Martin Marshall <law <at> 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.

Message #11 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Martin Marshall <law <at> martinmarshall.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Sat, 27 Jan 2024 13:27:31 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Martin Marshall <law <at> 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

Message #14 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Martin Marshall <law <at> martinmarshall.com>
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Sat, 27 Jan 2024 20:51:09 +0200

> From: Martin Marshall <law <at> martinmarshall.com>
> Cc: 68487 <at> debbugs.gnu.org
> Date: Sat, 27 Jan 2024 13:27:31 -0500
> 
> Eli Zaretskii <eliz <at> 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?

Message #17 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Martin Marshall <law <at> martinmarshall.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Sat, 27 Jan 2024 16:48:52 -0500

Eli Zaretskii <eliz <at> 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

Message #20 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Martin Marshall <law <at> martinmarshall.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca> 
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Sun, 28 Jan 2024 07:52:48 +0200

> From: Martin Marshall <law <at> martinmarshall.com>
> Cc: 68487 <at> debbugs.gnu.org
> Date: Sat, 27 Jan 2024 16:48:52 -0500
> 
> Eli Zaretskii <eliz <at> 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.

Message #23 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 68487 <at> debbugs.gnu.org, Martin Marshall <law <at> martinmarshall.com>
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
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.

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

It all depends on the specifics.


        Stefan

Message #26 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 68487 <at> debbugs.gnu.org, law <at> martinmarshall.com
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Sun, 28 Jan 2024 21:24:55 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Martin Marshall <law <at> martinmarshall.com>,  68487 <at> 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.

Message #29 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Martin Marshall <law <at> martinmarshall.com>
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Sun, 28 Jan 2024 14:45:37 -0500

> 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

Message #34 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Martin Marshall <law <at> martinmarshall.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Mon, 05 Feb 2024 16:46:55 -0500

Stefan Monnier <monnier <at> 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

Message #37 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Martin Marshall <law <at> martinmarshall.com>
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Mon, 05 Feb 2024 21:46:19 -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`?


        Stefan

Message #40 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Martin Marshall <law <at> martinmarshall.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Tue, 06 Feb 2024 17:11:59 -0500

Stefan Monnier <monnier <at> 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

Message #43 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Martin Marshall <law <at> martinmarshall.com>
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
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

Message #46 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Martin Marshall <law <at> martinmarshall.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Sun, 25 Feb 2024 20:26:22 -0500

Finally got around to looking at this again.  

Stefan Monnier <monnier <at> 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))
 

Message #49 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Martin Marshall <law <at> martinmarshall.com>
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Sat, 02 Mar 2024 23:07:18 -0500

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

Message #52 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: law <at> martinmarshall.com, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 68487 <at> debbugs.gnu.org
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Thu, 14 Mar 2024 09:50:59 +0200

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

> Cc: 68487 <at> 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 <at> 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
> 
> 
> 
> 
> 

Message #55 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: martin <law <at> martinmarshall.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 68487 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Thu, 21 Mar 2024 20:05:04 -0400

Eli Zaretskii <eliz <at> 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 <at> 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 <at> 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

Message #58 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: martin <law <at> martinmarshall.com>
Cc: 68487 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Sat, 06 Apr 2024 11:56:53 +0300

Any progress there?

> From: martin <law <at> martinmarshall.com>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>,  68487 <at> debbugs.gnu.org
> Date: Thu, 21 Mar 2024 20:05:04 -0400
> 
> Eli Zaretskii <eliz <at> 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 <at> 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 <at> 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
> 

Message #61 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: law <at> martinmarshall.com
Cc: 68487 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Thu, 18 Apr 2024 11:58:40 +0300

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

> Cc: 68487 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> Date: Sat, 06 Apr 2024 11:56:53 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> Any progress there?
> 
> > From: martin <law <at> martinmarshall.com>
> > Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>,  68487 <at> debbugs.gnu.org
> > Date: Thu, 21 Mar 2024 20:05:04 -0400
> > 
> > Eli Zaretskii <eliz <at> 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 <at> 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 <at> 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
> > 
> 
> 
> 
> 

Message #64 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: law <at> martinmarshall.com
Cc: 68487 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Thu, 02 May 2024 11:37:15 +0300

Ping! Ping!  Martin, could you please respond?

> Cc: 68487 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> Date: Thu, 18 Apr 2024 11:58:40 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> Ping!  Martin, did you have time to make any progress in this matter?
> 
> > Cc: 68487 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> > Date: Sat, 06 Apr 2024 11:56:53 +0300
> > From: Eli Zaretskii <eliz <at> gnu.org>
> > 
> > Any progress there?
> > 
> > > From: martin <law <at> martinmarshall.com>
> > > Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>,  68487 <at> debbugs.gnu.org
> > > Date: Thu, 21 Mar 2024 20:05:04 -0400
> > > 
> > > Eli Zaretskii <eliz <at> 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 <at> 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 <at> 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
> > > 
> > 
> > 
> > 
> > 
> 
> 
> 
> 

Message #67 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: law <at> martinmarshall.com
Cc: 68487 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Sat, 18 May 2024 11:28:00 +0300

Ping! Ping! Ping!  Martin, are you still there?

> Cc: 68487 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> Date: Thu, 02 May 2024 11:37:15 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> Ping! Ping!  Martin, could you please respond?
> 
> > Cc: 68487 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> > Date: Thu, 18 Apr 2024 11:58:40 +0300
> > From: Eli Zaretskii <eliz <at> gnu.org>
> > 
> > Ping!  Martin, did you have time to make any progress in this matter?
> > 
> > > Cc: 68487 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> > > Date: Sat, 06 Apr 2024 11:56:53 +0300
> > > From: Eli Zaretskii <eliz <at> gnu.org>
> > > 
> > > Any progress there?
> > > 
> > > > From: martin <law <at> martinmarshall.com>
> > > > Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>,  68487 <at> debbugs.gnu.org
> > > > Date: Thu, 21 Mar 2024 20:05:04 -0400
> > > > 
> > > > Eli Zaretskii <eliz <at> 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 <at> 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 <at> 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
> > > > 
> > > 
> > > 
> > > 
> > > 
> > 
> > 
> > 
> > 
> 
> 
> 
> 

Message #70 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: martin <law <at> martinmarshall.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68487 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Wed, 12 Feb 2025 23:03:22 -0800

martin <law <at> martinmarshall.com> writes:

> Eli Zaretskii <eliz <at> 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.

Did you find the time to look into this?  Thanks.

Message #73 received at 68487 <at> debbugs.gnu.org (full text, mbox):

From: Martin Marshall <law <at> martinmarshall.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68487 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Thu, 13 Feb 2025 06:28:48 -0500

[Message part 1 (text/plain, inline)]

> Did you find the time to look into this?  Thanks.

No, sorry for leaving this bug open.  It can be closed as far as I'm
concerned.

I ended up moving away from skeletons and haven't looked at the issue in a
long time.

[Message part 2 (text/html, inline)]

Message #78 received at 68487-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Martin Marshall <law <at> martinmarshall.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68487-done <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#68487: [PATCH] Make jump commands usable for all skeletons
Date: Thu, 13 Feb 2025 05:50:34 -0600

Martin Marshall <law <at> martinmarshall.com> writes:

>> Did you find the time to look into this?  Thanks.
>
> No, sorry for leaving this bug open.  It can be closed as far as I'm
> concerned.
>
> I ended up moving away from skeletons and haven't looked at the issue in a
> long time.

OK, thanks.  I'm therefore closing this bug report.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.