Re: [ELPA] New package: cursor-undo

[Philip Kaludercic <philipk@posteo.net>]

[-- Attachment #1: Type: text/plain, Size: 1958 bytes --]

路客 <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:


[-- Attachment #2: Type: text/plain, Size: 4207 bytes --]

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
+

[-- Attachment #3: Type: text/plain, Size: 3725 bytes --]


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