fix: mini-frame is slow in Windows #9

kiennq · 2020-04-27T07:19:11Z

Using mini-frame under Windows is slow, mostly due to it has to create new child frame every time mini-frame is invoked.
When I read the FIXME, it seems that in Windows, child frame can easily being display under the current frame, thus make the child frame not displayable problem.
In this PR I've tried to tackle this, produced quite contended result for Windows.

Add several more frame parameters like z-group to default mini-frame-frame parameters
In mini-frame-read-from-minibuffer, unify the condition of last two conditions, since both of them requires calling to (mini-frame--display fn args). And that function requires properly set up of hooks, variables and cleanups.
Hide mini-frame if visible before displaying it in Windows. This is done to make sure mini-frame restart its z order and will not be hidden under parent frame.
Reassign parent frame to nil when hiding mini frames. This is done so even if parent frame are dead, child frame will not be destroyed. It will be reassign to new parent when invoked anyway.
Try to find a root frame when return from mini frame, incase parent frame are invalid.

The change log may seems a lot, but all is done to ensure mini frame on Windows can be displayed quickly.
I've tested it with swiper, that seems to make mini frame goes to weird state every time we type + not select the result. With this fix the issue is resolved.

muffinmad

Thanks! I've left a few comments that should be addresses before this gets merged.

muffinmad · 2020-04-27T19:55:43Z

mini-frame.el

-   ((and (frame-live-p mini-frame-frame)
-         (frame-visible-p mini-frame-frame))
-    (mini-frame--display fn args))


It's here for the case when enable-recursive-minibuffers is t.

M-x

C-x 5 o

M-x

C-g

Focus should stay in mini-frame. With this changes mini-frame is closed.

But why this block must be removed at all? Is this Windows specific? Can you please provide receipt for issue that this removal is fixing?

What this block doing is just call bare mini-frame--display without any additional setup + cleanup.
That can be quite dangerous.

For example when I tried it on Windows, once the minibuffer some how got into visible state, and the focus is removed. Since we never run the cleanup code for it, it will just stays visible there forever.
This could be true even for other platform too.

What this block doing is just call bare mini-frame--display without any additional setup + cleanup.
That can be quite dangerous.

But mini-frame is visible. This mean that cleanup is set already and all what we want frommini-frame--display is maybe to update size and position of mini-frame.

For example when I tried it on Windows, once the minibuffer some how got into visible state, and the focus is removed. Since we never run the cleanup code for it, it will just stays visible there forever.
This could be true even for other platform too.

I'll check it on Windows.

But mini-frame is visible. This mean that cleanup is set already and all what we want frommini-frame--display is maybe to update size and position of mini-frame.

Let me clarify it more.
I found out one of the reason in Windows (or it may affect other GUI system) that some time the mini frame may get behind the main frame.
In that case the mini frame still show up as visible but user cannot see it.
Worse, I observe that when I'm using swiper, once the mini frame got behind main frame, the user will not be able to move away from mini frame without using mouse.
The reason may be because the mini frame is still visible (actually hidden since it's behind the main frame), so when we try to quit it, there is no cleanup code to be run to reassign the input focus.

To recover from the issue of mini frame get behind main frame, one easy solution is to just hide mini frame every time we enter it (except recursive entering). That will make sure we start it with a fresh z-order once it went wrong. However, because of that, we will need the frame cleanup code for the your mentioned block, since basically we're going to show the mini frame again.

Worse, I observe that when I'm using swiper, once the mini frame got behind main frame, the user will not be able to move away from mini frame without using mouse.

C-x 5 0 may help.

Can confirm. When ivy-mode is active or after M-x swiper mini-frame has visibility frame parameter set to t but it's not visible, e.g.:

M-x swiper

C-g

M-: (frame-parameter mini-frame-frame 'visibility)

I'll look further.

muffinmad · 2020-04-27T19:58:28Z

mini-frame.el

-          (select-frame-set-input-focus mini-frame-selected-frame))
+          (make-frame-invisible mini-frame-completions-frame)
+          (modify-frame-parameters mini-frame-completions-frame '((parent-frame))))
+        (if (frame-live-p mini-frame-selected-frame)


I wonder if the mini-frame-selected-frame variable is really needed. Maybe set focus to (frame-parameter mini-frame-frame 'parent-frame) will be enough.

Yep, I've removed all of mini-frame-selected-frame now.

mini-frame.el

muffinmad · 2020-04-27T20:08:53Z

mini-frame.el

+          ;; (if (eq system-type 'windows-nt)
+          ;; FIXME sometime buffer is not visible on windows
+          ;; (delete-frame mini-frame-frame)


IMO no need to keep commented code.

muffinmad · 2020-04-28T08:59:09Z

mini-frame.el

+          (when (frame-live-p mini-frame-completions-frame)
+            (make-frame-invisible mini-frame-completions-frame)
+            (modify-frame-parameters mini-frame-completions-frame '((parent-frame))))
+          (if (frame-parameter mini-frame-frame 'parent-frame)


In which case parent-frame parameter of mini-frame will be nil here?

As in the description, when I hide the mini-frame, I also detach its parent from it.
The purpose of that is to make sure mini-frame is not being destroyed when parent frame is gone.
For example, you open another frame M-x make-frame or using emacsclient to connect to server. At that time, if mini-frame is used inside the new frame, and you close the new frame after that, mini-frame will be gone with the new frame.
With this change, it's prevented, so we don't need to create new mini-frame again

I understand that.

But I mean this particular place (line 319). We are only going to hide mini-frame, so it is visible and it's parent-frame is not nil, right?

I see, Since this part is in unwind form, I assume anything may happens.
For example, the mini-frame is dead, thus the frame-parent parameter is returned as nil.
In that case it will go to else code bellow and try to find another root frame to set focus to. So we don't accidently lost the input focus in some unreachable place.

muffinmad · 2020-04-28T14:13:57Z

mini-frame.el

-   ((and (frame-live-p mini-frame-frame)
-         (frame-visible-p mini-frame-frame))
-    (mini-frame--display fn args))


What this block doing is just call bare mini-frame--display without any additional setup + cleanup.
That can be quite dangerous.

But mini-frame is visible. This mean that cleanup is set already and all what we want frommini-frame--display is maybe to update size and position of mini-frame.

For example when I tried it on Windows, once the minibuffer some how got into visible state, and the focus is removed. Since we never run the cleanup code for it, it will just stays visible there forever.
This could be true even for other platform too.

I'll check it on Windows.

muffinmad · 2020-04-28T14:15:13Z

mini-frame.el

+            ;; trying to find an alternate frame to return the focus
+            (when-let (fr (cl-find-if-not
+                           (lambda (fr)
+                             (or (equal fr mini-frame-completions-frame)
+                                 (equal fr mini-frame-frame)
+                                 (frame-parameter fr 'parent-frame)))
+                           (frame-list)))
+              (select-frame-set-input-focus fr)))


Can (other-frame) be used here to simplify the code?

Since we're going to detach mini-frame-completions-frame and mini-frame-frame to nil parent, (other-frame) may just return them back. So I need this to find the first frame that's not the mini frame.

kiennq · 2020-08-30T07:09:13Z

@muffinmad Is there any chance this can be merged? mini-frame-mode is quite slow on Windows

muffinmad · 2020-08-31T06:53:22Z

@kiennq Sorry for delay. Haven't enough time lately to find out the reason of such behavior on Windows, but your PR is on my list.

muffinmad · 2020-09-26T13:32:39Z

@kiennq Please check recent master version. Mini-frame is not deleted on Windows now.

kiennq · 2020-09-28T15:22:48Z

When I update mini-frame and try to use it, I encountered this problem

mini-frame--display: Cannot change the border width of a frame [2 times]

muffinmad · 2020-09-28T16:13:05Z

When I update mini-frame and try to use it, I encountered this problem
mini-frame--display: Cannot change the border width of a frame [2 times]

To reproduce the same error:

emacs -Q
M-: (modify-frame-parameters nil '((border-width . 5)))

Solutions:

Turn on the mini-frame-create-lazy option.
Use the internal-border-width parameter instead of border-width in mini-frame-show-parameters.

kiennq force-pushed the bug/win-slow branch 2 times, most recently from a3687e3 to e4fc024 Compare April 27, 2020 12:32

muffinmad reviewed Apr 27, 2020

View reviewed changes

kiennq force-pushed the bug/win-slow branch 2 times, most recently from 36151c3 to 117bc09 Compare April 28, 2020 03:14

muffinmad reviewed Apr 28, 2020

View reviewed changes

kiennq force-pushed the bug/win-slow branch from 117bc09 to f4cb9ae Compare April 30, 2020 07:48

fix: mini-frame is slow in Windows

b484d45

kiennq force-pushed the bug/win-slow branch from f4cb9ae to b484d45 Compare August 30, 2020 07:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: mini-frame is slow in Windows #9

fix: mini-frame is slow in Windows #9

kiennq commented Apr 27, 2020 •

edited

Loading

muffinmad left a comment

muffinmad Apr 27, 2020

kiennq Apr 28, 2020

muffinmad Apr 28, 2020

kiennq Apr 28, 2020

muffinmad Apr 28, 2020

kiennq Apr 28, 2020

muffinmad Apr 28, 2020

muffinmad Apr 27, 2020

kiennq Apr 28, 2020

muffinmad Apr 27, 2020

kiennq Apr 28, 2020

muffinmad Apr 28, 2020

kiennq Apr 28, 2020

muffinmad Apr 28, 2020

kiennq Apr 28, 2020

muffinmad Apr 28, 2020

muffinmad Apr 28, 2020

kiennq Apr 28, 2020

kiennq commented Aug 30, 2020

muffinmad commented Aug 31, 2020

muffinmad commented Sep 26, 2020

kiennq commented Sep 28, 2020

muffinmad commented Sep 28, 2020

fix: mini-frame is slow in Windows #9

Are you sure you want to change the base?

fix: mini-frame is slow in Windows #9

Conversation

kiennq commented Apr 27, 2020 • edited Loading

muffinmad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiennq commented Aug 30, 2020

muffinmad commented Aug 31, 2020

muffinmad commented Sep 26, 2020

kiennq commented Sep 28, 2020

muffinmad commented Sep 28, 2020

kiennq commented Apr 27, 2020 •

edited

Loading