From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: John Shahid Newsgroups: gmane.emacs.devel Subject: Re: Feedback on getting rid of `term-suppress-hard-newline' Date: Sun, 24 Feb 2019 13:00:08 -0500 Message-ID: <87lg2512zb.fsf@gmail.com> References: <87efanc576.fsf@gmail.com> <87sgxsr8p0.fsf@gmail.com> <871s56sw98.fsf@gmail.com> <87lg3d3g70.fsf@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="108803"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: mu4e 1.1.0; emacs 27.0.50 To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun Feb 24 19:01:01 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 1gxy5E-000SCh-Oo for ged-emacs-devel@m.gmane.org; Sun, 24 Feb 2019 19:01:00 +0100 Original-Received: from localhost ([127.0.0.1]:54026 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gxy5D-0007sG-Oo for ged-emacs-devel@m.gmane.org; Sun, 24 Feb 2019 13:00:59 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:56229) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gxy4V-0007s8-3U for emacs-devel@gnu.org; Sun, 24 Feb 2019 13:00:16 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gxy4T-0001su-PE for emacs-devel@gnu.org; Sun, 24 Feb 2019 13:00:15 -0500 Original-Received: from mail-qt1-x82f.google.com ([2607:f8b0:4864:20::82f]:40554) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gxy4T-0001si-Gf for emacs-devel@gnu.org; Sun, 24 Feb 2019 13:00:13 -0500 Original-Received: by mail-qt1-x82f.google.com with SMTP id j36so8047354qta.7 for ; Sun, 24 Feb 2019 10:00:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=references:user-agent:from:to:subject:in-reply-to:date:message-id :mime-version; bh=8zoUqKqfQQr6jmQx3oM7KN/Pkf0dLGo80/JGQsScn8g=; b=pz/yF7Z8+NCm2gFUKG14EMvUMM9fyHSsxopWfccHpo5Go50RsfGzbjsDohQZJ4+PZ3 e2lmFDrRLNsZis2IiPMMJwAvjqtYevFBvCBajUk5Mb6kdd5oeL2nQq0yO0W0KvFE0Qfg 6uSq4vOgFJT4CzNtQ4lxwLqg18XPQgSvVV9/99irw+wJqpwDjnzcSDOr4MQyGe5Eelq1 sSCeKkGqNeOCGjEVxSurlWmxzzDB63RQrwDMDCYB2zLnVLKtVc0Cxfvv+orJFBdRhbCV QqYCv10x1omITM5k+Jmmt6/Sehul6V66VEo/FnIyZaB09083y9fMBQFMTAzaokrO+Yac FzAg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:references:user-agent:from:to:subject :in-reply-to:date:message-id:mime-version; bh=8zoUqKqfQQr6jmQx3oM7KN/Pkf0dLGo80/JGQsScn8g=; b=ALvjEVn74CYkk2ZMNA24jJQXYXyxQkPiZ7zgHlrLtvTnaacuGo6TerrBH4M/aT8a9F fwRrs1l5Q4NSpojuNac3tefhB16ZZus1Q/ET8Bjp1yZrOt+cglBWtYi7eRTVWC1F2k2N PIzEmaMcIGv4+geAS+HmOE1ex9HW2J4qVRswl1BCsSjqpQPoSfZZAM1HF94ExonU8X5g 9dtoGhCZWaGaXBEVUyChO7jshLVYNXBoCtrggjLcZesBrYQPx5/qTQTOnpCM5VnGpQHC 9uPwYAG+JSaO/zmxzpjFUW/Pm523IAqBbKW244fwcZNcM+QB/OJKv7k3wzTNz2KOVkyP 743Q== X-Gm-Message-State: AHQUAua6xtqsUttL24N+mXWeqaKFCOYmGDkblrqFXvEKohGeHLMcMTCn xvhZFTJvhOMof7BOMa8EGqgEd55J X-Google-Smtp-Source: AHgI3IZ5JBT+NBlfE7lJw7xDt1ev+FDrQ7mY0blOR3fqo7K+Bt/hlcI/JJtdJDxoRBlHxpyqVbkdvQ== X-Received: by 2002:ac8:587:: with SMTP id a7mr10771399qth.241.1551031212143; Sun, 24 Feb 2019 10:00:12 -0800 (PST) Original-Received: from amun (cpe-104-162-85-46.nyc.res.rr.com. [104.162.85.46]) by smtp.gmail.com with ESMTPSA id a3sm5078656qta.21.2019.02.24.10.00.10 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 24 Feb 2019 10:00:10 -0800 (PST) In-reply-to: X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4864:20::82f 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:233571 Archived-At: --=-=-= Content-Type: text/plain Stefan Monnier writes: > 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. No worries and thanks for taking the time to review this patch. > Of course I couldn't resist making further comments, but feel free to > push it, Great, I made some changes to the patch, see below. > 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). Done. I can't remember what I was thinking at the time when I wrote this, it was a long time ago. >> + (add-function :filter-return >> + (local 'filter-buffer-substring-function) >> + 'term--filter-buffer-substring) > ^ > I'd put a # there. Done. >> +(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. I got rid of this function. Replaced it with calls to `term-unwrap-line'. >> +(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). I added an assertion. I don't see any uses of `insert-and-inherit' in term.el and based on my limited testing this test passes, in other words the assertion didn't fail. >> @@ -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. Done, introduced a new function `term--unwrap-visible-long-lines'. > 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"). I replaced this call with using `term-home-marker'. I believe the two approaches are equivalent. I'm assuming that this marker will correctly account for the home position and I'm also assuming that terminal programs won't try to go past that marker. I think that the accounting could be off, specially during resizing, but when I was testing, that assumption didn't cause any noticeable issues. BTW, I tried to look for documentation on how terminals handle resize but couldn't find one. Maybe I should pick a terminal emulator and inspect the code. > 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)))) Done. [...] > I think I'd leave the term-suppress-hard-newline test in for now (that's > the point of marking is as obsolete). I reverted this change. Cheers, JS --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0001-Adjust-line-wrapping-on-window-resize-and-killing-te.patch >From 966edc96b34b998b04d1d9de8529c6d01dae8192 Mon Sep 17 00:00:00 2001 From: John Shahid Date: Sun, 20 Jan 2019 19:08:17 -0500 Subject: [PATCH] Adjust line wrapping on window resize and killing text * lisp/term.el (term-mode): Advice filter-buffer-substring-function to remove line unwrapping from killed text. (term-reset-size): Add or remove line unwrapping depending on the new terminal width. (term-suppress-hard-newline): Mark obsolete. (term-unwrap-line): Use text properties to be able to find the newlines later. --- lisp/term.el | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 54 insertions(+), 2 deletions(-) diff --git a/lisp/term.el b/lisp/term.el index f49777f94c..fdcc39de72 100644 --- a/lisp/term.el +++ b/lisp/term.el @@ -545,6 +545,9 @@ term-suppress-hard-newline :version "24.4" :type 'boolean :group 'term) +(make-obsolete-variable 'term-suppress-hard-newline nil + "27.1" + 'set) ;; Where gud-display-frame should put the debugging arrow. This is ;; set by the marker-filter, which scans the debugger's output for @@ -1116,6 +1119,9 @@ term-mode (set (make-local-variable 'font-lock-defaults) '(nil t)) + (add-function :filter-return + (local 'filter-buffer-substring-function) + #'term--filter-buffer-substring) (add-function :filter-return (local 'window-adjust-process-window-size-function) (lambda (size) @@ -1132,9 +1138,51 @@ term-mode (setq term-input-ring (make-ring term-input-ring-size))) (term-update-mode-line)) +(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) + (assert (eq ?\n (char-after))) + (let ((inhibit-read-only t)) + (delete-char 1))))) + +(defun term--filter-buffer-substring (content) + (with-temp-buffer + (insert content) + (term--remove-fake-newlines) + (buffer-string))) + +(defun term--unwrap-visible-long-lines (width) + ;; Unwrap lines longer than width using fake newlines. Only do it + ;; for lines that are currently visible (i.e. following the home + ;; marker). Invisible lines don't have to be unwrapped since they + ;; are unreachable using the cursor movement anyway. Not having to + ;; unwrap the entire buffer means the runtime of this function is + ;; bounded by the size of the screen instead of the buffer size. + + (save-excursion + ;; We will just assume that our accounting for the home marker is + ;; correct, i.e. programs will not try to reach any position + ;; earlier than this marker. + (goto-char term-home-marker) + + (move-to-column width) + (while (not (eobp)) + (if (eolp) + (forward-char) + (let ((inhibit-read-only t)) + (term-unwrap-line))) + (move-to-column width)))) + (defun term-reset-size (height width) (when (or (/= height term-height) (/= width term-width)) + ;; Delete all newlines used for wrapping + (when (/= width term-width) + (save-excursion + (term--remove-fake-newlines))) (let ((point (point))) (setq term-height height) (setq term-width width) @@ -1147,7 +1195,8 @@ 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)) + (term--unwrap-visible-long-lines width))) ;; Recursive routine used to check if any string in term-kill-echo-list ;; matches part of the buffer before point. @@ -3719,7 +3768,10 @@ term-down ;; if the line above point wraps around, add a ?\n to undo the wrapping. ;; FIXME: Probably should be called more than it is. (defun term-unwrap-line () - (when (not (bolp)) (insert-before-markers ?\n))) + (when (not (bolp)) + (let ((old-point (point))) + (insert-before-markers ?\n) + (put-text-property old-point (point) 'term-line-wrap t)))) (defun term-erase-in-line (kind) (when (= kind 1) ;; erase left of point -- 2.20.1 --=-=-=--