Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Early-out of resize handling if the window has not been presented yet. #5

Merged
merged 1 commit into from Apr 27, 2015
Merged

Early-out of resize handling if the window has not been presented yet. #5

merged 1 commit into from Apr 27, 2015

Conversation

jpetrie
Copy link
Contributor

@jpetrie jpetrie commented Apr 15, 2015

This change address an issue I noticed on OS 10.10.3 when MacVim starts up and tries to restore a saved window position to a location on a secondary monitor.

When that happens, MacVim's attempt to set the window's position before it actually becomes visible will trigger a resize event (because the window moved from one NSScreen to another), even though the actual size of the window frame doesn't change. This resize event occurs too early, before the window's size constraints based on the initial rows/columns setting from Vim have been correctly set up. This causes the window to appear at the proper size briefly and then immediate shrink to a size based on the window's default, small frame.

By ensuring that the window controller does not propagate logic from the resize event until the window has been fully presented, the issue is avoided.

I am fairly certain this is a safe, clean way to accomplish this. Moving around where setupDone is set to YES was another approach, but given the number of checks against setupDone in the controller I thought it might be too risky. It is, however, the first change I've attempted to make in MacVim's source so I could be missing a critical piece of logic.

My testing hasn't revealed any issues so far. I've run MacVim with my current set of RC files, as well as via mvim -u NONE and mvim -u NONE -c 'set columns=X' and so on (in both cases, with and without nuking the autosaved window size user defaults). Everything appears normal.

@jasonseney
Copy link

Haven't tested this yet, but wanted to drop a note that I can replicate this issue reliably under the same circumstances mentioned by @jpetrie. Thank you for tracking this down and putting out the pull request, hope we can get some 👀 on this!

@reshleman
Copy link

This has been driving me crazy, and I hadn't been able to identify the secondary display as a cause until I found this thread (so thanks, @jpetrie!). I can also replicate this, given the information here.

(FWIW, I've also tried manually setting the window size in .gvimrc with, i.e., set lines=40 columns=80 without success.)

If I have some time today, I'll try to pull this patch down today and test it out.

@reshleman
Copy link

If I have some time today, I'll try to pull this patch down today and test it out.

This patch does indeed resolve the issue for me. I haven't encountered any other problems with this change, but I'll report back here if I notice anything amiss.

Thanks, @jpetrie!

@jasonseney
Copy link

Confirmed that this this works for me after pulling down the branch and building/testing. Looking forward to the fix getting in, thanks again!

douglasdrumond added a commit that referenced this pull request Apr 27, 2015
Early-out of resize handling if the window has not been presented yet.
@douglasdrumond douglasdrumond merged commit c69fd43 into macvim-dev:master Apr 27, 2015
@chdiza
Copy link
Contributor

chdiza commented Apr 27, 2015

This seems to have caused an ugly white flash inside the MacVim window's rectangle upon first doing Cmd-N, or doing mvim foo at the terminal. I'm on 10.10.3.

@chdiza
Copy link
Contributor

chdiza commented Apr 27, 2015

This is really visible if your colorscheme (in .gvimrc) has a dark background.

@jpetrie
Copy link
Contributor Author

jpetrie commented Apr 27, 2015

Thanks for the heads-up. I'll take a look this evening and see if I can track it down.

@chdiza
Copy link
Contributor

chdiza commented Apr 27, 2015

I've bisected to verify that that commit is the culprit, and it is.

Also, I have reproduced this on a separate machine.

@chdiza
Copy link
Contributor

chdiza commented Apr 27, 2015

I can get you screenshots later tonight or tomorrow.

This white flash is MEGA annoying, as you'll see if I can figure out how to capture video.

@douglasdrumond
Copy link
Member

I reproduced it at work. I'll look into it tomorrow.

@ElDeveloper
Copy link

@chdiza, you may want to use LICEcap http://www.cockos.com/licecap/ it
will produce a GIF that can be added to your GitHub comment.

On (Apr-27-15| 8:07), chdiza wrote:

I can get you screenshots later tonight or tomorrow.

This white flash is MEGA annoying, as you'll see if I can figure out how to capture video.


Reply to this email directly or view it on GitHub:
#5 (comment)

@jasonseney
Copy link

I do see a bit of a flash as the window is opening, although for me it's not terribly annoying. I've attached a screenshot and GIF of the issue. @chdiza is this what you're seeing?
2015-04-27 at 11 58 am
2015-04-27 12_02_17

@jpetrie
Copy link
Contributor Author

jpetrie commented Apr 27, 2015

That looks super gross. :(

It generates an interesting pattern of correct and incorrect window background, though.

Probably since my change prevents the resize logic from executing that early, until the window is fully set up, it's also preventing the preparation of the window's background colors based on the Vim backend. My hunch is that those two updates may need to be divorced from eachother somewhat to allow them to happen independently, although I might have to go back to the drawing board with how I implemented the initial fix.

@chdiza
Copy link
Contributor

chdiza commented Apr 27, 2015

Yeah, that's it, and it's unbearable. Up close and in person, it's like a camera flash going off, especially if the desktop wallpaper (or other nearby windows) are on the dark side of things.

@douglasdrumond I'd like to ask that this commit be reverted until we know what to do.

@chdiza
Copy link
Contributor

chdiza commented Apr 27, 2015

Oh man, it gets better :)

Pipe something into mvim as stdin, e.g., $ foo bar | mvim -, where the time for foo bar to complete is less than instantaneous. Then, the big ugly white space sits there until mvim has taken in all of the pipe.

For (non)fun: ls -R /| mvim -.

I pipe stuff into into mvim all the time :(

@jpetrie
Copy link
Contributor Author

jpetrie commented Apr 28, 2015

The windowDidResize case where !windowPresented is true executes twice. The first comes from the setFrameTopLeftPoint call at MMAppController.m:798; that's the one that's bad and which we want to avoid. The second one comes from MMWindowController.m:1340 and appears to be the one that we need to have executed in order to correctly get the opening frame set correctly (and thus have correct background colors).

Since the crux of the issue is moving a window between monitors, I tried something a bit more invasive: adding a method to the window controller to do the move. This can track sufficient state to skip the resize code if we're actually intending only to move the window.

This works; both the original problem and the one @chdiza reported are gone. But it's a bit ugly. There's already a "setTopLeft" in the window controller (can't be used in this instance however), and the method itself is a bit "dangerous" in the sense that if somebody ever uses it to move a window without moving that window to a different monitor, the next resize event will be skipped. I'm also accepting suggestions for a sane name for such an abomination. "moveWindowAcrossScreens" is what I've currently got.

@eirnym
Copy link
Contributor

eirnym commented Apr 28, 2015

@chdiza you can do find / -print0 | xargs -0 mvim to avoid space and names like Icon\n problems

@chdiza
Copy link
Contributor

chdiza commented Apr 28, 2015

@eirnym I'm not having any such problems. The issue is with how MacVim looks while waiting, not with the eventual buffer contents.

chdiza referenced this pull request Aug 23, 2015
Regression of "Early-out of resize handling if the window has not been presented yet…"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants