From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: Feedback on getting rid of `term-suppress-hard-newline' Date: Thu, 21 Feb 2019 09:55:20 -0500 Message-ID: References: <87efanc576.fsf@gmail.com> <87sgxsr8p0.fsf@gmail.com> <871s56sw98.fsf@gmail.com> <87lg3d3g70.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="139426"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Feb 21 15:55:49 2019 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1gwplL-000a9N-NN for ged-emacs-devel@m.gmane.org; Thu, 21 Feb 2019 15:55:47 +0100 Original-Received: from localhost ([127.0.0.1]:33357 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gwplK-0002jz-J7 for ged-emacs-devel@m.gmane.org; Thu, 21 Feb 2019 09:55:46 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:53112) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gwpl9-0002iN-EV for emacs-devel@gnu.org; Thu, 21 Feb 2019 09:55:36 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gwpl8-00071W-5x for emacs-devel@gnu.org; Thu, 21 Feb 2019 09:55:35 -0500 Original-Received: from [195.159.176.226] (port=39244 helo=blaine.gmane.org) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gwpl5-0006yo-Br for emacs-devel@gnu.org; Thu, 21 Feb 2019 09:55:32 -0500 Original-Received: from list by blaine.gmane.org with local (Exim 4.89) (envelope-from ) id 1gwpl2-000Zkg-TD for emacs-devel@gnu.org; Thu, 21 Feb 2019 15:55:28 +0100 X-Injected-Via-Gmane: http://gmane.org/ Cancel-Lock: sha1:jOyhPTDrDI1buvmM4W2masLNnoE= X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 195.159.176.226 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 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.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:233519 Archived-At: John Shahid writes: > Are there more changes to the previously attached patch in order to > merge it into master ? Oh, sorry, I dropped the ball. Yes, it looks good, thank you very much. Of course I couldn't resist making further comments, but feel free to push it, Stefan > +(make-obsolete-variable 'term-suppress-hard-newline nil > + "term removes newlines used for wrapping on resize and when text is copied" > + "27.1") Please remove the third argument above (it should be the version). > + (add-function :filter-return > + (local 'filter-buffer-substring-function) > + 'term--filter-buffer-substring) ^ I'd put a # there. > +(defun term--insert-fake-newline (&optional count) > + (let ((old-point (point))) > + (term-insert-char ?\n count) > + (put-text-property old-point (point) 'term-line-wrap t))) `count` is always nil, AFAICT, so you can just drop this argument. > +(defun term--remove-fake-newlines () > + (goto-char (point-min)) > + (let (fake-newline) > + (while (setq fake-newline (next-single-property-change (point) > + 'term-line-wrap)) > + (goto-char fake-newline) > + (let ((inhibit-read-only t)) > + (delete-char 1))))) I'd add a test (or an assertion) that (eq ?\n (char-after))), in case the text-property ends up applied somewhere we didn't expect (e.g. because it was inherited via insert-and-inherit). > @@ -1147,7 +1177,19 @@ term-reset-size > (setq term-start-line-column nil) > (setq term-current-row nil) > (setq term-current-column nil) > - (goto-char point)))) > + (goto-char point)) > + (save-excursion > + ;; Add newlines to wrap long lines that are currently displayed > + (forward-line (- (term-current-row))) > + (beginning-of-line) > + (while (not (eobp)) > + (let* ((eol (save-excursion (end-of-line) (current-column)))) > + (when (> eol width) > + (move-to-column width) > + (let ((inhibit-read-only t)) > + (term--insert-fake-newline))) > + (unless (eobp) > + (forward-char))))))) I'd move this to a separate function. More importantly, I don't quite understand why (- (term-current-row))) should be the exactly correct number of lines to move back at the beginning, so please add a comment explaining it (or if it's not exactly right, the comment could explain why it's "right enough"). Also, I think you can simplify the loop by always calling `move-to-column` and never `current-column`, as in the 100% untested code below: (while (not (eobp)) (move-to-column width) (if (eolp) (forward-char) (let ((inhibit-read-only t)) (term--insert-fake-newline)))) > @@ -2895,20 +2937,20 @@ term-emulate-terminal > (let ((old-column (term-horizontal-column)) > (old-point (point)) > columns) > - (unless term-suppress-hard-newline > - (while (> (+ (length decoded-substring) old-column) > - term-width) > - (insert (substring decoded-substring 0 > - (- term-width old-column))) > - ;; Since we've enough text to fill the whole line, > - ;; delete previous text regardless of > - ;; `term-insert-mode's value. > - (delete-region (point) (line-end-position)) > - (term-down 1 t) > - (term-move-columns (- (term-current-column))) > - (setq decoded-substring > - (substring decoded-substring (- term-width old-column))) > - (setq old-column 0))) > + (while (> (+ (length decoded-substring) old-column) > + term-width) > + (insert (substring decoded-substring 0 > + (- term-width old-column))) > + ;; Since we've enough text to fill the whole line, > + ;; delete previous text regardless of > + ;; `term-insert-mode's value. > + (delete-region (point) (line-end-position)) > + (term-down 1 t) > + (term-move-columns (- (term-current-column))) > + (put-text-property (1- (point)) (point) 'term-line-wrap t) > + (setq decoded-substring > + (substring decoded-substring (- term-width old-column))) > + (setq old-column 0)) > (insert decoded-substring) > (setq term-current-column (current-column) > columns (- term-current-column old-column)) I think I'd leave the term-suppress-hard-newline test in for now (that's the point of marking is as obsolete).