From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philip Kaludercic Newsgroups: gmane.emacs.devel Subject: Re: [NonGNU ELPA] 11 new packages! Date: Sun, 27 Nov 2022 20:26:32 +0000 Message-ID: <87zgccta87.fsf@posteo.net> References: <87r0y6ug9z.fsf@disroot.org> <87y1sct2hp.fsf@posteo.net> <87k03vf5m8.fsf@disroot.org> <87edu2narn.fsf@posteo.net> <8735aieqtr.fsf@disroot.org> <87cz9mlq3o.fsf@posteo.net> <875yfdd5ad.fsf@disroot.org> <87wn7hh438.fsf@posteo.net> <87sfi5h3ne.fsf@posteo.net> <874julpgi4.fsf@disroot.org> <87v8n04o4b.fsf@posteo.net> <87cz98nxah.fsf@disroot.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="39529"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Emacs Developer List To: Akib Azmain Turja Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Nov 27 21:27:31 2022 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ozOFO-000A65-Ve for ged-emacs-devel@m.gmane-mx.org; Sun, 27 Nov 2022 21:27:31 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ozOEp-0005Sb-6O; Sun, 27 Nov 2022 15:26:55 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ozOEX-0005Q5-VV for emacs-devel@gnu.org; Sun, 27 Nov 2022 15:26:39 -0500 Original-Received: from mout02.posteo.de ([185.67.36.66]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ozOEV-0001mZ-Ha for emacs-devel@gnu.org; Sun, 27 Nov 2022 15:26:37 -0500 Original-Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id 86E09240103 for ; Sun, 27 Nov 2022 21:26:32 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1669580792; bh=GJ3+nBQj28JEn1+gU6Odp9VtfHZh4iajM8Fs4BOXFOc=; h=From:To:Cc:Subject:Date:From; b=aHp5Q/+kyWdzJ4rGfZXZkI4VqH76wi3ACHn+4sp5b9IhOvN3ZsSJ13Nt87TycluIo /WX9pdFB4csuFGGKAT8sG5JD/IkSfSN9YNwqEXlCC1YNHZV7/7Ii+GoSVSpXMzd7G8 lm5Ny/vZLkrhJxqbpv+xndCJOo4VY8TMijwIVzC30/R9GsN8rVd+E2UKIf23F+HPvT Ea4sKvR4ifyc2DVTVk0PUEEBRR0zwJ8WnxbloRnEWs0xEeVSYs9IXCOuDbBeUnCXIk EbQyFJxy/AO8IbocYVTi6AoZEWzH6/LPDfm8XAYB+Pbz7iB/utG3zhKDpCP0rYFE1u gzzEi49n/CLRw== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4NL0Vw05WNz9rxV; Sun, 27 Nov 2022 21:26:30 +0100 (CET) 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") Original-References: <87r0y6ug9z.fsf@disroot.org> <87y1sct2hp.fsf@posteo.net> <87k03vf5m8.fsf@disroot.org> <87edu2narn.fsf@posteo.net> <8735aieqtr.fsf@disroot.org> <87cz9mlq3o.fsf@posteo.net> <875yfdd5ad.fsf@disroot.org> <87wn7hh438.fsf@posteo.net> <87sfi5h3ne.fsf@posteo.net> <874julpgi4.fsf@disroot.org> <87v8n04o4b.fsf@posteo.net> <87cz98nxah.fsf@disroot.org> Received-SPF: pass client-ip=185.67.36.66; envelope-from=philipk@posteo.net; helo=mout02.posteo.de X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:300646 Archived-At: Akib Azmain Turja writes: >> @@ -161,6 +161,7 @@ This is left disabled for security reasons." >> "Custom type specification for Eat's cursor type variables.") >>=20=20 >> (defcustom eat-default-cursor-type >> + ;; Why are you checking the `default-value'? >> `(,(default-value 'cursor-type) nil nil) >> "Cursor to use in Eat buffer. >>=20=20 > > 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) >>=20=20 >> +;; 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. >>=20=20 > > 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)))) >>=20=20 >> (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 (=3D (1value (char-after)) ?\n)))) >> (delete-char 1))) >>=20=20 > > To convince testcover. Ah, OK. I have never used testcover (or honestly heard of it), so I was co= nfused. >> @@ -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. >>=20=20 >> 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)) >>=20=20 > > 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 `defco= nst' >> (let ((replacement (alist-get (aref s i) >> '((?+ . ?=E2=86=92) >> (?, . ?=E2=86=90) > > 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 (/=3D (point) (point-max)) >> + (if (/=3D (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 heigh= t 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 hei= ght 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)))))) >>=20=20 > > 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))))) >>=20=20 > > 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 (=3D (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)) >>=20=20 >> +;; Perhaps require `gv' at the top? >> (gv-define-setter eat-term-input-function (function terminal) >> `(setf (eat--t-term-input-fn ,terminal) ,function)) >>=20=20 > > 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)))))))) >>=20=20 > > 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-clea= nup', 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?