Re: [NonGNU ELPA] 11 new packages!

[Philip Kaludercic]

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

>> @@ -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.
>>  
>
> Because the current buffer (which is it?) while loading might have
> cursor-type locally set.

That make sense.

>> @@ -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.
>>  
>
> I think it's better to keep them separate, so that I can describe better
> in docstring.  And also I can't think of any good name of the combined
> variable.

If you think they should be separate, so be they.  But a intuitive
name might have been `eat-minimum'.

>> @@ -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)
>
> I think I do that very often.  TODO.  (A reminder for me.  I'll do any
> change you suggested later, as I'm working to fix a bug in Eat.)

FYI the changes I proposed aren't exhaustive.  It might be that I
proposed a change once that could be made multiple times.

>> @@ -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)))
>>  
>
> To convince testcover.

Ah, OK.  I have never used testcover (or honestly heard of it), so I was 
confused.

>> @@ -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)
>
> Obviously, no.  That'll remove narrowing, and narrowing to a
> fundamentally part of Eat's design.

I see, I missed that detail (all my comments are the result of my static
analysis).

>>  (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))
>>  
>
> Thanks, that assertion should be there.  TODO.

Again, this might also apply to a few other place, but you'd know that best.

>>  (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)
>>                                                 '((?+ . ?→)
>>                                                   (?, . ?←)
>
> Yes, I'm planning to use a hash-table (made with eval-when-compile).
> TODO.
>
>> @@ -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.
>
> Again, I'm afraid.

Even though `eobp' respects narrowed buffers?

>> @@ -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))))))
>>  
>
> Did you mean "when" instead of "while"?  TODO.

Yes.

>> @@ -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)))))
>>  
>
> rx generates regexp that (sort of) parses string.  Do you the something
> that does the opposite, i.e. joins parts into string?
>
> But yes, there indeed a bit repetition.

A macro or pure function that is in "the spirit of rx", converts a
symbolic expression into a textual encoding.  What I was thinking of
here was something like

  (eat-cc 'color 255 0 100) "^[]10;00ff/0000/0064^[\\"

You obviously know more about control codes that I do, so you'll be a
better judge of how this should look like.

>> @@ -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) ?\\)
>
> Somehow I prefer to use one setq for each variable.  Is setting multiple
> at once faster?  Benchmarking with "benchmark"...  Yes, about 1.5 times.
> TODO.

I guess because a multi-form setq requires a single funcall?

>> @@ -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)))
>
> Again...

I hope my comments don't sound too "harsh".  Even if I just changed
something without an explanation, I hope you won't interpret it as me
being annoyed.  I am sure if you look around in my code there are many
instances of me making these kinds of innocent mistakes.  It is just
easier for a fresh part of eyes to detect these, than someone who has a
loaded a mental image of the code's logic in their head.

>> @@ -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))
>>  
>
> What do you mean by "top?"  At the top of the file?  What's the
> advantage of that?

Top of the file.  I just find it cleaner, because then you'll avoid
autoloading a definition midway through loading a file.  It might also
be possible, that in this case the `require' call could be wrapped in a
`eval-when-compile', but I am not sure about that.

>> @@ -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)
>
> I don't know, I somehow still prefer this format in Eat.

That is fine, but you used `kbd' somewhere later on, so I wasn't sure
which you preferred.

>> @@ -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))))))))
>>  
>
> Ha ha, ODOT.  :)
> Nothing, that's a reminder for me find out the most efficient code.
> string-join creates garbage by throw all the string away, and using a
> loop might cause Eshell to generate more garbage.

I didn't consider it from that perspective.  If performance is of that
importance, then think about it, but my hunch is that I/O will
overshadow the overhead created by garbage, be it from strings or from a
temporary buffer.

>> @@ -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
>
> To shut package-lint up.

I am surprised to hear that.  Is this perhaps a bug in package-lint, or
did it have a good reason to recommend this?