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

[Eli Zaretskii <eliz@gnu.org>]

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