Re: Browse skeleton positions

[Stefan Monnier <monnier@iro.umontreal.ca>]

>     (define-key kmap [ return ] 'skeleton-position-exit)

Please use [?\C-m] (or "\C-m"), unless you really mean that this binding
should not work in a tty.

> (defsubst turn-on-skeleton-position-mode ()

I recommend you use `defun' here, since it's unlikely that the advantages
of defsubst will outweigh its disadvantages in this case.

> (defvar skeleton-position-marker-list nil
>   "Global list of markers pointing to skeleton positions.")
> (make-variable-buffer-local 'skeleton-position-marker-list)

"Global" for a defvarred variable in elisp generally means "non-local
to any buffer".

> (defvar skeleton-position-push-mark nil
>   "\\<skeleton-position-mode-map>\
> When set, causes \\[skeleton-position-next] and \\[skeleton-position-prev] \
> in Skeleton Position mode to push a mark at point.")

Instead of using this state variable, you could maybe push the mark
directly when you enter skeleton-position-mode ?

> (defvar skeleton-position-current nil
>   "\\<skeleton-position-mode-map>\
> The last position visited by \\[skeleton-position-next] or \\[skeleton-position-prev] \
> in Skeleton Position mode, as a pointer into `skeleton-position-marker-list'.")

Maybe it would be better to just use (point) instead, and when you do
`next' or `prev', go through the list looking for the nearest marker in the
requested direction ?

> (unless (featurep 'skelposn)
>   (add-hook 'skeleton-end-hook
>             (lambda ()
>               (setq skeleton-position-marker-list
>                     (append
>                      (nreverse
>                       (mapcar
>                        (lambda (p) (set-marker (make-marker) p))
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                         'copy-marker

>                        skeleton-positions))
>                      skeleton-position-marker-list))
>               (when (and skeleton-position-marker-list
>                          (null skeleton-position-current))
>                 (setq skeleton-position-push-mark t))))

It's generally preferable to only add function symbols onto hooks, so it's
easier to remove and so there's less/no risk of adding it a second time.

As for the behavior, the above has the nasty side effect that just loading
"skelposn" into your Emacs will cause all your skeleton positions to
accumulate in the form of markers (at least until you finally call
skeleton-position-exit which will likely be never if you don't actually use
skelposn).  This can lead to a significant slowdown because of the way
markers are implemented.

Maybe it's better to process skeleton-positions only when requested,
i.e. in skeleton-position-mode (or maybe in skeleton-position-next
if you want to do it JIT-style).

How about a less in-yuor-face mode that lets you edit the text at the same
time as you navigate between the different markers ?


        Stefan