Re: [ELPA] New package: cursor-undo

[路客 <luke.yx.lee@gmail.com>]

>  Before pushing a package directly to elpa.git, you should try to build
>  it locally.  Currently it fails because the :doc specification links to
>  a .txt file, which cannot be converted to a Info manual:

I followed the ELPA steps but there is no info about how to build a new
package locally before pushing into the main git branch external/<package>
. A "make build/cursor-undo" command sometimes cause a pull from external
git.  Even in the ELPA `elpa-admin.el' the first TODO is:

  ;; - bug#45345: [elpa-archive] "make build/<package>" should not pull
  ;;   unconditionally

and I found this is /sometimes/ true.  Sometimes I modified my source code
and do "make build/cursor-undo" again it will be overwritten back to the
old one in git external/cursor-undo.  Therefore, the only way is to push
it back into git and wait a while then rebuild.  However, sometimes it
won't so it's confusing what the correct steps to perform a pure local
build really are.  So, let me know if there is a way to build a new
package locally without push into the external/<package> first.

But as a hindsight I now learned I could first modify "elpa-packages"
locally and build but still need to push into external/<package> first.
Next time I will make sure I do that.

After I managed to build my package I found the ELPA README did not mention
that the ";;; <package> ends here" is required.

Some of your comments are followed while some not:

> +;; shouldn't this be a major mode?
> (defvar cundo-enable-cursor-tracking t) ; global enable flag

Actually this package is not a mode, but it seems it's preferred as Stefan
also mentioned this to me so I now made it a minor mode.

>                   ;; unless I am misreading this, `prev-screen-start'
>                   ;; is being evaluated in the context of the macro,
>                   ;; /not/ the macro-expansion.  Yet
>                   ;; `prev-screen-start' is bound and set inside the
>                   ;; context of the macro-expansion.  This seems to
>                   ;; trigger the 'Unused lexical variable
>                   ;; ‘prev-screen-start’' byte-compiler warning
>                   ;; below.

Yes it was meant to be evaluated instead of being expanded since the
beginning many years ago, this part of code has been tested for many
years, I reviewed the generated code again and it's still valid.  I think
the byte compiler must have mis-interpreted my intention and gave the
incorrect warnings.  Anyway, before the root cause is found a quick
mitigation is to (defvar prev-screen-start).

>  +(provide 'cursor-undo)

This was done earlier in the code.

> +;;; cursor-undo.el ends here

This step is a must but the ELPA README did not mention that.  I found
the local build would fail without it, so it's now added.

The fixed version has been pushed into the git.  Thanks.

Best regards,
Luke Lee


On Sun, Jul 21, 2024 at 6:57 PM Philip Kaludercic <philipk@posteo.net> wrote:
>
> 路客 <luke.yx.lee@gmail.com> writes:
>
> > Hi all,
> > I've just pushed my new package into ELPA.
>
> Before pushing a package directly to elpa.git, you should try to build
> it locally.  Currently it fails because the :doc specification links to
> a .txt file, which cannot be converted to a Info manual:
>
> --8<---------------cut here---------------start------------->8---
> $ make build/cursor-undo
> emacs --batch -Q -l admin/elpa-admin.el \
>          -f elpaa-batch-pkg-spec-make-dependencies .pkg-descs.mk
> Source file ‘/home/phi/Source/emacs/lisp/emacs-lisp/pcase.el’ newer than byte-compiled file; using older file
> emacs --batch -l /home/phi/Source/elpa/admin/elpa-admin.el      \
>          -f elpaa-batch-make-one-package cursor-undo
> Source file ‘/home/phi/Source/emacs/lisp/emacs-lisp/pcase.el’ newer than byte-compiled file; using older file
> ======== Building tarball archive-devel/cursor-undo-1.0.0.20240720.150723.tar...
> Build error for archive-devel/cursor-undo-1.0.0.20240720.150723.tar: (error "Not a supported doc format: README.txt")
> ######## Build of package archive-devel/cursor-undo-1.0.0.20240720.150723.tar FAILED!!
> ======== Building tarball archive/cursor-undo-1.0.tar...
> Warning (package): Package lacks a terminating comment
> Build error for archive/cursor-undo-1.0.tar: (error "Not a supported doc format: README.txt")
> ######## Build of package archive/cursor-undo-1.0.tar FAILED!!
> --8<---------------cut here---------------end--------------->8---
>
> I can fix that myself by replacing the :doc with a :readme -- though
> since your commentary section has the same contents as the README file,
> we could also extract the documentation from there, without having to
> duplicate it.
>
> It would also be nice to announce a package and wait for some comments,
> before pushing it to elpa.git (though I'll admit that I have done the
> same in the past as well).  I therefore have a few comments that I think
> you should consider:
>
> diff --git a/cursor-undo.el b/cursor-undo.el
> index 3af2dc8d23..717ad10bc0 100644
> --- a/cursor-undo.el
> +++ b/cursor-undo.el
> @@ -83,7 +83,7 @@
>  ;;   find the advised function not working properly, consider comment out
>  ;;   the following source code `(define-advice viper-undo ...' to restore
>  ;;   the original `viper-undo' function and use Emacs default `undo'
> -;;   (C-_, C-/ or C-x u ...) to undo cursor movements.
> +;;   (\\[undo]) to undo cursor movements.
>  ;;
>  ;; Future TODO thoughts: a more desired implementation is to integrate it
>  ;; into Emacs source by extending the `interactive' spec to add code
> @@ -93,11 +93,11 @@
>
>  ;;; Code:
>
> +;; shouldn't this be a major mode?
>  (defvar cundo-enable-cursor-tracking t) ; global enable flag
>  ;; Local disable flag, use reverse logic as NIL is still a list and we can
>  ;; pop it again and again
> -(defvar cundo-disable-local-cursor-tracking nil)
> -(make-variable-buffer-local 'cundo-disable-local-cursor-tracking)
> +(defvar-local cundo-disable-local-cursor-tracking nil)
>
>  ;; Clear duplicated `(apply cdr nil) nil' pairs in `buffer-undo-list' done by
>  ;; `primitive-undo'.
> @@ -108,9 +108,9 @@
>    (if cundo-clear-usless-undoinfo
>        ;; This tries to remove as much garbage as possible but still some left
>        ;; in the `buffer-undo-list'.  TODO: add a idle task to remove them
> -      (while (and (null (first buffer-undo-list)) ;; nil
> -                  (equal (second buffer-undo-list) '(apply cdr nil))
> -                  (null (third buffer-undo-list)))
> +      (while (and (null (nth 0 buffer-undo-list)) ;; nil
> +                  (equal (nth 1 buffer-undo-list) '(apply cdr nil))
> +                  (null (nth 2 buffer-undo-list)))
>          ;; (nil (apply cdr nil) nil a b...) -> (nil a b...)
>          (setq buffer-undo-list (cddr buffer-undo-list)))))
>
> @@ -143,8 +143,8 @@ Parameters:
>    (if (and (not screen-pos)
>             no-move)
>        (error
> -"Error: No undo information will be stored if we're neither recording cursor
> -relative screen position (screen-pos=NIL) nor `point' position (no-move=t)."))
> +"Error: No undo information will be stored if we're neither recording cursor \
> +relative screen position (screen-pos=NIL) nor `point' position (no-move=t)"))
>    (let* ((func-sym-str (symbol-name func-sym))
>           (advice-sym-str (concat func-sym-str "-cursor-undo"))
>           (advice-sym (make-symbol advice-sym-str))
> @@ -161,7 +161,7 @@ relative screen position (screen-pos=NIL) nor `point' position (no-move=t)."))
>                  ;; prevent nested calls for complicated compound commands
>                  (cundo-enable-cursor-tracking nil)
>                  (prev-point (point))
> -                prev-screen-start)
> +                (prev-screen-start))
>             ,@(when screen-pos
>                 '((when cursor-tracking
>                     (setq prev-screen-start (window-start)))))
> @@ -206,6 +206,14 @@ relative screen position (screen-pos=NIL) nor `point' position (no-move=t)."))
>                          (numberp (cadr buffer-undo-list))
>                          (= prev-point (cadr buffer-undo-list))))
>               ,@(if screen-pos
> +                   ;; unless I am misreading this, `prev-screen-start'
> +                   ;; is being evaluated in the context of the macro,
> +                   ;; /not/ the macro-expansion.  Yet
> +                   ;; `prev-screen-start' is bound and set inside the
> +                   ;; context of the macro-expansion.  This seems to
> +                   ;; trigger the 'Unused lexical variable
> +                   ;; ‘prev-screen-start’' byte-compiler warning
> +                   ;; below.
>                    '((push `(apply cundo-restore-win (,@prev-screen-start))
>                            buffer-undo-list)))
>               ,@(unless no-move
> @@ -598,3 +606,7 @@ relative screen position (screen-pos=NIL) nor `point' position (no-move=t)."))
>       (def-cursor-undo viper-search-forward                   t       t)
>       (def-cursor-undo viper-beginning-of-line)
>       (def-cursor-undo viper-repeat-find                      t       t)))
> +
> +(provide 'cursor-undo)
> +;;; cursor-undo.el ends here
> +
>
> A general comment also regarding the `eval-after-load' blocks.  If I
> were you, I'll pull all this extra support into additional files that
> require cursor-undo and the packages you wish to support.  And instead
> of the `eval-after-load' blocks, you could just scan the current
> directory for all "cursor-undo-[...].el" files and call
> `eval-after-load', requiring "cursor-undo-[...]".  That should clear a
> number of the byte-compiler warnings.  It would also be nice if you
> could tackle the checkdoc warnings as well.
>
> > Cursor-undo allows you to undo cursor movement commands using the Emacs
> > standard `undo' command.
> >
> > For frequent cursor movements such as up/down/left/right, it
> > combines the movements of the same direction into a single undo entry.
> > This prevents the undo command from reversing each individual
> > character movement separately.  For example, if you move the cursor 20
> > characters to the right and then 10 lines up, the first undo will go
> > down 10 lines back, and the next undo move back 20 characters left.
> > On the other hand, for search commands that often jump across multiple
> > pages, each search command has its own undo entry, allowing you to
> > undo them one at a time rather than as a combined operation.
> >
> > This cursor-undo functionality has existed in my local Emacs init file
> > for over 11+ years, since version 0 on 2013-06-26.  It was originally
> > intended to support my Brief Editor Mode only, but I later found it
> > would be more useful if implemented in a more generalized way.  For
> > years I have hoped for an official implementation of this feature,
> > which is commonly seen among various editors.  Considering my
> > implementation using advice functions a bit inelegant so I have always
> > hesitated to release it till recently.
> >
> > Until there is official support for the cursor undo feature, this
> > package serves most common daily needs.  The core design is to align
> > with Emacs's native `undo' function by recording cursor positions
> > and screen-relative position undo entries in the `buffer-undo-list'
> > in accordance with its documentation.
> >
> > As this package primarily consists of advice functions to wrap cursor
> > movement commands, each cursor movement command needs to be manually
> > wrapped with `def-cursor-undo'.  For interactive functions that
> > heavily invoke advised cursor movement commands, you may even need to
> > advise them with `disable-cursor-tracking' to prevent generating
> > numerous distinct cursor undo entries from a single command.  For user
> > convenience, we have prepared ready `def-cursor-undo' advice sets for
> > standard Emacs cursor movement commands, Brief Editor mode, Viper
> > mode, and EVIL mode.
> >
> > Usage:
> >
> >   In theory, once this package is installed, you should already have
> >   cursor-undo autoloaded and enabled.  If not, or if you downloaded this
> >   package as source, put "cursor-undo.el" file in a `load-path' and add
> >   the following line into your Emacs init file .emacs or init.el:
> >
> >     (require 'cursor-undo)
> >
> > Notes for EVIL mode user:
> >
> >   If you choose to use default Emacs `undo' system, you should be able
> >   to use `evil-undo' to undo cursor movements.  If your choice is
> >   tree-undo or another undo system, you might need to use Emacs default
> >   `undo' (C-_, C-/ or C-x u ...) to undo cursor movements.
> >
> > Notes for Viper mode user:
> >
> >   The default `viper-undo' is advised to allow cursor-undo.  If you
> >   find the advised function not working properly, consider comment out
> >   the following source code `(define-advice viper-undo ...' to restore
> >   the original `viper-undo' function and use Emacs default `undo'
> >   (C-_, C-/ or C-x u ...) to undo cursor movements.
>
> --
>         Philip Kaludercic on peregrine



--
Best regards,
Luke Lee