Skip to content

Support guioptions 'k' flag in MacVim (2nd attempt) #731

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
Sep 14, 2018

Conversation

ychin
Copy link
Member

@ychin ychin commented Aug 12, 2018

This re-commits and fixes the issues introduced in the previous pull request that implemented guioptions 'k' (#708). That commit was reverted in #727.

The issues it fixes:

  • Zoom button ("zoom" option in window menu stopped working with snapshot-150 #724): There was an error in refactoring leading to the handler calling the wrong function.

  • Toolbar addition/removal: Toolbar now respects the 'k' option. Toolbar is different from scrollbar and tabs because when you add/remove a toolbar in Cocoa it automatically resizes the window for you, so the implementation needs to manually un-resize the window and re-calculates the Vim view's 'lines' and 'columns' to fit.

NOTE: There's another issue where the last implementation broke MacVim on external monitors (#721 and #728). I do not have access to an external monitor now and can't test whether this fixes it (no USB-C adapter). I looked through the code and still couldn't figure out why the external monitor would cause issues especially if the user hasn't set go+=k. I will need to find an external monitor to test, but if someone can quickly try this out and sanity check it I would appreciate it.

Please do not merge yet until I can verify that the external monitor issue is fixed.

Edit: The external monitor issue is now fixed (e35b0a6). This PR should be OK to merge!

…size

Adds support for native GVim's 'k' flag. Adding/removing tabs/scrollbars
and setting 'linespace'/'columnspace' would now cause the number of
lines and columns in the buffer change to keep the window size constant,
instead of the other way round of resizing the window to keep the view
size constant. This helps prevent the window from resizing itself
unncessarily, which could be especially annoying when the window is
pinned/maximized.

Manually setting 'lines'/'columns', going to full screen, dragging the
window corner to resize would still resize the window.

Also removed misc calls within MMWindowController.m that were setting
shouldResizeVimView. Those calls were already handled by native Vim's
gui.c's gui_set_shellsize so it's redundant.

rebased from 1333bc6
@ychin
Copy link
Member Author

ychin commented Aug 12, 2018

Just pinging @ichizok @dctucker for this PR. I'm working to fix the previous issues in my implementation for guioptions 'k', and if you have time feel free to try this out to see if this fixes the issue. As I mentioned in the PR I don't have a way to test external monitors at the moment.

Fix the following:

* Zoom button: There was an error in refactoring leading to the handler
  calling the wrong function.

* Toolbar addition/removal: This now respects the 'k' option. Toolbar is
  different from scrollbar and tabs because when you add/remove a
  toolbar in Cocoa it automatically resizes the window for you, so the
  implementation needs to manually un-resize the window and
  re-calculates the Vim view's 'lines' and 'columns' to fit.
@dctucker
Copy link
Contributor

Unfortunately, it looks like the problem persists with this build.

@ychin
Copy link
Member Author

ychin commented Aug 13, 2018

Ok thanks for testing it out. I’ll find an external monitor to test it out.

@ychin
Copy link
Member Author

ychin commented Aug 19, 2018

I believe I have fixed all the issues introduced in the original implementation. Those are:

  1. Zoom button not working properly ("zoom" option in window menu stopped working with snapshot-150 #724)
  2. Opening Vim in an external monitor or opening a new Vim window in another monitor would result in broken rendering (Screen error #721, 3/4 of window is gray until resize #728)
  3. Tool bar not respecting guioptions+=k. (This one was less problematic since it didn't screw up the behavior for normal users who didn't set it)

Pinging @dctucker @ichizok again. Also @splhack if he wants to merge it. I tested this in an external monitor after the fix (yay USB-C dongle!) and couldn't reproduce it. I'm pretty confident the issue is now fixed.

Fix the issue that MacVim's window will have broken rendering (wrong Vim
size) if the window was opened in another monitor. This was introduced
as part of the implementation for guioptions 'k',

The issue was that the function `moveWindowAcrossScreens` was buggy. It
sets a flag "resizingDueToMove" but doesn't unset it after the
`setFrameTopLeftPoint`, which may or may not call the resize function
that is responsible in unsetting "resizeDueToMove". Fix the function to
always unset it so that the flag doesn't leak till the next resize.
Previously it "worked" due to MacVim's excessive resize messages masking
the issue.
@ichizok
Copy link
Member

ichizok commented Aug 22, 2018

@ychin Thank you, I confirmed the fix about toolbar on/off with guioptions-k.

@ychin
Copy link
Member Author

ychin commented Aug 26, 2018

Is there anything else I need to do for this pull request? I am pretty sure I addressed the issues caused by the original change and I have been using the editor after my fix and they seem to work fine. I have a few additional fixes for some full screen rendering and resizing bugs but they rely on this change so wondering what I can do to get this merged. Thanks.

@jpetrie jpetrie merged commit d02a194 into macvim-dev:master Sep 14, 2018
@ychin ychin deleted the guioptions-k-2 branch September 14, 2018 15:15
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
Development

Successfully merging this pull request may close these issues.

4 participants