* bug#29118: 25.2.50; Undoing shell flush output results in weird state @ 2017-11-02 21:25 Allen Li 2017-11-10 1:26 ` Noam Postavsky 0 siblings, 1 reply; 11+ messages in thread From: Allen Li @ 2017-11-02 21:25 UTC (permalink / raw) To: 29118 Repro: 1. emacs -Q 2. M-x shell RET 3. echo hi RET 4. C-c C-o 5. C-/ 6. echo bye RET The shell buffer ends up in a weird state that's hard to describe. In GNU Emacs 25.2.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.10.8), modified by Debian Windowing system distributor 'The X.Org Foundation', version 11.0.11803000 System Description: Ubuntu 14.04 LTS ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#29118: 25.2.50; Undoing shell flush output results in weird state 2017-11-02 21:25 bug#29118: 25.2.50; Undoing shell flush output results in weird state Allen Li @ 2017-11-10 1:26 ` Noam Postavsky 2017-11-10 8:10 ` Eli Zaretskii 2017-11-11 7:20 ` bug#29118: 25.2.50; Undoing shell flush output results in weird state Allen Li 0 siblings, 2 replies; 11+ messages in thread From: Noam Postavsky @ 2017-11-10 1:26 UTC (permalink / raw) To: Allen Li; +Cc: 29118 found 29118 26.0.90 tags 29118 + confirmed quit Allen Li <vianchielfaura@gmail.com> writes: > Repro: > > 1. emacs -Q > 2. M-x shell RET > 3. echo hi RET > 4. C-c C-o > 5. C-/ > 6. echo bye RET > > The shell buffer ends up in a weird state that's hard to describe. The process mark ends up in the wrong place. That is, (process-mark (get-buffer-process (current-buffer))) gives a different answer after step 3 vs after step 5. Not really sure why, debugging undo-list stuff is kind of a pain. ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#29118: 25.2.50; Undoing shell flush output results in weird state 2017-11-10 1:26 ` Noam Postavsky @ 2017-11-10 8:10 ` Eli Zaretskii 2017-11-12 2:12 ` Noam Postavsky 2017-11-11 7:20 ` bug#29118: 25.2.50; Undoing shell flush output results in weird state Allen Li 1 sibling, 1 reply; 11+ messages in thread From: Eli Zaretskii @ 2017-11-10 8:10 UTC (permalink / raw) To: Noam Postavsky; +Cc: 29118, vianchielfaura > From: Noam Postavsky <npostavs@users.sourceforge.net> > Date: Thu, 09 Nov 2017 20:26:53 -0500 > Cc: 29118@debbugs.gnu.org > > debugging undo-list stuff is kind of a pain. Why is that a pain? What can we do to make its debugging experience better? Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#29118: 25.2.50; Undoing shell flush output results in weird state 2017-11-10 8:10 ` Eli Zaretskii @ 2017-11-12 2:12 ` Noam Postavsky 2017-11-12 4:47 ` Eli Zaretskii 2017-11-26 19:59 ` bug#29461: Wishlist: better facilities for debugging undo problems Noam Postavsky 0 siblings, 2 replies; 11+ messages in thread From: Noam Postavsky @ 2017-11-12 2:12 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 29118, vianchielfaura Eli Zaretskii <eliz@gnu.org> writes: >> debugging undo-list stuff is kind of a pain. > > Why is that a pain? Just from this very simple recipe, buffer-undo-list contains all of this (assuming you made no mistakes typing it in, it's unclear to me how to reproduce it non-interactively). And I find it pretty difficult to interpret buffer positions in my head. So interpreting what all the element means is ... a pain. (nil (22 . 25) (22 . 35) (#("*** output flushed *** ~/src/emacs$ " 0 23 (fontified t front-sticky #1=(field inhibit-line-move-field-capture) rear-nonsticky t field output inhibit-line-move-field-capture t) 23 36 (font-lock-face (comint-highlight-prompt) inhibit-line-move-field-capture t field output rear-nonsticky t front-sticky #1# fontified nil)) . 22) (#<marker at 38 in *shell*> . -36) (#<marker at 22 in *shell*> . -36) (#<marker at 22 in *shell*> . -23) (#<marker at 22 in *shell*> . -36) (nil font-lock-face (comint-highlight-prompt . #2=(comint-highlight-prompt)) 45 . 58) nil (nil font-lock-face #2# 45 . 58) (22 . 58) (#("~/src/emacs$ " 0 13 (fontified nil front-sticky (field inhibit-line-move-field-capture) rear-nonsticky t field output inhibit-line-move-field-capture t font-lock-face (comint-highlight-prompt))) . -22) (#<marker at 38 in *shell*> . -13) (#<marker at 22 in *shell*> . -13) (#("hi " 0 3 (fontified t front-sticky (field inhibit-line-move-field-capture) rear-nonsticky t field output inhibit-line-move-field-capture t)) . -22) 38 nil (nil font-lock-face nil 25 . 38) (21 . 38) (#("echo hi" 0 7 (fontified nil)) . 14) (#<marker at 38 in *shell*> . -7) (21 . 28) nil (14 . 21) nil (nil font-lock-face nil 1 . 14) (1 . 14) (t . 0)) > What can we do to make its debugging experience better? Some way of visualizing the effect of undo list elements. A way to replay and rewind without affecting the undo list. Implementing such a thing would probably be pretty difficult. I have some crude visualization for markers in yasnippet-debug.el (in GNU ELPA). ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#29118: 25.2.50; Undoing shell flush output results in weird state 2017-11-12 2:12 ` Noam Postavsky @ 2017-11-12 4:47 ` Eli Zaretskii 2017-11-26 19:59 ` bug#29461: Wishlist: better facilities for debugging undo problems Noam Postavsky 1 sibling, 0 replies; 11+ messages in thread From: Eli Zaretskii @ 2017-11-12 4:47 UTC (permalink / raw) To: Noam Postavsky; +Cc: 29118, vianchielfaura > From: Noam Postavsky <npostavs@users.sourceforge.net> > Cc: 29118@debbugs.gnu.org, vianchielfaura@gmail.com > Date: Sat, 11 Nov 2017 21:12:30 -0500 > > > What can we do to make its debugging experience better? > > Some way of visualizing the effect of undo list elements. A way to > replay and rewind without affecting the undo list. > > Implementing such a thing would probably be pretty difficult. I have > some crude visualization for markers in yasnippet-debug.el (in GNU > ELPA). Maybe you should file a wishlist bug about this, then. ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#29461: Wishlist: better facilities for debugging undo problems 2017-11-12 2:12 ` Noam Postavsky 2017-11-12 4:47 ` Eli Zaretskii @ 2017-11-26 19:59 ` Noam Postavsky 1 sibling, 0 replies; 11+ messages in thread From: Noam Postavsky @ 2017-11-26 19:59 UTC (permalink / raw) To: 29461 Severity: wishlist See also https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29118#21 To make debugging trouble with undo easier, it would nice to have a way to "replay" and "rewind" operations to easily see what they do. And a way to visualize the meaning of undo list elements without actually "playing" them, as it's hard to see what a pair of buffer positions refers to at a glance. I've written some crude visualization code for markers under elpa/packages/yasnippet/yasnippet-debug.el. ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#29118: 25.2.50; Undoing shell flush output results in weird state 2017-11-10 1:26 ` Noam Postavsky 2017-11-10 8:10 ` Eli Zaretskii @ 2017-11-11 7:20 ` Allen Li 2017-11-12 18:30 ` Noam Postavsky 1 sibling, 1 reply; 11+ messages in thread From: Allen Li @ 2017-11-11 7:20 UTC (permalink / raw) To: Noam Postavsky; +Cc: 29118 On Thu, Nov 9, 2017 at 5:26 PM, Noam Postavsky <npostavs@users.sourceforge.net> wrote: > found 29118 26.0.90 > tags 29118 + confirmed > quit > > Allen Li <vianchielfaura@gmail.com> writes: > >> Repro: >> >> 1. emacs -Q >> 2. M-x shell RET >> 3. echo hi RET >> 4. C-c C-o >> 5. C-/ >> 6. echo bye RET >> >> The shell buffer ends up in a weird state that's hard to describe. > > The process mark ends up in the wrong place. That is, > > (process-mark (get-buffer-process (current-buffer))) > > gives a different answer after step 3 vs after step 5. Not really sure > why, debugging undo-list stuff is kind of a pain. Thanks. A couple of followup observations: 1. As a workaround, to fix the shell buffer, you can use comint-set-process-mark with point at where the end of the current prompt is. 2. Changing the process-mark insertion type before undoing prevents the bug: (set-marker-insertion-type (process-mark (get-buffer-process (current-buffer))) t) However, you need to set it back to nil after undoing or all user input will get inserted before process-mark. So one way to fix this would be to define a comint-undo that sets and resets the process-mark insertion type. That's probably not the most elegant solution. I think undo in general does not behave well with markers. undo will always rely on the insertion type of the markers, whereas the text to re-insert may well have been on either side of the marker. Perhaps undo should be improved to track which side of a marker deleted text was at. I think that might fix a whole class of undo-related bugs. However, this is very difficult to implement (I think, but I would be glad to be proven wrong). ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#29118: 25.2.50; Undoing shell flush output results in weird state 2017-11-11 7:20 ` bug#29118: 25.2.50; Undoing shell flush output results in weird state Allen Li @ 2017-11-12 18:30 ` Noam Postavsky 2017-11-13 8:29 ` Allen Li 2017-11-13 13:26 ` Stefan Monnier 0 siblings, 2 replies; 11+ messages in thread From: Noam Postavsky @ 2017-11-12 18:30 UTC (permalink / raw) To: Allen Li; +Cc: 29118, Barry OReilly, Stefan found 29118 24.4 quit Allen Li <vianchielfaura@gmail.com> writes: > 2. Changing the process-mark insertion type before undoing prevents the bug: > > (set-marker-insertion-type (process-mark (get-buffer-process > (current-buffer))) t) Ah, interesting. > However, you need to set it back to nil after undoing or all user > input will get inserted before process-mark. > > So one way to fix this would be to define a comint-undo that sets and > resets the process-mark insertion type. > That's probably not the most elegant solution. > > I think undo in general does not behave well with markers. undo will > always rely on the insertion type of the markers, > whereas the text to re-insert may well have been on either side of the marker. > > Perhaps undo should be improved to track which side of a marker > deleted text was at. Hmm, as far as I can tell, in this case the text was inserted on the "normal" side of the marker, but then the marker is moved afterwards. So I'm not sure if such tracking would help. (defun comint-output-filter (process string) ... ;; insert-before-markers is a bad thing. XXX ;; Luckily we don't have to use it any more, we use ;; window-point-insertion-type instead. (insert string) ;; Advance process-mark (set-marker (process-mark process) (point)) However, while looking at the undo code, I noticed that it checks for valid marker adjustments by checking that the marker position is the same as the POSITION of the deletion record. However, for a negative POSITION (indicating text was at the end of the deleted text) this comparison will always be false (markers can't have negative positions). (defun primitive-undo (n list) ... (`(,(and string (pred stringp)) . ,(and pos (pred integerp))) ... (and (eq (marker-buffer m) (current-buffer)) (= pos m) ; <<<<<<<<<<<< Always nil for negative `pos' (push marker-adj valid-marker-adjustments)))) I don't see why the position of point should affect the adjustment of other markers. This seems to have been added by [1: 37ea8275f7] to fix Bug#16818. Before that, if I understand correctly, all marker adjustments were applied without checking location at all. And indeed, this bug does not occur in Emacs 24.3. So I would suggest the following patch, which does seem to actually fix this bug. It might break some other things though; I have not tested it apart from the scenario in this bug. I'm adding the participants of Bug#16818 to Cc, in the hope they might have some more insight. --- i/lisp/simple.el +++ w/lisp/simple.el @@ -2579,7 +2579,7 @@ primitive-undo (let* ((marker-adj (pop list)) (m (car marker-adj))) (and (eq (marker-buffer m) (current-buffer)) - (= pos m) + (= (abs pos) m) (push marker-adj valid-marker-adjustments)))) ;; Insert string and adjust point (if (< pos 0) [1: 37ea8275f7]: 2014-03-24 22:47:39 -0400 Undo in region after markers in undo history relocated https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=37ea8275f7faad1192ddaba9f4a0789580675e17 ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#29118: 25.2.50; Undoing shell flush output results in weird state 2017-11-12 18:30 ` Noam Postavsky @ 2017-11-13 8:29 ` Allen Li 2017-11-13 13:26 ` Stefan Monnier 1 sibling, 0 replies; 11+ messages in thread From: Allen Li @ 2017-11-13 8:29 UTC (permalink / raw) To: Noam Postavsky; +Cc: 29118, Barry OReilly, Stefan On Sun, Nov 12, 2017 at 10:30 AM, Noam Postavsky <npostavs@users.sourceforge.net> wrote: > Hmm, as far as I can tell, in this case the text was inserted on the > "normal" side of the marker, but then the marker is moved afterwards. > So I'm not sure if such tracking would help. > > (defun comint-output-filter (process string) > ... > ;; insert-before-markers is a bad thing. XXX > ;; Luckily we don't have to use it any more, we use > ;; window-point-insertion-type instead. > (insert string) > > ;; Advance process-mark > (set-marker (process-mark process) (point)) > > > However, while looking at the undo code, I noticed that it checks for > valid marker adjustments by checking that the marker position is the > same as the POSITION of the deletion record. However, for a negative > POSITION (indicating text was at the end of the deleted text) this > comparison will always be false (markers can't have negative positions). I was mistaken, undo does track marker movement correctly; or mostly correctly, excluding this bug. > > (defun primitive-undo (n list) > ... > (`(,(and string (pred stringp)) . ,(and pos (pred integerp))) > ... > (and (eq (marker-buffer m) (current-buffer)) > (= pos m) ; <<<<<<<<<<<< Always nil for negative `pos' > (push marker-adj valid-marker-adjustments)))) > > I don't see why the position of point should affect the adjustment of > other markers. This seems to have been added by [1: 37ea8275f7] to fix > Bug#16818. Before that, if I understand correctly, all marker > adjustments were applied without checking location at all. And indeed, > this bug does not occur in Emacs 24.3. I agree with your analysis. The intent of the code is to make sure that the marker hasn't been moved from the original location before trying to restore it. In that case, comparing against the absolute value of pos makes sense, as we use the sign to carry extra information in band that is not relevant to the marker adjustment. > So I would suggest the following patch, which does seem to actually fix > this bug. It might break some other things though; I have not tested it > apart from the scenario in this bug. I'm adding the participants of > Bug#16818 to Cc, in the hope they might have some more insight. > > --- i/lisp/simple.el > +++ w/lisp/simple.el > @@ -2579,7 +2579,7 @@ primitive-undo > (let* ((marker-adj (pop list)) > (m (car marker-adj))) > (and (eq (marker-buffer m) (current-buffer)) > - (= pos m) > + (= (abs pos) m) > (push marker-adj valid-marker-adjustments)))) > ;; Insert string and adjust point > (if (< pos 0) > > [1: 37ea8275f7]: 2014-03-24 22:47:39 -0400 > Undo in region after markers in undo history relocated > https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=37ea8275f7faad1192ddaba9f4a0789580675e17 I have added this to my trusty personal monkeypatch library. I can confirm that it fixes the current bug. I can use this patch for a while and see if I run into any obvious issues. The patch seems quite straightforward and correct, but perhaps I am naive. ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#29118: 25.2.50; Undoing shell flush output results in weird state 2017-11-12 18:30 ` Noam Postavsky 2017-11-13 8:29 ` Allen Li @ 2017-11-13 13:26 ` Stefan Monnier 2017-11-15 13:05 ` Noam Postavsky 1 sibling, 1 reply; 11+ messages in thread From: Stefan Monnier @ 2017-11-13 13:26 UTC (permalink / raw) To: Noam Postavsky; +Cc: 29118, Allen Li, Barry OReilly > --- i/lisp/simple.el > +++ w/lisp/simple.el > @@ -2579,7 +2579,7 @@ primitive-undo > (let* ((marker-adj (pop list)) > (m (car marker-adj))) > (and (eq (marker-buffer m) (current-buffer)) > - (= pos m) > + (= (abs pos) m) > (push marker-adj valid-marker-adjustments)))) > ;; Insert string and adjust point > (if (< pos 0) Looks correct to me, this belongs in emacs-26, thanks. Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#29118: 25.2.50; Undoing shell flush output results in weird state 2017-11-13 13:26 ` Stefan Monnier @ 2017-11-15 13:05 ` Noam Postavsky 0 siblings, 0 replies; 11+ messages in thread From: Noam Postavsky @ 2017-11-15 13:05 UTC (permalink / raw) To: Stefan Monnier; +Cc: 29118, Allen Li, Barry OReilly tags 29118 fixed close 29118 26.1 quit Stefan Monnier <monnier@iro.umontreal.ca> writes: >> (and (eq (marker-buffer m) (current-buffer)) >> - (= pos m) >> + (= (abs pos) m) >> (push marker-adj valid-marker-adjustments)))) > Looks correct to me, this belongs in emacs-26, thanks. Thanks, pushed to emacs-26. [1: 1faade8821]: 2017-11-15 07:05:35 -0500 Fix marker adjustment for undo (Bug#29118) https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=1faade882165de2e9bd63702c59c8f26531c18ed ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-11-26 19:59 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-11-02 21:25 bug#29118: 25.2.50; Undoing shell flush output results in weird state Allen Li 2017-11-10 1:26 ` Noam Postavsky 2017-11-10 8:10 ` Eli Zaretskii 2017-11-12 2:12 ` Noam Postavsky 2017-11-12 4:47 ` Eli Zaretskii 2017-11-26 19:59 ` bug#29461: Wishlist: better facilities for debugging undo problems Noam Postavsky 2017-11-11 7:20 ` bug#29118: 25.2.50; Undoing shell flush output results in weird state Allen Li 2017-11-12 18:30 ` Noam Postavsky 2017-11-13 8:29 ` Allen Li 2017-11-13 13:26 ` Stefan Monnier 2017-11-15 13:05 ` Noam Postavsky
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).