From: Philip Kaludercic <philipk@posteo.net>
To: Akib Azmain Turja <akib@disroot.org>
Cc: Emacs Developer List <emacs-devel@gnu.org>
Subject: Re: [NonGNU ELPA] 11 new packages!
Date: Sun, 27 Nov 2022 20:26:32 +0000 [thread overview]
Message-ID: <87zgccta87.fsf@posteo.net> (raw)
In-Reply-To: <87cz98nxah.fsf@disroot.org> (Akib Azmain Turja's message of "Sun, 27 Nov 2022 23:04:54 +0600")
In-Reply-To: <87cz98nxah.fsf@disroot.org> (Akib Azmain Turja's message of "Sun, 27 Nov 2022 23:04:54 +0600")
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?
next prev parent reply other threads:[~2022-11-27 20:26 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-14 7:42 [NonGNU ELPA] 11 new packages! Akib Azmain Turja
2022-11-15 17:42 ` Akib Azmain Turja
2022-11-15 19:53 ` Filipp Gunbin
2022-11-16 12:22 ` Akib Azmain Turja
2022-11-16 16:53 ` Eli Zaretskii
2022-11-16 17:43 ` Akib Azmain Turja
2022-11-16 19:47 ` Eli Zaretskii
2022-11-17 14:27 ` Akib Azmain Turja
2022-11-17 18:41 ` Stefan Monnier
2022-11-17 18:51 ` Yuan Fu
2022-11-24 23:38 ` Richard Stallman
2022-11-17 18:56 ` Eli Zaretskii
2022-11-15 19:57 ` Philip Kaludercic
2022-11-16 12:25 ` Akib Azmain Turja
2022-11-16 16:07 ` Philip Kaludercic
2022-11-16 17:45 ` Akib Azmain Turja
2022-11-16 18:19 ` Philip Kaludercic
2022-11-17 14:28 ` Akib Azmain Turja
2022-11-21 18:32 ` Philip Kaludercic
2022-11-22 15:20 ` Akib Azmain Turja
2022-11-22 17:07 ` Philip Kaludercic
2022-11-22 17:42 ` Akib Azmain Turja
2022-11-25 8:29 ` Akib Azmain Turja
2022-11-25 16:32 ` Philip Kaludercic
2022-11-25 17:14 ` Akib Azmain Turja
2022-11-22 21:16 ` Stefan Monnier
2022-11-25 7:14 ` Philip Kaludercic
2022-11-25 7:22 ` Philip Kaludercic
2022-11-25 12:45 ` Akib Azmain Turja
2022-11-25 13:07 ` Akib Azmain Turja
2022-11-25 17:16 ` Akib Azmain Turja
2022-11-25 17:31 ` Philip Kaludercic
2022-11-26 5:53 ` Akib Azmain Turja
2022-11-26 20:12 ` Stefan Monnier
2022-11-25 19:07 ` Philip Kaludercic
2022-11-26 7:49 ` Akib Azmain Turja
2022-11-27 8:11 ` Philip Kaludercic
2022-11-27 19:22 ` Akib Azmain Turja
2022-11-27 19:55 ` Akib Azmain Turja
2022-11-27 20:30 ` Philip Kaludercic
2022-11-26 18:44 ` Akib Azmain Turja
2022-11-26 20:07 ` Philip Kaludercic
2022-11-26 20:17 ` Philip Kaludercic
2022-11-26 21:12 ` Akib Azmain Turja
2022-11-27 11:45 ` Philip Kaludercic
2022-11-27 17:04 ` Akib Azmain Turja
2022-11-27 20:26 ` Philip Kaludercic [this message]
2022-11-28 19:07 ` Akib Azmain Turja
2022-11-28 19:42 ` Philip Kaludercic
2022-11-28 20:32 ` Akib Azmain Turja
2022-11-28 20:57 ` Stefan Monnier
2022-11-27 18:40 ` [NonGNU ELPA] 12 " Akib Azmain Turja
2022-11-27 20:36 ` Philip Kaludercic
2022-11-27 21:11 ` Akib Azmain Turja
2022-11-27 6:40 ` [NonGNU ELPA] 11 " Akib Azmain Turja
2022-11-19 12:05 ` Richard Stallman
2022-11-19 12:17 ` Philip Kaludercic
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zgccta87.fsf@posteo.net \
--to=philipk@posteo.net \
--cc=akib@disroot.org \
--cc=emacs-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).