Re: [NonGNU ELPA] 11 new packages!

[Philip Kaludercic <philipk@posteo.net>]

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

Akib Azmain Turja <akib@disroot.org> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
> [...]
>
>> I just realised that none of the patches can be applies, because the
>> last commit in nongnu.git changed the package names from strings to
>> symbols.  This makes applying the patches a nuisance.  Could you perhaps
>> update the patches to use symbol for names, and make sure nothing has
>> changed?
>>
>
> Hmm, I think it's a good decision to use symbols.
>
> It's already midnight here, so I'll update the patches tomorrow.  

Sure, no problem.  Thank you for updating the patches at all.

>                                                                   BTW,
> can you please you review my Eat package[1] (terminal emulator, you have
> probably noticed it)?  I'm going to submit that too with the next batch
> of patches.

Looks interesting!  Here are my comments:


[-- Attachment #2: comments.diff --]
[-- Type: text/x-diff, Size: 24000 bytes --]

diff --git a/eat.el b/eat.el
index aa048a4..99b681e 100644
--- a/eat.el
+++ b/eat.el
@@ -114,7 +114,7 @@ This is the default name used when running Eat."
   :type 'boolean
   :group 'eat-ui)
 
-(defcustom eat-term-scrollback-size 131072 ; 128 K
+(defcustom eat-term-scrollback-size (* 128 1024) ; 128 K
   "Size of scrollback area in characters.  nil means unlimited."
   :type '(choice integer (const nil))
   :group 'eat-term
@@ -161,6 +161,7 @@ This is left disabled for security reasons."
   "Custom type specification for Eat's cursor type variables.")
 
 (defcustom eat-default-cursor-type
+  ;; Why are you checking the `default-value'?
   `(,(default-value 'cursor-type) nil nil)
   "Cursor to use in Eat buffer.
 
@@ -198,6 +199,8 @@ blinking frequency of cursor."
   :group 'eat-ui
   :group 'eat-ehell)
 
+;; Do you think combining this and `eat-maximum-latency' into a single
+;; variable makes sense?
 (defcustom eat-minimum-latency 0.008
   "Minimum display latency in seconds.
 
@@ -445,6 +448,10 @@ If your process is choking on big inputs, try lowering the value."
 
 (put 'eat-term-color-bright-white 'face-alias 'eat-term-color-15)
 
+;; Perhaps you could automatically generate this block and make it
+;; more maintainable?  `defface' is just a wrapper around
+;; `custom-declare-face', so you could invoke that in a loop that
+;; defines all the faces.
 (defface eat-term-color-16
   '((t :foreground "#000000" :background "#000000"))
   "Face used to render text with 16th color of 256 color palette."
@@ -1732,6 +1739,9 @@ Treat LINE FEED (?\\n) as the line delimiter."
                         (< (point) (point-max)))
                     (< moved n))
           (cl-incf moved)
+          ;; I have a hunch that there should be a simpler way to do
+          ;; this.  Something involving `line-end-position' and/or
+          ;; `forward-line'.
           (and (search-forward "\n" nil 'move)
                (= moved n)
                (goto-char (match-beginning 0))))
@@ -1775,7 +1785,7 @@ Treat LINE FEED (?\\n) as the line delimiter."
   ;; Move to the beginning of line, record the point, and return that
   ;; point and the distance of that point from current line in lines.
   (save-excursion
-    (let* ((moved (eat--t-goto-eol n)))
+    (let ((moved (eat--t-goto-eol n)))
       (cons (point) moved))))
 
 (defun eat--t-col-motion (n)
@@ -1823,9 +1833,9 @@ Assume all characters occupy a single column."
   (eat--t-col-motion n))
 
 (defun eat--t-repeated-insert (c n &optional face)
-  "Insert C, N times.
+  "Insert character C, N times.
 
-C is a character.  FACE is the face to use, or nil."
+FACE is the face to use, or nil."
   (let ((str (make-string n c)))
     (insert (if face (propertize str 'face face) str))))
 
@@ -1841,6 +1851,7 @@ where `*' indicates point."
                 (point) 'eat--t-wrap-line nil limit)))
   ;; Remove the newline.
   (when (< (point) (or limit (point-max)))
+    ;; What is the point of using `1value' here?
     (1value (cl-assert (1value (= (1value (char-after)) ?\n))))
     (delete-char 1)))
 
@@ -1855,7 +1866,7 @@ For example: when THRESHOLD is 3, \"*foobarbaz\" is converted to
       ;; Go to the threshold column.
       (eat--t-goto-col threshold)
       ;; Are we at the end of line?
-      (if (eq (char-after) ?\n)
+      (if (eq (char-after) ?\n)         ;`eolp'?
           ;; We are already at the end of line, so move to the next
           ;; line and start from the beginning.
           (forward-char)
@@ -1981,7 +1992,7 @@ Don't `set' it, bind it to a value with `let'.")
 
 (defun eat--t-reset ()
   "Reset terminal."
-  (let* ((disp (eat--t-term-display eat--t-term)))
+  (let ((disp (eat--t-term-display eat--t-term)))
     ;; Reset most of the things to their respective default values.
     (setf (eat--t-term-parser-state eat--t-term) nil)
     (setf (eat--t-disp-begin disp) (point-min-marker))
@@ -2011,7 +2022,7 @@ Don't `set' it, bind it to a value with `let'.")
     (setf (eat--t-term-mouse-encoding eat--t-term) nil)
     (setf (eat--t-term-focus-event-mode eat--t-term) nil)
     ;; Clear everything.
-    (delete-region (point-min) (point-max))
+    (erase-buffer)
     ;; Inform the UI about our new state.
     (funcall (eat--t-term-grab-mouse-fn eat--t-term) eat--t-term nil)
     (funcall (eat--t-term-set-focus-ev-mode-fn eat--t-term)
@@ -2036,7 +2047,7 @@ display."
     (unless (zerop n)
       ;; Move to the Nth next column, use spaces to reach that column
       ;; if needed.
-      (eat--t-repeated-insert ?\  (- n (eat--t-col-motion n)))
+      (eat--t-repeated-insert ?\s (- n (eat--t-col-motion n)))
       (cl-incf (eat--t-cur-x cursor) n))))
 
 (defun eat--t-cur-left (&optional n)
@@ -2297,6 +2308,7 @@ of range, place cursor at the edge of display."
   "Change character set to CHARSET.
 
 CHARSET should be one of `g0', `g1', `g2' and `g3'."
+  (cl-assert (memq charset '(g0 g1 g2 g3)))
   (setf (car (eat--t-term-charset eat--t-term)) charset))
 
 (defun eat--t-write (str)
@@ -2319,6 +2331,7 @@ CHARSET should be one of `g0', `g1', `g2' and `g3'."
             ('dec-line-drawing
              (let ((s (copy-sequence str)))
                (dotimes (i (length s))
+                 ;; Perhaps you should pull out the alist into a `defconst'
                  (let ((replacement (alist-get (aref s i)
                                                '((?+ . ?→)
                                                  (?, . ?←)
@@ -2476,7 +2489,7 @@ N default to 1."
               (eat--t-carriage-return)
             ;; If we are somehow moved from the end of terminal,
             ;; `eat--t-beg-of-next-line' is the best option.
-            (if (/= (point) (point-max))
+            (if (/= (point) (point-max)) ;(eobp)?
                 (eat--t-beg-of-next-line 1)
               ;; We are still at the end!  We can can simply insert a
               ;; newline and update the cursor position.
@@ -2561,7 +2574,7 @@ N defaults to 0.  When N is 0, erase cursor to end of line.  When N is
   (let* ((disp (eat--t-term-display eat--t-term))
          (face (eat--t-term-face eat--t-term))
          (cursor (eat--t-disp-cursor disp)))
-    (pcase n
+    (pcase-exhaustive n                 ;
       ((or 0 'nil (pred (< 2)))
        ;; Delete cursor position (inclusive) to end of line.
        (delete-region (point) (car (eat--t-eol)))
@@ -2619,7 +2632,7 @@ to (1, 1).  When N is 3, also erase the scrollback."
   (let* ((disp (eat--t-term-display eat--t-term))
          (face (eat--t-term-face eat--t-term))
          (cursor (eat--t-disp-cursor disp)))
-    (pcase n
+    (pcase-exhaustive n
       ((or 0 'nil (pred (< 3)))
        ;; Delete from cursor position (inclusive) to end of terminal.
        (delete-region (point) (point-max))
@@ -2631,14 +2644,14 @@ to (1, 1).  When N is 3, also erase the scrollback."
          ;; integer.
          (let ((pos (point)))
            ;; Fill current line.
-           (eat--t-repeated-insert ?\  (1+ (- (eat--t-disp-width disp)
+           (eat--t-repeated-insert ?\s (1+ (- (eat--t-disp-width disp)
                                               (eat--t-cur-x cursor)))
                                    (eat--t-face-face face))
            ;; Fill the following lines.
            (dotimes (_ (- (eat--t-disp-height disp)
                           (eat--t-cur-y cursor)))
              (insert ?\n)
-             (eat--t-repeated-insert ?\  (eat--t-disp-width disp)
+             (eat--t-repeated-insert ?\s (eat--t-disp-width disp)
                                      (eat--t-face-face face)))
            ;; Restore position.
            (goto-char pos))))
@@ -2656,7 +2669,7 @@ to (1, 1).  When N is 3, also erase the scrollback."
          (if (not (eat--t-face-bg face))
              (eat--t-repeated-insert ?\n (1- y))
            (dotimes (_ (1- y))
-             (eat--t-repeated-insert ?\  (eat--t-disp-width disp)
+             (eat--t-repeated-insert ?\s (eat--t-disp-width disp)
                                      (eat--t-face-face face))
              (insert ?\n)))
          ;; Fill the current line to keep the cursor unmoved.  Use
@@ -2906,18 +2919,16 @@ position."
     ;; on failure.
     (when (and (<= scroll-begin (eat--t-cur-y cursor) scroll-end)
                (not (zerop n)))
+      (eat--t-goto-bol)
+      (eat--t-repeated-insert ?\  (1- (eat--t-cur-x cursor))
+                              (and (eat--t-face-bg face)
+                                   (eat--t-face-face face)))
       (goto-char
-       (prog1
-           (progn
-             ;; This function doesn't move the cursor, but pushes all
-             ;; the line below and including current line.  So to keep
-             ;; the cursor unmoved, go to the beginning of line and
-             ;; insert enough spaces to not move the cursor.
-             (eat--t-goto-bol)
-             (eat--t-repeated-insert ?\  (1- (eat--t-cur-x cursor))
-                                     (and (eat--t-face-bg face)
-                                          (eat--t-face-face face)))
-             (point))
+       ;; This function doesn't move the cursor, but pushes all
+       ;; the line below and including current line.  So to keep
+       ;; the cursor unmoved, go to the beginning of line and
+       ;; insert enough spaces to not move the cursor.
+       (prog1 (point)
          ;; Insert N lines.
          (if (not (eat--t-face-bg face))
              (eat--t-repeated-insert ?\n n)
@@ -3189,7 +3200,8 @@ TOP defaults to 1 and BOTTOM defaults to the height of the display."
                 nil t)))))
     ;; Update face according to the attributes.
     (setf (eat--t-face-face face)
-          `(,@(when-let ((fg (or (if (eat--t-face-conceal face)
+          ;; `while' is for side-effects, `and' is for expressions
+          `(,@(and-let* ((fg (or (if (eat--t-face-conceal face)
                                      (eat--t-face-bg face)
                                    (eat--t-face-fg face))
                                  (cond
@@ -3201,31 +3213,31 @@ TOP defaults to 1 and BOTTOM defaults to the height of the display."
                           :background
                         :foreground)
                       fg))
-            ,@(when-let ((bg (or (eat--t-face-bg face)
+            ,@(and-let* ((bg (or (eat--t-face-bg face)
                                  (and (eat--t-face-inverse face)
                                       (face-background 'default)))))
                 (list (if (eat--t-face-inverse face)
                           :foreground
                         :background)
                       bg))
-            ,@(when-let ((underline (eat--t-face-underline face)))
+            ,@(and-let* ((underline (eat--t-face-underline face)))
                 (list
                  :underline
                  (list :color (eat--t-face-underline-color face)
                        :style underline)))
-            ,@(when-let ((crossed (eat--t-face-crossed face)))
+            ,@(and-let* ((crossed (eat--t-face-crossed face)))
                 ;; REVIEW: How about colors?  No terminal supports
                 ;; crossed attribute with colors, so we'll need to be
                 ;; creative to add the feature.
                 `(:strike-through t))
             :inherit
-            (,@(when-let ((intensity (eat--t-face-intensity face)))
+            (,@(and-let* ((intensity (eat--t-face-intensity face)))
                  (list intensity))
-             ,@(when-let ((italic (eat--t-face-italic face)))
+             ,@(and-let* ((italic (eat--t-face-italic face)))
                  (cl-assert (1value (eq (1value italic)
                                         'eat-term-italic)))
                  (list (1value italic)))
-             ,@(when-let ((blink (eat--t-face-blink face)))
+             ,@(and-let* ((blink (eat--t-face-blink face)))
                  (list blink))
              ,(eat--t-face-font face))))))
 
@@ -3261,7 +3273,7 @@ MODE should be one of nil and `x10', `normal', `button-event',
     (setf (eat--t-term-mouse-pressed eat--t-term) nil))
   ;; Inform the UI.
   (funcall (eat--t-term-grab-mouse-fn eat--t-term) eat--t-term
-           (pcase mode
+           (pcase-exhaustive mode
              ('x10 :click)
              ('normal :modifier-click)
              ('button-event :drag)
@@ -3327,6 +3339,8 @@ output."
   (funcall
    (eat--t-term-input-fn eat--t-term) eat--t-term
    (let ((rgb (color-values (face-foreground 'default))))
+     ;; I wonder if it would make sense to write a `rx'-like macro for
+     ;; generating these escape codes.
      (format "\e]10;%04x/%04x/%04x\e\\"
              (pop rgb) (pop rgb) (pop rgb)))))
 
@@ -3788,17 +3802,15 @@ DATA is the selection data encoded in base64."
                                       (rx ?\\))
                                     output index)))
            (if (not match)
-               (progn
-                 ;; Not found, store the text to process it later when
-                 ;; we get the end of string.
-                 (setf (eat--t-term-parser-state eat--t-term)
-                       `(,state ,(concat buf (substring output
-                                                        index))))
-                 (setq index (length output)))
+               ;; Not found, store the text to process it later when
+               ;; we get the end of string.
+               (setf (eat--t-term-parser-state eat--t-term)
+                     `(,state ,(concat buf (substring output
+                                                      index)))
+                     index (length output))
              ;; Matched!  Get the string from the output and previous
              ;; runs.
-             (let ((str (concat buf (substring output index
-                                               match))))
+             (let ((str (concat buf (substring output index match))))
                (setq index (match-end 0))
                ;; Is it really the end of string?
                (if (and (= (aref output match) ?\\)
@@ -3937,7 +3949,7 @@ DATA is the selection data encoded in base64."
         ;; Calculate the beginning position of display.
         (goto-char (point-max))
         ;; TODO: This part needs explanation.
-        (let* ((disp-begin (car (eat--t-bol (- (1- height))))))
+        (let ((disp-begin (car (eat--t-bol (- (1- height))))))
           (when (< (eat--t-disp-begin disp) disp-begin)
             (goto-char (max (- (eat--t-disp-begin disp) 1)
                             (point-min)))
@@ -3985,6 +3997,10 @@ DATA is the selection data encoded in base64."
   "Setup the environment for TERMINAL and eval BODY in it."
   (declare (indent 1))
   `(let ((eat--t-term ,terminal))
+     ;; This won't change much here, because the next line would
+     ;; trigger a similar exception, but there might be some other
+     ;; place where an explicit check could be of use.
+     (cl-check-type eat--t-term eat--t-term)
      (with-current-buffer (eat--t-term-buffer eat--t-term)
        (save-excursion
          (save-restriction
@@ -4038,6 +4054,7 @@ To set it, use (`setf' (`eat-term-input-function' TERMINAL) FUNCTION),
 where FUNCTION is the input function."
   (eat--t-term-input-fn terminal))
 
+;; Perhaps require `gv' at the top?
 (gv-define-setter eat-term-input-function (function terminal)
   `(setf (eat--t-term-input-fn ,terminal) ,function))
 
@@ -4386,8 +4403,8 @@ client process may get confused."
                     (setq tmp (get char 'ascii-character))
                     (setq char tmp))))
            (when (numberp char)
-             (let* ((base (event-basic-type char))
-                    (mods (event-modifiers char)))
+             (let ((base (event-basic-type char))
+                   (mods (event-modifiers char)))
                ;; Try to avoid event-convert-list if possible.
                (if (and (characterp char)
                         (not (memq 'meta mods))
@@ -4399,7 +4416,7 @@ client process may get confused."
                  (let ((ch (pcase (event-convert-list
                                    (append (remq 'meta mods)
                                            (list base)))
-                             (?\C-\  ?\C-@)
+                             (?\C-\s  ?\C-@)
                              (?\C-/ ?\C-?)
                              (?\C-- ?\C-_)
                              (c c))))
@@ -4608,9 +4625,9 @@ keywords:
 EXCEPTIONS is a list of key sequences to not bind.  Don't use
 \"M-...\" key sequences in EXCEPTIONS, use \"ESC ...\" instead."
   (let ((map (make-sparse-keymap)))
-    (cl-labels ((bind (key)
-                  (unless (member key exceptions)
-                    (define-key map key input-command))))
+    (cl-flet ((bind (key)
+                (unless (member key exceptions)
+                  (define-key map key input-command))))
       (when (memq :ascii categories)
         ;; Bind ASCII and self-insertable characters except ESC and
         ;; DEL.
@@ -4618,7 +4635,7 @@ EXCEPTIONS is a list of key sequences to not bind.  Don't use
         (cl-loop
          for i from ?\C-@ to ?~
          do (unless (= i meta-prefix-char)
-              (bind `[,i])))
+              (bind (vector i))))
         ;; Bind `backspace', `delete', `deletechar', and all modified
         ;; variants.
         (dolist (key '( backspace C-backspace
@@ -4780,11 +4797,12 @@ return \"eat-color\", otherwise return \"eat-mono\"."
 (defvar eat--fast-blink-timer nil
   "Timer for blinking rapidly blinking text.")
 
+(declare-function face-remap-add-relative "face-remap"
+                    (face &rest specs))
+(declare-function face-remap-remove-relative "face-remap" (cookie))
+
 (defun eat--flip-slow-blink-state ()
   "Flip the state of slowly blinking text."
-  (declare-function face-remap-add-relative "face-remap"
-                    (face &rest specs))
-  (declare-function face-remap-remove-relative "face-remap" (cookie))
   (face-remap-remove-relative eat--slow-blink-remap)
   (setq eat--slow-blink-remap
         (face-remap-add-relative
@@ -4794,9 +4812,6 @@ return \"eat-color\", otherwise return \"eat-mono\"."
 
 (defun eat--flip-fast-blink-state ()
   "Flip the state of rapidly blinking text."
-  (declare-function face-remap-add-relative "face-remap"
-                    (face &rest specs))
-  (declare-function face-remap-remove-relative "face-remap" (cookie))
   (face-remap-remove-relative eat--fast-blink-remap)
   (setq eat--fast-blink-remap
         (face-remap-add-relative
@@ -4853,6 +4868,7 @@ return \"eat-color\", otherwise return \"eat-mono\"."
     (face-remap-remove-relative eat--fast-blink-remap)
     (remove-hook 'pre-command-hook #'eat--blink-stop-timers t)
     (remove-hook 'post-command-hook #'eat--blink-start-timers t)
+    ;; I think `mapc' comes in nicely here
     (kill-local-variable 'eat--slow-blink-state)
     (kill-local-variable 'eat--fast-blink-state)
     (kill-local-variable 'eat--slow-blink-remap)
@@ -5140,6 +5156,7 @@ ARG is passed to `yank-pop', which see."
 ;; and commentary.
 (defvar eat-mode-map
   (let ((map (make-sparse-keymap)))
+    ;; Why not use `kbd'?
     (define-key map [?\C-c ?\M-d] #'eat-char-mode)
     (define-key map [?\C-c ?\C-j] #'eat-semi-char-mode)
     (define-key map [?\C-c ?\C-k] #'eat-kill-process)
@@ -5216,13 +5233,13 @@ ARG is passed to `yank-pop', which see."
 (defun eat-semi-char-mode ()
   "Switch to semi-char mode."
   (interactive)
-  (if (not eat--terminal)
-      (error "Process not running")
-    (setq buffer-read-only nil)
-    (eat--char-mode -1)
-    (eat--semi-char-mode +1)
-    (eat--grab-mouse nil eat--mouse-grabbing-type)
-    (force-mode-line-update)))
+  (unless eat--terminal
+    (error "Process not running"))
+  (setq buffer-read-only nil)
+  (eat--char-mode -1)
+  (eat--semi-char-mode +1)
+  (eat--grab-mouse nil eat--mouse-grabbing-type)
+  (force-mode-line-update))
 
 (defun eat-char-mode ()
   "Switch to char mode."
@@ -5302,7 +5319,7 @@ selection, or nil if none."
 
 (defun eat--bell (_)
   "Ring the bell."
-  (beep t))
+  (ding t))
 
 \f
 ;;;;; Major Mode.
@@ -5340,6 +5357,7 @@ END if it's safe to do so."
 (define-derived-mode eat-mode fundamental-mode "Eat"
   "Major mode for Eat."
   :group 'eat-ui
+  ;; You can abbreviate parts of the definition with `setq-local'.
   (make-local-variable 'buffer-read-only)
   (make-local-variable 'buffer-undo-list)
   (make-local-variable 'filter-buffer-substring-function)
@@ -5372,8 +5390,8 @@ END if it's safe to do so."
                  (:propertize
                   "semi-char"
                   help-echo
-                  ,(concat "mouse-1: Switch to char mode, "
-                           "mouse-3: Switch to emacs mode")
+                  ,"mouse-1: Switch to char mode, \
+mouse-3: Switch to emacs mode"
                   mouse-face mode-line-highlight
                   local-map
                   (keymap
@@ -5469,7 +5487,7 @@ OS's."
         (l (length string)))
     (while (< i l)
       (process-send-string process (substring string i (min j l)))
-      (accept-process-output)
+      (accept-process-output)           ;does this require a timeout?
       (cl-incf i eat-input-chunk-size)
       (cl-incf j eat-input-chunk-size))))
 
@@ -5910,6 +5928,7 @@ PROGRAM can be a shell command."
             (eat-term-beginning eat--terminal)
             (eat-term-end eat--terminal)
           ;; TODO: Is `string-join' OK or should we use a loop?
+          ;; ODOT: What should be the issue with `string-join'?
           (eshell-output-filter
            process (string-join (nreverse queue))))))))
 
@@ -5941,6 +5960,9 @@ PROGRAM can be a shell command."
   (declare-function eshell-sentinel "esh-proc" (proc string))
   (when (buffer-live-p (process-buffer process))
     (with-current-buffer (process-buffer process)
+      ;; As you use `cl-letf'/`cl-letf*' with `symbol-function' quite
+      ;; frequently, perhaps it would make sense to define a macro
+      ;; that makes the process less repetitive?
       (cl-letf* ((process-send-string
                   (symbol-function #'process-send-string))
                  ((symbol-function #'process-send-string)
@@ -5974,6 +5996,8 @@ modify its argument to change the filter, the sentinel and invoke
                                       (expand-file-name command))
                                      args))))
               (apply make-process plist)
+            ;; `plist-put' is not destructive, you need to store the
+            ;; return value.
             (plist-put plist :filter #'eat--eshell-filter)
             (plist-put plist :sentinel #'eat--eshell-sentinel)
             (plist-put
@@ -6328,7 +6352,7 @@ FN, `eat-exec', which see."
             (push (cons var (symbol-value var)) variables)))
         (with-current-buffer buf
           (lisp-data-mode)
-          (insert ";; -*- lisp-data -*-\n")
+          (insert ";; -*- mode: lisp-data -*-\n") ;just a suggestion
           (eat--trace-log time 'create 'eat width height
                           variables))))))
 
@@ -6495,7 +6519,7 @@ FN is the original definition of `eat--eshell-cleanup', which see."
 (define-minor-mode eat-trace-mode
   "Toggle tracing Eat terminal."
   :global t
-  :require 'eat
+  :require 'eat                         ;why the `require', if the mode isn't autoloaded
   :lighter " Eat-Trace"
   (if eat-trace-mode
       (progn

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


> Bye, going to sleep.  Good night!
>
> Footnotes:
> [1]  https://codeberg.org/akib/emacs-eat/