* bug#57367: [PATCH] Speed up em-smart
@ 2022-08-23 20:06 Morgan Smith
2022-09-04 21:56 ` Lars Ingebrigtsen
0 siblings, 1 reply; 12+ messages in thread
From: Morgan Smith @ 2022-08-23 20:06 UTC (permalink / raw)
To: 57367; +Cc: johnw
[-- Attachment #1: Type: text/plain, Size: 306 bytes --]
Hello,
em-smart is very slow. This patch makes it much faster. idk why there
was all this redisplay stuff in the module as everything seems to work
great when I remove it all.
Thanks John Wiegley for writing this module! It's a pretty cool concept
and I'm having fun playing with it
Thanks,
Morgan
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Speed-up-em-smart.patch --]
[-- Type: text/x-patch, Size: 5907 bytes --]
From f015d6ac42d18bf4dbc2223dc6c96d964e4dd994 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Tue, 23 Aug 2022 15:47:17 -0400
Subject: [PATCH] Speed up em-smart
em-smart was forcibly re-displaying the screen upwards of 500 times
per screen of output. This caused the eshell to feel quite slow when
the module was in use. Everything seems to work just fine without any
redisplay stuff so I just tore it all out.
list/eshell/em-smart.el:
(eshell-mart-unload-hook): Remove eshell-refresh-windows
(eshell-smart-displayed, eshell-currently-handling-window,
eshell-refresh-windows, eshell-smart-redisplay): Delete
(eshell-smart-scroll-window): Add a bunch of logic originally from
eshell-smart-redisplay
(eshell-smart-initialize): Don't add hooks for deleted functions. Use
eshell-output-filter-functions to run eshell-smart-scroll-window
(eshell-smart-display-setup): Don't refresh windows
---
lisp/eshell/em-smart.el | 79 +++++++++++------------------------------
1 file changed, 20 insertions(+), 59 deletions(-)
diff --git a/lisp/eshell/em-smart.el b/lisp/eshell/em-smart.el
index 6768cee4c3..708f950d48 100644
--- a/lisp/eshell/em-smart.el
+++ b/lisp/eshell/em-smart.el
@@ -92,11 +92,7 @@ it to get a real sense of how it works."
:type 'hook
:group 'eshell-smart)
-(defcustom eshell-smart-unload-hook
- (list
- (lambda ()
- (remove-hook 'window-configuration-change-hook
- 'eshell-refresh-windows)))
+(defcustom eshell-smart-unload-hook nil
"A hook that gets run when `eshell-smart' is unloaded."
:type 'hook
:group 'eshell-smart)
@@ -159,9 +155,7 @@ The options are `begin', `after' or `end'."
;;; Internal Variables:
-(defvar eshell-smart-displayed nil)
(defvar eshell-smart-command-done nil)
-(defvar eshell-currently-handling-window nil)
;;; Functions:
@@ -174,10 +168,7 @@ The options are `begin', `after' or `end'."
(setq-local eshell-scroll-to-bottom-on-input nil)
(setq-local eshell-scroll-show-maximum-output t)
- (add-hook 'window-scroll-functions 'eshell-smart-scroll-window nil t)
- (add-hook 'window-configuration-change-hook 'eshell-refresh-windows)
-
- (add-hook 'eshell-output-filter-functions 'eshell-refresh-windows t t)
+ (add-hook 'eshell-output-filter-functions 'eshell-smart-scroll-window nil t)
(add-hook 'after-change-functions 'eshell-disable-after-change nil t)
@@ -193,29 +184,23 @@ The options are `begin', `after' or `end'."
(add-hook 'eshell-post-command-hook
'eshell-smart-maybe-jump-to-end nil t))))
-;; This is called by window-scroll-functions with two arguments.
-(defun eshell-smart-scroll-window (wind _start)
- "Scroll the given Eshell window WIND accordingly."
- (unless eshell-currently-handling-window
- (let ((inhibit-point-motion-hooks t)
- (eshell-currently-handling-window t))
- (with-selected-window wind
- (eshell-smart-redisplay)))))
-
-(defun eshell-refresh-windows (&optional frame)
- "Refresh all visible Eshell buffers."
- (let (affected)
- (walk-windows
- (lambda (wind)
- (with-current-buffer (window-buffer wind)
- (if eshell-mode
- (let (window-scroll-functions) ;;FIXME: Why?
- (eshell-smart-scroll-window wind (window-start))
- (setq affected t)))))
- 0 frame)
- (if affected
- (let (window-scroll-functions) ;;FIXME: Why?
- (eshell-redisplay)))))
+(defun eshell-smart-scroll-window ()
+ "Scroll the selected window to display as much output as possible, smartly."
+ (when (eq (window-buffer (selected-window)) (current-buffer))
+ (let ((top-point (point)))
+ (and (memq 'eshell-smart-display-move pre-command-hook)
+ (>= (point) eshell-last-input-start)
+ (< (point) eshell-last-input-end)
+ (set-window-start (selected-window)
+ (line-beginning-position) t))
+ (if (pos-visible-in-window-p (point-max))
+ (save-excursion
+ (goto-char (point-max))
+ (recenter -1)
+ (unless (pos-visible-in-window-p top-point)
+ (goto-char top-point)
+ (set-window-start (selected-window)
+ (line-beginning-position) t)))))))
(defun eshell-smart-display-setup ()
"Set the point to somewhere in the beginning of the last command."
@@ -232,8 +217,7 @@ The options are `begin', `after' or `end'."
(t
(error "Invalid value for `eshell-where-to-jump'")))
(setq eshell-smart-command-done nil)
- (add-hook 'pre-command-hook 'eshell-smart-display-move nil t)
- (eshell-refresh-windows))
+ (add-hook 'pre-command-hook 'eshell-smart-display-move nil t))
;; Called from after-change-functions with 3 arguments.
(defun eshell-disable-after-change (_b _e _l)
@@ -255,29 +239,6 @@ and the end of the buffer are still visible."
(goto-char (point-max))
(remove-hook 'pre-command-hook 'eshell-smart-display-move t)))
-(defun eshell-smart-redisplay ()
- "Display as much output as possible, smartly."
- (if (eobp)
- (save-excursion
- (recenter -1)
- ;; trigger the redisplay now, so that we catch any attempted
- ;; point motion; this is to cover for a redisplay bug
- (eshell-redisplay))
- (let ((top-point (point)))
- (and (memq 'eshell-smart-display-move pre-command-hook)
- (>= (point) eshell-last-input-start)
- (< (point) eshell-last-input-end)
- (set-window-start (selected-window)
- (line-beginning-position) t))
- (if (pos-visible-in-window-p (point-max))
- (save-excursion
- (goto-char (point-max))
- (recenter -1)
- (unless (pos-visible-in-window-p top-point)
- (goto-char top-point)
- (set-window-start (selected-window)
- (line-beginning-position) t)))))))
-
(defun eshell-smart-goto-end ()
"Like `end-of-buffer', but do not push a mark."
(interactive)
--
2.37.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#57367: [PATCH] Speed up em-smart
2022-08-23 20:06 bug#57367: [PATCH] Speed up em-smart Morgan Smith
@ 2022-09-04 21:56 ` Lars Ingebrigtsen
2022-09-05 19:01 ` Jim Porter
0 siblings, 1 reply; 12+ messages in thread
From: Lars Ingebrigtsen @ 2022-09-04 21:56 UTC (permalink / raw)
To: Morgan Smith; +Cc: Jim Porter, johnw, 57367
Morgan Smith <Morgan.J.Smith@outlook.com> writes:
> em-smart is very slow. This patch makes it much faster. idk why there
> was all this redisplay stuff in the module as everything seems to work
> great when I remove it all.
Skimming the patch, I think it makes sense, but I'm not that familiar
with eshell. Perhaps Jim has some comments; added to the CCs.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#57367: [PATCH] Speed up em-smart
2022-09-04 21:56 ` Lars Ingebrigtsen
@ 2022-09-05 19:01 ` Jim Porter
2022-09-05 21:48 ` Morgan Smith
0 siblings, 1 reply; 12+ messages in thread
From: Jim Porter @ 2022-09-05 19:01 UTC (permalink / raw)
To: Lars Ingebrigtsen, Morgan Smith; +Cc: johnw, 57367
On 9/4/2022 2:56 PM, Lars Ingebrigtsen wrote:
> Morgan Smith <Morgan.J.Smith@outlook.com> writes:
>
>> em-smart is very slow. This patch makes it much faster. idk why there
>> was all this redisplay stuff in the module as everything seems to work
>> great when I remove it all.
>
> Skimming the patch, I think it makes sense, but I'm not that familiar
> with eshell. Perhaps Jim has some comments; added to the CCs.
I don't know much about the smart scrolling module in Eshell, but I'll
try to familiarize myself with it and then take a look. I'm slightly
hesitant about deleting a bunch of code until I know why it was there in
the first place; from the comments in em-smart.el, it sounds like these
hooks are intentional:
;; @ While output is being generated from a command, the window will
;; be constantly reconfigured (until it would otherwise make no
;; difference) in order to always show you the most output from the
;; command possible. This happens if you change window sizes,
;; scroll, etc.
However, maybe there's a better way to do this, or failing that, to at
least provide some more options in the module so that users can tune the
performance to their liking.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#57367: [PATCH] Speed up em-smart
2022-09-05 19:01 ` Jim Porter
@ 2022-09-05 21:48 ` Morgan Smith
2022-09-05 21:51 ` Lars Ingebrigtsen
0 siblings, 1 reply; 12+ messages in thread
From: Morgan Smith @ 2022-09-05 21:48 UTC (permalink / raw)
To: Jim Porter; +Cc: Lars Ingebrigtsen, johnw, 57367
Hello,
Jim Porter <jporterbugs@gmail.com> writes:
> ;; @ While output is being generated from a command, the window will
> ;; be constantly reconfigured (until it would otherwise make no
> ;; difference) in order to always show you the most output from the
> ;; command possible. This happens if you change window sizes,
> ;; scroll, etc.
Yep the window is constantly reconfigured and that remains true even
with all the stuff I removed. I only removed stuff which forced a
redisplay. From what I can tell, we don't need to force redisplays as
everything shows up just fine. If I did my job correctly (which I
didn't, see below) then there is zero change to the logic and user
experience.
That being said I have discovered one bug in my patch. I use this
little tid bit to determine if I should scroll or not in the
eshell-output-filter-functions hook.
"(when (eq (window-buffer (selected-window)) (current-buffer)) <logic here>)"
Whereas the previous code used this logic in a few hooks:
(walk-windows
(lambda (wind)
(with-current-buffer (window-buffer wind)
(if eshell-mode
<logic here>)))
0 frame)
My code prevents the hook from being run on buffers that aren't the
eshell buffer (no clue how but it does) but it doesn't allow the hook to
run if the window isn't selected. The old code seems very complicated
though and will scroll all eshell buffers.
Is there a good way to ensure a hook is performed only on the buffer
that it was installed to?
Thanks,
Morgan
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#57367: [PATCH] Speed up em-smart
2022-09-05 21:48 ` Morgan Smith
@ 2022-09-05 21:51 ` Lars Ingebrigtsen
[not found] ` <DM5PR03MB31639CBC75F62622AC4E70ABC57E9@DM5PR03MB3163.namprd03.prod.outlook.com>
0 siblings, 1 reply; 12+ messages in thread
From: Lars Ingebrigtsen @ 2022-09-05 21:51 UTC (permalink / raw)
To: Morgan Smith; +Cc: Jim Porter, johnw, 57367
Morgan Smith <Morgan.J.Smith@outlook.com> writes:
> Is there a good way to ensure a hook is performed only on the buffer
> that it was installed to?
`add-hook' takes a LOCAL parameter. (But I haven't looked at the logic
here, so perhaps something else is needed.)
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#57367: [PATCH] Speed up em-smart
[not found] ` <DM5PR03MB31639CBC75F62622AC4E70ABC57E9@DM5PR03MB3163.namprd03.prod.outlook.com>
@ 2022-09-06 10:00 ` Lars Ingebrigtsen
2022-09-07 1:30 ` bug#57367: [PATCH V2] " Morgan Smith
0 siblings, 1 reply; 12+ messages in thread
From: Lars Ingebrigtsen @ 2022-09-06 10:00 UTC (permalink / raw)
To: Morgan Smith; +Cc: 57367
(Please keep the debbugs address in the CCs -- otherwise it won't reach
the bug tracker.)
Morgan Smith <Morgan.J.Smith@outlook.com> writes:
> So the hook is local to the buffer but it runs this bit of code:
>
> (set-window-start (selected-window) (line-beginning-position) t)
>
> So I want to set all windows that refer to the hook buffer to start at a
> certain location. Ideally I want to do this even if the window doesn't
> currently exist as I want it to be properly scrolled when I get back to
> it.
Hm, right.
> I guess that explains why the original author simply walked through
> all windows and also attached the hook to
> `window-configuration-change-hook'. I guess I could restore the
> original solution but it doesn't seem particularly elegant.
>
> Let me know if you have a better idea, otherwise I'll simply send
> another patch that puts some of the old stuff back in.
I think you probably have to wait until the window actually exists to
effect scrolling in it (you may have the same buffer in several windows,
or in none), really, so I think walking the windows is the only
practical solution here.
But there may be other tricks here -- perhaps somebody else has some
ideas.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#57367: [PATCH V2] Speed up em-smart
2022-09-06 10:00 ` Lars Ingebrigtsen
@ 2022-09-07 1:30 ` Morgan Smith
2022-09-07 12:55 ` Lars Ingebrigtsen
2022-09-09 4:36 ` Jim Porter
0 siblings, 2 replies; 12+ messages in thread
From: Morgan Smith @ 2022-09-07 1:30 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: 57367
[-- Attachment #1: Type: text/plain, Size: 342 bytes --]
Hello,
I've attached my patch V2.
This restores some more of the original logic that I have now realized
was indeed necessary. Again, this patch should not actually change
anything with respect to program logic, flow, or user experience. In my
limited testing, it seems to act just as it did before (but
significantly more performant).
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Speed-up-em-smart.patch --]
[-- Type: text/x-patch, Size: 6297 bytes --]
From 9e5b3d44019d08e12f95e5cc06c2273e3030e1bb Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Tue, 6 Sep 2022 21:18:51 -0400
Subject: [PATCH] Speed up em-smart
em-smart was forcibly re-displaying the screen upwards of 500 times
per screen of output. This caused the eshell to feel quite slow when
the module was in use. Everything seems to work just fine without any
redisplay stuff so I just tore it all out.
lisp/eshell/em-smart.el:
(eshell-smart-unload-hook): Remove `eshell-smart-scroll-windows'
instead of the now deleted `eshell-refresh-windows'
(eshell-smart-displayed, eshell-currently-handling-window,
eshell-refresh-windows, eshell-smart-redisplay): Delete
(eshell-smart-scroll-window): Rename to `eshell-smart-scroll-windows'
and add a bunch of logic originally from `eshell-refresh-windows'
(eshell-smart-initialize): Don't add a hook onto
`window-scroll-functions'. Replace `eshell-refresh-windows' with
`eshell-smart-scroll-windows'
(eshell-smart-display-setup): Don't refresh windows
(eshell-smart-scroll): Add function. Most of the logic is copied from
the now deleted `eshell-smart-redisplay'.
---
lisp/eshell/em-smart.el | 81 ++++++++++++++---------------------------
1 file changed, 28 insertions(+), 53 deletions(-)
diff --git a/lisp/eshell/em-smart.el b/lisp/eshell/em-smart.el
index 6768cee4c3..7763396a7d 100644
--- a/lisp/eshell/em-smart.el
+++ b/lisp/eshell/em-smart.el
@@ -96,7 +96,7 @@ it to get a real sense of how it works."
(list
(lambda ()
(remove-hook 'window-configuration-change-hook
- 'eshell-refresh-windows)))
+ 'eshell-smart-scroll-windows)))
"A hook that gets run when `eshell-smart' is unloaded."
:type 'hook
:group 'eshell-smart)
@@ -159,9 +159,7 @@ The options are `begin', `after' or `end'."
;;; Internal Variables:
-(defvar eshell-smart-displayed nil)
(defvar eshell-smart-command-done nil)
-(defvar eshell-currently-handling-window nil)
;;; Functions:
@@ -174,10 +172,9 @@ The options are `begin', `after' or `end'."
(setq-local eshell-scroll-to-bottom-on-input nil)
(setq-local eshell-scroll-show-maximum-output t)
- (add-hook 'window-scroll-functions 'eshell-smart-scroll-window nil t)
- (add-hook 'window-configuration-change-hook 'eshell-refresh-windows)
+ (add-hook 'eshell-output-filter-functions 'eshell-smart-scroll-windows nil t)
- (add-hook 'eshell-output-filter-functions 'eshell-refresh-windows t t)
+ (add-hook 'window-configuration-change-hook 'eshell-smart-scroll-windows nil t)
(add-hook 'after-change-functions 'eshell-disable-after-change nil t)
@@ -193,29 +190,14 @@ The options are `begin', `after' or `end'."
(add-hook 'eshell-post-command-hook
'eshell-smart-maybe-jump-to-end nil t))))
-;; This is called by window-scroll-functions with two arguments.
-(defun eshell-smart-scroll-window (wind _start)
- "Scroll the given Eshell window WIND accordingly."
- (unless eshell-currently-handling-window
- (let ((inhibit-point-motion-hooks t)
- (eshell-currently-handling-window t))
- (with-selected-window wind
- (eshell-smart-redisplay)))))
-
-(defun eshell-refresh-windows (&optional frame)
- "Refresh all visible Eshell buffers."
- (let (affected)
- (walk-windows
- (lambda (wind)
- (with-current-buffer (window-buffer wind)
- (if eshell-mode
- (let (window-scroll-functions) ;;FIXME: Why?
- (eshell-smart-scroll-window wind (window-start))
- (setq affected t)))))
- 0 frame)
- (if affected
- (let (window-scroll-functions) ;;FIXME: Why?
- (eshell-redisplay)))))
+(defun eshell-smart-scroll-windows ()
+ "Scroll all eshell windows to display as much output as possible, smartly."
+ (walk-windows
+ (lambda (wind)
+ (with-current-buffer (window-buffer wind)
+ (if eshell-mode
+ (eshell-smart-scroll wind))))
+ 0 t))
(defun eshell-smart-display-setup ()
"Set the point to somewhere in the beginning of the last command."
@@ -232,8 +214,7 @@ The options are `begin', `after' or `end'."
(t
(error "Invalid value for `eshell-where-to-jump'")))
(setq eshell-smart-command-done nil)
- (add-hook 'pre-command-hook 'eshell-smart-display-move nil t)
- (eshell-refresh-windows))
+ (add-hook 'pre-command-hook 'eshell-smart-display-move nil t))
;; Called from after-change-functions with 3 arguments.
(defun eshell-disable-after-change (_b _e _l)
@@ -255,28 +236,22 @@ and the end of the buffer are still visible."
(goto-char (point-max))
(remove-hook 'pre-command-hook 'eshell-smart-display-move t)))
-(defun eshell-smart-redisplay ()
- "Display as much output as possible, smartly."
- (if (eobp)
- (save-excursion
- (recenter -1)
- ;; trigger the redisplay now, so that we catch any attempted
- ;; point motion; this is to cover for a redisplay bug
- (eshell-redisplay))
- (let ((top-point (point)))
- (and (memq 'eshell-smart-display-move pre-command-hook)
- (>= (point) eshell-last-input-start)
- (< (point) eshell-last-input-end)
- (set-window-start (selected-window)
- (line-beginning-position) t))
- (if (pos-visible-in-window-p (point-max))
- (save-excursion
- (goto-char (point-max))
- (recenter -1)
- (unless (pos-visible-in-window-p top-point)
- (goto-char top-point)
- (set-window-start (selected-window)
- (line-beginning-position) t)))))))
+(defun eshell-smart-scroll (window)
+ "Scroll WINDOW to display as much output as possible, smartly."
+ (let ((top-point (point)))
+ (and (memq 'eshell-smart-display-move pre-command-hook)
+ (>= (point) eshell-last-input-start)
+ (< (point) eshell-last-input-end)
+ (set-window-start window
+ (line-beginning-position) t))
+ (if (pos-visible-in-window-p (point-max) window)
+ (save-excursion
+ (goto-char (point-max))
+ (recenter -1)
+ (unless (pos-visible-in-window-p top-point window)
+ (goto-char top-point)
+ (set-window-start window
+ (line-beginning-position) t))))))
(defun eshell-smart-goto-end ()
"Like `end-of-buffer', but do not push a mark."
--
2.37.2
[-- Attachment #3: Type: text/plain, Size: 205 bytes --]
Lars Ingebrigtsen <larsi@gnus.org> writes:
> (Please keep the debbugs address in the CCs -- otherwise it won't reach
> the bug tracker.)
Sorry, still trying to get the hang of gnus :P
Thanks,
Morgan
^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#57367: [PATCH V2] Speed up em-smart
2022-09-07 1:30 ` bug#57367: [PATCH V2] " Morgan Smith
@ 2022-09-07 12:55 ` Lars Ingebrigtsen
2022-09-09 4:36 ` Jim Porter
1 sibling, 0 replies; 12+ messages in thread
From: Lars Ingebrigtsen @ 2022-09-07 12:55 UTC (permalink / raw)
To: Morgan Smith; +Cc: Jim Porter, 57367
Morgan Smith <Morgan.J.Smith@outlook.com> writes:
> I've attached my patch V2.
>
> This restores some more of the original logic that I have now realized
> was indeed necessary. Again, this patch should not actually change
> anything with respect to program logic, flow, or user experience. In my
> limited testing, it seems to act just as it did before (but
> significantly more performant).
It think that makes sense... Jim, any comments?
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#57367: [PATCH V2] Speed up em-smart
2022-09-07 1:30 ` bug#57367: [PATCH V2] " Morgan Smith
2022-09-07 12:55 ` Lars Ingebrigtsen
@ 2022-09-09 4:36 ` Jim Porter
2023-09-06 22:46 ` bug#57367: [PATCH] " Stefan Kangas
1 sibling, 1 reply; 12+ messages in thread
From: Jim Porter @ 2022-09-09 4:36 UTC (permalink / raw)
To: Morgan Smith, Lars Ingebrigtsen; +Cc: 57367
On 9/6/2022 6:30 PM, Morgan Smith wrote:
> I've attached my patch V2.
>
> This restores some more of the original logic that I have now realized
> was indeed necessary. Again, this patch should not actually change
> anything with respect to program logic, flow, or user experience. In my
> limited testing, it seems to act just as it did before (but
> significantly more performant).
Thanks, this does indeed seem a lot faster. I do notice one small change
in behavior though: after starting Eshell with the em-smart module
loaded, run "echo hi" and then split the window vertically with 'C-x 2'.
Before the patch, the window doesn't scroll. After the patch, the window
is scrolled so that the "echo hi" block is at the top (i.e. the "Welcome
to the Emacs shell" banner is hidden).
The same sort of thing happens if you run a command with a lot of output
first and then run "echo hi". Before splitting, the "echo hi" block is
at the bottom. Without this patch, 'C-x 2' maintains that position (i.e.
it scrolls the minimum amount of the previous command out of view so
that you can see the most-recent command). With the patch, 'C-x 2'
scrolls *all* of the previous command out of view, so the "echo hi"
block is at the top.
I think the pre-patch behavior is the most-usable: if you can fit all of
the last command in the window, it should be at the bottom so that you
can see (some of) the previous command.
(I also saw some strange issue with resizing the windows via the mouse,
but I can't reproduce it anymore. If I can figure out how trigger this
reliably, I'll send an update. Sorry that's not very helpful at present...)
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#57367: [PATCH] Speed up em-smart
2022-09-09 4:36 ` Jim Porter
@ 2023-09-06 22:46 ` Stefan Kangas
2023-10-18 15:46 ` bug#57367: [PATCH v3] " Morgan Smith
0 siblings, 1 reply; 12+ messages in thread
From: Stefan Kangas @ 2023-09-06 22:46 UTC (permalink / raw)
To: Jim Porter; +Cc: Morgan Smith, Lars Ingebrigtsen, 57367
Jim Porter <jporterbugs@gmail.com> writes:
> On 9/6/2022 6:30 PM, Morgan Smith wrote:
>> I've attached my patch V2.
>> This restores some more of the original logic that I have now realized
>> was indeed necessary. Again, this patch should not actually change
>> anything with respect to program logic, flow, or user experience. In my
>> limited testing, it seems to act just as it did before (but
>> significantly more performant).
>
> Thanks, this does indeed seem a lot faster. I do notice one small change in
> behavior though: after starting Eshell with the em-smart module loaded, run
> "echo hi" and then split the window vertically with 'C-x 2'. Before the patch,
> the window doesn't scroll. After the patch, the window is scrolled so that the
> "echo hi" block is at the top (i.e. the "Welcome to the Emacs shell" banner is
> hidden).
>
> The same sort of thing happens if you run a command with a lot of output first
> and then run "echo hi". Before splitting, the "echo hi" block is at the
> bottom. Without this patch, 'C-x 2' maintains that position (i.e. it scrolls the
> minimum amount of the previous command out of view so that you can see the
> most-recent command). With the patch, 'C-x 2' scrolls *all* of the previous
> command out of view, so the "echo hi" block is at the top.
>
> I think the pre-patch behavior is the most-usable: if you can fit all of the
> last command in the window, it should be at the bottom so that you can see (some
> of) the previous command.
>
> (I also saw some strange issue with resizing the windows via the mouse, but I
> can't reproduce it anymore. If I can figure out how trigger this reliably, I'll
> send an update. Sorry that's not very helpful at present...)
That was one year ago.
Morgan, did you get a chance to look into the issues Jim pointed out
above?
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#57367: [PATCH v3] Speed up em-smart
2023-09-06 22:46 ` bug#57367: [PATCH] " Stefan Kangas
@ 2023-10-18 15:46 ` Morgan Smith
2023-10-28 22:47 ` Jim Porter
0 siblings, 1 reply; 12+ messages in thread
From: Morgan Smith @ 2023-10-18 15:46 UTC (permalink / raw)
To: Stefan Kangas; +Cc: Jim Porter, Lars Ingebrigtsen, 57367
[-- Attachment #1: Type: text/plain, Size: 2263 bytes --]
Stefan Kangas <stefankangas@gmail.com> writes:
> Jim Porter <jporterbugs@gmail.com> writes:
>
>> On 9/6/2022 6:30 PM, Morgan Smith wrote:
>>> I've attached my patch V2.
>>> This restores some more of the original logic that I have now realized
>>> was indeed necessary. Again, this patch should not actually change
>>> anything with respect to program logic, flow, or user experience. In my
>>> limited testing, it seems to act just as it did before (but
>>> significantly more performant).
>>
>> Thanks, this does indeed seem a lot faster. I do notice one small change in
>> behavior though: after starting Eshell with the em-smart module loaded, run
>> "echo hi" and then split the window vertically with 'C-x 2'. Before the patch,
>> the window doesn't scroll. After the patch, the window is scrolled so that the
>> "echo hi" block is at the top (i.e. the "Welcome to the Emacs shell" banner is
>> hidden).
>>
>> The same sort of thing happens if you run a command with a lot of output first
>> and then run "echo hi". Before splitting, the "echo hi" block is at the
>> bottom. Without this patch, 'C-x 2' maintains that position (i.e. it scrolls the
>> minimum amount of the previous command out of view so that you can see the
>> most-recent command). With the patch, 'C-x 2' scrolls *all* of the previous
>> command out of view, so the "echo hi" block is at the top.
>>
>> I think the pre-patch behavior is the most-usable: if you can fit all of the
>> last command in the window, it should be at the bottom so that you can see (some
>> of) the previous command.
>>
>> (I also saw some strange issue with resizing the windows via the mouse, but I
>> can't reproduce it anymore. If I can figure out how trigger this reliably, I'll
>> send an update. Sorry that's not very helpful at present...)
>
> That was one year ago.
>
> Morgan, did you get a chance to look into the issues Jim pointed out
> above?
Thanks for the ping. Reading the documentation for
`window-configuration-change-hook' I found out I can run the scroll
command only on updated windows. Furthermore, that hook selects the
window which is nice. I believe the problems pointed out above stem
from using `(point)` to scroll a window that wasn't actually selected.
Anyways here is V3
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v3-0001-Speed-up-em-smart.patch --]
[-- Type: text/x-patch, Size: 6493 bytes --]
From 801bcf3fb761f9f32423b2c46e7e1b68a2c2ecdc Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Tue, 6 Sep 2022 21:18:51 -0400
Subject: [PATCH v3] Speed up em-smart
em-smart was forcibly re-displaying the screen upwards of 500 times
per screen of output. This caused the eshell to feel quite slow when
the module was in use. By using fewer hooks and never explicitly
calling `redisplay` (which was unnecessary) the performance issues go
away.
lisp/eshell/em-smart.el:
(em-smart-unload-hook, eshell-smart-unload-hook): Remove
`eshell-smart-scroll' instead of the now deleted
`eshell-refresh-windows'
(eshell-smart-displayed, eshell-currently-handling-window,
eshell-refresh-windows): Delete
(eshell-smart-scroll-window): Rename to `eshell-smart-scroll-windows'
and add a bunch of logic originally from `eshell-refresh-windows'
(eshell-smart-initialize): Don't add a hook onto
`window-scroll-functions'. Replace `eshell-refresh-windows' with
`eshell-smart-scroll-windows'
(eshell-smart-display-setup): Don't refresh windows
(eshell-smart-redisplay): Rename to `eshell-smart-scroll'. Delete
'eobp' case.
---
lisp/eshell/em-smart.el | 83 +++++++++++++++--------------------------
1 file changed, 30 insertions(+), 53 deletions(-)
diff --git a/lisp/eshell/em-smart.el b/lisp/eshell/em-smart.el
index 4c39a991ec6..f34a9ec0309 100644
--- a/lisp/eshell/em-smart.el
+++ b/lisp/eshell/em-smart.el
@@ -95,7 +95,7 @@ eshell-smart-unload-hook
(list
(lambda ()
(remove-hook 'window-configuration-change-hook
- 'eshell-refresh-windows)))
+ 'eshell-smart-scroll)))
"A hook that gets run when `eshell-smart' is unloaded."
:type 'hook
:group 'eshell-smart)
@@ -159,9 +159,7 @@ eshell-where-to-jump
;;; Internal Variables:
-(defvar eshell-smart-displayed nil)
(defvar eshell-smart-command-done nil)
-(defvar eshell-currently-handling-window nil)
;;; Functions:
@@ -174,10 +172,9 @@ eshell-smart-initialize
(setq-local eshell-scroll-to-bottom-on-input nil)
(setq-local eshell-scroll-show-maximum-output t)
- (add-hook 'window-scroll-functions 'eshell-smart-scroll-window nil t)
- (add-hook 'window-configuration-change-hook 'eshell-refresh-windows)
+ (add-hook 'window-configuration-change-hook 'eshell-smart-scroll nil t)
- (add-hook 'eshell-output-filter-functions 'eshell-refresh-windows t t)
+ (add-hook 'eshell-output-filter-functions 'eshell-smart-scroll-windows 90 t)
(add-hook 'after-change-functions 'eshell-disable-after-change nil t)
@@ -193,28 +190,15 @@ eshell-smart-initialize
(add-hook 'eshell-post-command-hook
'eshell-smart-maybe-jump-to-end nil t))))
-;; This is called by window-scroll-functions with two arguments.
-(defun eshell-smart-scroll-window (wind _start)
- "Scroll the given Eshell window WIND accordingly."
- (unless eshell-currently-handling-window
- (let ((eshell-currently-handling-window t))
- (with-selected-window wind
- (eshell-smart-redisplay)))))
-
-(defun eshell-refresh-windows (&optional frame)
- "Refresh all visible Eshell buffers."
- (let (affected)
- (walk-windows
- (lambda (wind)
- (with-current-buffer (window-buffer wind)
- (if eshell-mode
- (let (window-scroll-functions) ;;FIXME: Why?
- (eshell-smart-scroll-window wind (window-start))
- (setq affected t)))))
- 0 frame)
- (if affected
- (let (window-scroll-functions) ;;FIXME: Why?
- (redisplay)))))
+(defun eshell-smart-scroll-windows ()
+ "Scroll all eshell windows to display as much output as possible, smartly."
+ (walk-windows
+ (lambda (wind)
+ (with-current-buffer (window-buffer wind)
+ (if eshell-mode
+ (with-selected-window wind
+ (eshell-smart-scroll)))))
+ 0 t))
(defun eshell-smart-display-setup ()
"Set the point to somewhere in the beginning of the last command."
@@ -231,8 +215,7 @@ eshell-smart-display-setup
(t
(error "Invalid value for `eshell-where-to-jump'")))
(setq eshell-smart-command-done nil)
- (add-hook 'pre-command-hook 'eshell-smart-display-move nil t)
- (eshell-refresh-windows))
+ (add-hook 'pre-command-hook 'eshell-smart-display-move nil t))
;; Called from after-change-functions with 3 arguments.
(defun eshell-disable-after-change (_b _e _l)
@@ -254,28 +237,22 @@ eshell-smart-maybe-jump-to-end
(goto-char (point-max))
(remove-hook 'pre-command-hook 'eshell-smart-display-move t)))
-(defun eshell-smart-redisplay ()
- "Display as much output as possible, smartly."
- (if (eobp)
- (save-excursion
- (recenter -1)
- ;; trigger the redisplay now, so that we catch any attempted
- ;; point motion; this is to cover for a redisplay bug
- (redisplay))
- (let ((top-point (point)))
- (and (memq 'eshell-smart-display-move pre-command-hook)
- (>= (point) eshell-last-input-start)
- (< (point) eshell-last-input-end)
- (set-window-start (selected-window)
- (line-beginning-position) t))
- (if (pos-visible-in-window-p (point-max))
- (save-excursion
- (goto-char (point-max))
- (recenter -1)
- (unless (pos-visible-in-window-p top-point)
- (goto-char top-point)
- (set-window-start (selected-window)
- (line-beginning-position) t)))))))
+(defun eshell-smart-scroll ()
+ "Scroll WINDOW to display as much output as possible, smartly."
+ (let ((top-point (point)))
+ (and (memq 'eshell-smart-display-move pre-command-hook)
+ (>= (point) eshell-last-input-start)
+ (< (point) eshell-last-input-end)
+ (set-window-start (selected-window)
+ (pos-bol) t))
+ (if (pos-visible-in-window-p (point-max) (selected-window))
+ (save-excursion
+ (goto-char (point-max))
+ (recenter -1)
+ (unless (pos-visible-in-window-p top-point (selected-window))
+ (goto-char top-point)
+ (set-window-start (selected-window)
+ (pos-bol) t))))))
(defun eshell-smart-goto-end ()
"Like `end-of-buffer', but do not push a mark."
@@ -323,7 +300,7 @@ eshell-smart-display-move
(remove-hook 'pre-command-hook 'eshell-smart-display-move t))))
(defun em-smart-unload-hook ()
- (remove-hook 'window-configuration-change-hook #'eshell-refresh-windows))
+ (remove-hook 'window-configuration-change-hook #'eshell-smart-scroll))
(provide 'em-smart)
--
2.41.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#57367: [PATCH v3] Speed up em-smart
2023-10-18 15:46 ` bug#57367: [PATCH v3] " Morgan Smith
@ 2023-10-28 22:47 ` Jim Porter
0 siblings, 0 replies; 12+ messages in thread
From: Jim Porter @ 2023-10-28 22:47 UTC (permalink / raw)
To: Morgan Smith, Stefan Kangas; +Cc: Lars Ingebrigtsen, 57367-done
Version: 30.1
On 10/18/2023 8:46 AM, Morgan Smith wrote:
> Thanks for the ping. Reading the documentation for
> `window-configuration-change-hook' I found out I can run the scroll
> command only on updated windows. Furthermore, that hook selects the
> window which is nice. I believe the problems pointed out above stem
> from using `(point)` to scroll a window that wasn't actually selected.
>
> Anyways here is V3
Thanks for the updated patch. As far as I can tell (I don't use the
smart display module in Eshell), everything works here, and you even
fixed a bug I noticed on master with long output! (Previously, if you
ran a command with a lot of output, the prompt got hidden for some
reason, probably due to using field properties for the prompt/output.
That's fixed now.)
I've merged this to master as e08238cdd74 now. Closing this bug. Thanks
again.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-10-28 22:47 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-23 20:06 bug#57367: [PATCH] Speed up em-smart Morgan Smith
2022-09-04 21:56 ` Lars Ingebrigtsen
2022-09-05 19:01 ` Jim Porter
2022-09-05 21:48 ` Morgan Smith
2022-09-05 21:51 ` Lars Ingebrigtsen
[not found] ` <DM5PR03MB31639CBC75F62622AC4E70ABC57E9@DM5PR03MB3163.namprd03.prod.outlook.com>
2022-09-06 10:00 ` Lars Ingebrigtsen
2022-09-07 1:30 ` bug#57367: [PATCH V2] " Morgan Smith
2022-09-07 12:55 ` Lars Ingebrigtsen
2022-09-09 4:36 ` Jim Porter
2023-09-06 22:46 ` bug#57367: [PATCH] " Stefan Kangas
2023-10-18 15:46 ` bug#57367: [PATCH v3] " Morgan Smith
2023-10-28 22:47 ` Jim Porter
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).