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

Previous Next

Full log

View this message in rfc822 format

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

Previous Next

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