Skip to content

Fix native fullscreen rendering and resizing bugs #745

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

Merged
merged 3 commits into from
Nov 2, 2018

Conversation

ychin
Copy link
Member

@ychin ychin commented Sep 15, 2018

This fixes 3 issues related to native fullscreen rendering and resizing by utilizing the guioptions-k (#731) code by reducing unnecessary resizing. Copied from the commit messages:

Fixed native fullscreen rendering corruption bug

Fix the issue where MacVim would occasionally draws corrupted image in fullscreen (it would draw mostly black).

The easiest way to reproduce this is as follows:

  1. Make a new MacVim window, enter fullscreen
  2. Open a new tab or hit Cmd-= a few times
  3. Switch to another fullscreen app or desktop, click around, then
    switch back
  4. Observe most of the screen is black.

The reason this happens is that the MacVim resize code always tries to resize the window to fit the content size (calculated from how many lines / columns we have and whether other elements like tab bar are visible). This means the resize code (resizeWindowToFitContentSize:keepOnScreen:) would make the window smaller than the full size of screen. For some reason, when you switch away from the space, macOS decides to resize the window back to screen size again, causing a window resize event to happen. The resize event invalidates the NSView, causing it to draw black.

This is also why fullscreen mode has black bars on top / bottom, which is especially jarring when font size is large of linespace is high.

The fix is to treat guioptions 'k' to be on when in full screen mode, since the option means we will always try to fit the Vim content inside the window, rather than resize the window to fit the Vim content. This way the fullscreen Vim window will take up the whole screen and won't keep getting resized. This is also more similar to how native Gvim works when maximized.

Close #496 (black bars)

Close #557, close #674 (full screen rendering issues)

A related issue is that MacVim (without CGLayer backing) doesn't actually know how to redraw itself properly when invalidated, which is the root cause of this bug. It receives Vim draw calls incrementally and doesn't actually cache the rendered content, so it relies on the fact that MacVim's NSWindow doesn't usually invalidates all the content which allows it to draw incrementally without needing to perform a full redraw. This is why non-native fullscreen requires CGLayer backing mode as macOS's behavior in this mode (basically a borderless window) is that it does clear the NSWindow's content when setWantsDisplay: is called. This is also why Vim live window resizing is limited to cell size instead of allowing smooth resize (to avoid having to trigger redraws). These are issues that should be fixed later.

Fix toggling native fullscreen on/off to restore window to correct size

Previously when using native fullscreen mode, if you toggle fullscreen off, the window will end up taking up the whole screen rather than restoring back to the original size. Fix that.

The real issue is because when you resize MacVim's window (which the fullscreen restore code does), then callback (windowDidResize) triggered a complicated set of callbacks by calling setFrameSize:, which in turn resizes Vim, which in turn calls windowDidResize again, which usually does the right thing, but not always. Fix the window resize handler code to always respect the new window size by calling setFrameSizeKeepGUISize: instead which doesn't resize the window.

Edit: Close #497, per #745 (comment)

Fixed double-clicking on border resulting in bad resize

Fix issue where if the user double-clicks on the window border or corner to resize MacVim (the macOS behavior is to resize the window all the way to the screen's border), it results in an incomplete resize and also takes a long time.

The code was spamming the Vim instance with live resize messages, leading to slowdown and dropped messages. Fix it by rate limiting the messages to one at a time, which speeds things up, and clean up when live resize finishes to make sure things look right.

@ychin
Copy link
Member Author

ychin commented Sep 15, 2018

I believe this also fix #497 as a result of the second commit (58381df). It's kind of an indirect effect of it. When an external program resizes MacVim, it usually resizes it to some random size, rather than to a fixed multiple of row/column size. windowDidResize then used to attempt to resize it down to fixed multiples by calling setFrameSize. This small window size change invalidates the frame's content, but Vim doesn't know it needs to redraw itself since the number of rows/columns remain the same.

With this new change windowDidResize won't try to change the size of the window and therefore there will be no invalidation.

I tested this with Divvy and I did reproduce the bug described in #497 before my change, but with my change I can no longer reproduce the black screen.

Note: I think it's worth thinking about a better rendering solution in the long term that can handle invalidation. I may write up something. Currently I think the best bet is to implement a Metal renderer which will allow us to keep having good scrolling performance, as the CGLayer implementation results in laggy scrolling performance due to the fact that it's not GPU accelerated.

@chdiza
Copy link
Contributor

chdiza commented Sep 15, 2018

Is any of this going to affect NON-native fullscreen mode?

(I'm going to build with this PR and experiment, but I just want to know ahead of time if such effects are even theoretically plausible.)

@eirnym
Copy link
Contributor

eirnym commented Sep 15, 2018

I'll check this if it solves my issue as it looks pretty similar to full-screen problems

@ychin
Copy link
Member Author

ychin commented Sep 15, 2018

@chdiza Unfortunately, no, this change does not fix non-native fullscreen mode. I was planning to write up something to describe the details for the rendering issues more, but basically non-native fullscreen mode is a little trickier to fix and likely requires rewriting the rendering code (or at least some refactoring). As you probably know it currently automatically uses CGLayer mode (as if MMUserCGLayerAlways is turned on) which leads to scrolling performance issues and other problems.

I'll need to think a little more about how to fix it.

@chdiza
Copy link
Contributor

chdiza commented Sep 15, 2018

@ychin I was actually hoping that this PR does not touch non-native fullscreen mode. So it's good news that it should not.

FWIW, I have never, in ten years, experienced scrolling performance issues with it. I've only had problems with incorrect resizes when exiting NNFS and artifacts when changing the font size while in NNFS. But you're right, these are separate worries and should be dealt with separately.

@ychin
Copy link
Member Author

ychin commented Sep 15, 2018

@chdiza It's pretty easy to reproduce the scrolling performance issues in non-native fullscreen mode. Just use a retina MacBook (or a Mac with HiDPI / high resolution displays) with a trackpad and scroll down quickly with a flick, then scroll up. Or just scroll up/down rapidly and you will notice the lag and general jitteriness compared to windowed/native-fullscreen mode.

But if you are fine with it already that's fine too. This PR doesn't change that mode.

Edit: #312, #665, #499 all have discussions about the scrolling performance issue.

@chdiza
Copy link
Contributor

chdiza commented Sep 15, 2018

Huh. I don't have a retina display or a fancy one of any kind.

@eirnym
Copy link
Contributor

eirnym commented Sep 26, 2018

@ychin Could you add a trigger to check if column/line count has changed by setting it directly with :set command? This happening when MMUseCGLayerAlways is 1.

Other resizing issues I've had are gone by this patch.

@ychin
Copy link
Member Author

ychin commented Sep 27, 2018

@eirnym I think you are talking about #732? I prefer just waiting until a later PR to address that, as this PR is already a little bloated with different commits and has been sitting a while so I kind of want this to be merged first.

Also, just curious, why are you using MMUseCGLayerAlways? Unless you are running into Mojave issues, you shouldn't need to set it anymore after this patch as it fixes a lot of the black screen issues.

For the long term solution for Mojave, my own opinion is we should deprecate MMUseCGLayerAlways and implement a new GPU-accelerated version (CGLayer isn't really GPU accelerated) using Metal.

@eirnym
Copy link
Contributor

eirnym commented Sep 27, 2018

@ychin yes, I've just installed Mojave as I waited for many changes in macOS itself.

I like an idea of GPU accelerated backend in general and would like to participate in the process to bring it into MacVim.

As per #732 and my wish… this only happeing when I set columns or lines by :set only. I've found a workaround of it: autocmd VimResized * redraw!, but I'd like to make it as an integrated solution if possible. If it's not possible to do with this PR, I'm satisfied with your solution and put my LGTM on it.

@chdiza
Copy link
Contributor

chdiza commented Oct 9, 2018

I've been using this patch for the last 3 weeks or so, and I haven't yet come across any bad side-effects for non-native fullscreen mode.

ychin added 3 commits November 1, 2018 01:06
Fix the issue where MacVim would occasionally draws corrupted image in
fullscreen (it would draw mostly black).

The easiest way to reproduce this is as follows:

1. Make a new MacVim window, enter fullscreen
2. Open a new tab or hit Cmd-= a few times
3. Switch to another fullscreen app or desktop, click around, then
   switch back
4. Observe most of the screen is black.

The reason this happens is that the MacVim resize code always tries to
resize the window to fit the content size (calculated from how many
lines / columns we have and whether other elements like tab bar are
visible).  This means the resize code
(resizeWindowToFitContentSize:keepOnScreen:) would make the window
smaller than the full size of screen. For some reason, when you switch
away from the space, macOS decides to resize the window back to screen
size again, causing a window resize event to happen. The resize event
invalidates the NSView, causing it to draw black.

This is also why fullscreen mode has black bars on top / bottom, which
is especially jarring when font size is large of `linespace` is high.

The fix is to treat guioptions 'k' to be on when in full screen mode,
since the option means we will always try to fit the Vim content inside
the window, rather than resize the window to fit the Vim content. This
way the fullscreen Vim window will take up the whole screen and won't
keep getting resized. This is also more similar to how native Gvim works
when maximized.

Close macvim-dev#496 (black bars)

Close macvim-dev#557, close macvim-dev#674 (full screen rendering issues)

A related issue is that MacVim (without CGLayer backing) doesn't
actually know how to redraw itself properly when invalidated, which is
the root cause of this bug. It receives Vim draw calls incrementally and
doesn't actually cache the rendered content, so it relies on the fact
that MacVim's NSWindow doesn't usually invalidates all the content which
allows it to draw incrementally without needing to perform a full
redraw. This is why non-native fullscreen requires CGLayer backing mode
as macOS's behavior in this mode (basically a borderless window) is that
it does clear the NSWindow's content when setWantsDisplay: is called.
This is also why Vim live window resizing is limited to cell size
instead of allowing smooth resize (to avoid having to trigger redraws).
These are issues that should be fixed later.
Previously when using native fullscreen mode, if you toggle fullscreen
off, the window will end up taking up the whole screen rather than
restoring back to the original size. Fix that.

The real issue is because when you resize MacVim's window (which the
fullscreen restore code does), then callback (windowDidResize) triggered
a complicated set of callbacks by calling setFrameSize:, which in turn
resizes Vim, which in turn calls windowDidResize again, which usually
does the right thing, but not always. Fix the window resize handler code
to always respect the new window size by calling
setFrameSizeKeepGUISize: instead which doesn't resize the window.
Fix issue where if the user double-clicks on the window border or corner
to resize MacVim (the macOS behavior is to resize the window all the way
to the screen's border), it results in an incomplete resize and also
takes a long time.

The code was spamming the Vim instance with live resize messages,
leading to slowdown and dropped messages. Fix it by rate limiting the
messages to one at a time, which speeds things up, and clean up when
live resize finishes to make sure things look right.
@ychin ychin merged commit 91aa372 into macvim-dev:master Nov 2, 2018
@ychin ychin deleted the rendering_fixes branch November 2, 2018 05:33
ychin added a commit to ychin/macvim that referenced this pull request Dec 5, 2018
Vim patch 8.1.560

Targets macOS 10.8+

Features:
- macOS Mojave (10.14) is now supported.
    - MacVim's UI now works with Dark Mode.
    - Fixed broken rendering and flickering under Mojave when using the
      default Core Text renderer. macvim-dev#757
- guioption 'k' is supported again. macvim-dev#731
    - This option prevents window from resizing when UI elements such as
      toolbars or tabs show or hide themselves.

Fixes:
- Fixed misc fullscreen and window resizing bugs and artifacts macvim-dev#745
- Dragging tabs to reorder now works properly macvim-dev#789
- Fixed timer callback handling in GUI macvim-dev#749
- Fixed native tabs (10.12+) interring with Vim tabs macvim-dev#788
- Fixed Japanese IME Ctrl-U/Ctrl-O handling macvim-dev#742
- Fixed MMShareFindPboard and Cmd-E/Cmd-G interactions macvim-dev#780
- Better handling of guifontwide font size macvim-dev#737
- Better python discovery in default vimrc macvim-dev#739

Known Issues:
- Scrolling performance is slightly worse under Mojave macvim-dev#796

Script interfaces have compatibility with these versions:
- Lua 5.3
- Perl 5.18
- Python2 2.7
- Python3 3.7
- Ruby 2.5
ychin added a commit that referenced this pull request Dec 5, 2018
Vim patch 8.1.560

Targets macOS 10.8+

Features:
- macOS Mojave (10.14) is now supported.
    - MacVim's UI now works with Dark Mode.
    - Fixed broken rendering and flickering under Mojave when using the
      default Core Text renderer. #757
- guioption 'k' is supported again. #731
    - This option prevents window from resizing when UI elements such as
      toolbars or tabs show or hide themselves.

Fixes:
- Fixed misc fullscreen and window resizing bugs and artifacts #745
- Dragging tabs to reorder now works properly #789
- Fixed timer callback handling in GUI #749
- Fixed native tabs (10.12+) interring with Vim tabs #788
- Fixed Japanese IME Ctrl-U/Ctrl-O handling #742
- Fixed MMShareFindPboard and Cmd-E/Cmd-G interactions #780
- Better handling of guifontwide font size #737
- Better python discovery in default vimrc #739

Known Issues:
- Scrolling performance is slightly worse under Mojave #796

Script interfaces have compatibility with these versions:
- Lua 5.3
- Perl 5.18
- Python2 2.7
- Python3 3.7
- Ruby 2.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants