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

[fix #766] Add an event for OS appearance change #929

Merged
merged 1 commit into from Oct 8, 2019
Merged

[fix #766] Add an event for OS appearance change #929

merged 1 commit into from Oct 8, 2019

Conversation

L-TChen
Copy link
Contributor

@L-TChen L-TChen commented Jul 30, 2019

This is my attempt to fulfil the feature request issued by myself (#766).

Overview

  • An event OSAppearanceChanged is added. This event is emitted when MacVim changes its appearance. (It's called OSAppearanceChanged, since most of end users may only change view's appearance via Preference.)

  • A vim variable v:os_appearance is added. This variable reflects the appearance setting.
    There are four possible values as explained in the comment in MMVimView.m

    // 0 : (default) The standard (light) system appearance.
    // 1 : The standard dark system appearance.
    // 2 : The high-contrast version of the standard light system appearance.
    // 3 : The high-contrast version of the standard dark system appearance.
    // see
    // https://developer.apple.com/documentation/appkit/nsappearancename

Test

The following Vimscript snippet demonstrates how this change can be used to change
background as well as colorscheme by hooking a callback function to the proposed event.

func! ChangeBackground()
  if (v:os_appearance == 1)
    set background=dark
    colorscheme NeoSolarized
  else 
    set background=light
    colorscheme NeoSolarized
  endif
  redraw!
  AirlineRefresh " if vim-airline is installed
endfunc

au OSAppearanceChanged * call ChangeBackground()

Result
Screen Recording 2019-07-30 at 16 05 28

To-do

  • MacVim should set up the variable v:os_appearance before running .vimrc. MMBackground does not respond the message when initialising MMVimView.

Any advice to set v:os_appearance during startup would be appreciated.

  • Changing background does not update until the window is focused, although the event is actually handled in the background. Is there a way to force the window to update? (30-07-2019 Update) Use redraw!.

  • Document it.

@eirnym
Copy link
Contributor

eirnym commented Jul 30, 2019

This is a nice addition!
AFAIK, There's a .gvimrc defined to initialize GUI-specific state and process.

@L-TChen
Copy link
Contributor Author

L-TChen commented Jul 31, 2019

I am not sure what is the best practice to initialise a vim variable when launching a Vim process.

Communicating through message queue seems unreliable as it may happen anytime. Also, the user may relay on this variable during startup, i.e..vimrc. Hence, it seems better to retrieve the value when launching a Vim process using the main application's appearance NSApp.effectiveAppearance since its MMVimView object is initialised yet (right?). Using system’s default value won't work in the next macOS as it has an Auto Mode ...

Taking the value from command-line arguments or the environment variable should work well..maybe?

@L-TChen L-TChen marked this pull request as ready for review July 31, 2019 13:47
@L-TChen
Copy link
Contributor Author

L-TChen commented Aug 25, 2019

@ychin It will be nice of you if you can give some suggestion on how to check the dark mode while launching the app. .. :-)

@ychin
Copy link
Member

ychin commented Sep 9, 2019

Hi sorry I will get back to you this weekend.

@ychin
Copy link
Member

ychin commented Sep 30, 2019

Hi @L-TChen I'm sorry for the late response. I checked out the code and it seems to work! I think there are a few minor things we should do:

  1. Squash the changes, and resolve merge conflicts (The change in src/eval.c got moved to src/evalvars.c, and there were move events added to src/vim.h).

  2. Documentation:

    • Can you add a quick documentation in runtime/doc/gui_mac.txt under "MacVim differences", right after macvim_commands? I'm thinking it could look something like this:
    @@ -121,6 +121,10 @@ These are the non-standard options that MacVim supports:
     These are the non-standard commands that MacVim supports:
            |:macaction|    |:macmenu|
     
    +                                                       *macvim-autocommands*
    +These are the non-standard autocommands that MacVim supports:
    +       |OSAppearanceChanged|
    +
                                                            *macvim-find*
     Whenever you search for something in Vim (e.g. using "/"), or hit <D-e> when
     you have text selected, the search query is copied to the macOS "Find
    • Can you add this line to both OSAppearanceChanged and v:os_appearance's documentation? Just to make it clear it's MacVim only:

        	{only in MacVim GUI}
      
  3. Initializing v:os_appearance when MacVim starts up: The reason why MMBackend doesn't respond in MMVimView's initWithFrame: is that when that init function is called, it's still within MMVimController's initialization, which hasn't completed yet. You probably need to add a line to MMVimController's initWithBackEnd: as follows (you need to do this after isInitialized is set to true for the message sending to work):

    diff --git a/src/MacVim/MMVimController.m b/src/MacVim/MMVimController.m
    index d29c77606..ef974d9a0 100644
    --- a/src/MacVim/MMVimController.m
    +++ b/src/MacVim/MMVimController.m
    @@ -175,6 +175,8 @@ - (id)initWithBackend:(id)backend pid:(int)processIdentifier
         [mainMenu addItem:appMenuItem];
     
         isInitialized = YES;
    +    
    +    [self appearanceChanged:getCurrentAppearance([windowController vimView].effectiveAppearance)];
     
         return self;
     }

@L-TChen
Copy link
Contributor Author

L-TChen commented Sep 30, 2019

Thanks for getting back to me! Okay, I will look into this this weekend.

@L-TChen
Copy link
Contributor Author

L-TChen commented Oct 5, 2019

All done, @ychin! Here are some differences from what you suggested:

  • In runtime/doc/gui_mac.txt, I added "... non-standard autocommands events..." instead.

  • The patch Get MacVim to build in Xcode 11 #936 is included, otherwise I am not able to compile MacVim any more.

However, there is a glitch with Powerline which does not render its tabline properly. That is,

ezgif-5-e06f72c663eb

No idea how to fix it. Not even know which part should be responsible for this.

@ychin
Copy link
Member

ychin commented Oct 6, 2019

The change looks good! Can you squash your changes, and also remove the #936 patch since it's now merged?

However, there is a glitch with Powerline which does not render its tabline properly. That is … No idea how to fix it. Not even know which part should be responsible for this.

Hmm, is that related to your change? You have to manually switch colorscheme for this bug to show up right? I would imagine just switching color schemes without invoking this autocommand would trigger this bug? If so that seems unrelated.

@L-TChen
Copy link
Contributor Author

L-TChen commented Oct 6, 2019

You have to manually switch colorscheme for this bug to show up right? I would imagine just switching color schemes without invoking this autocommand would trigger this bug? If so that seems unrelated.

You're right---it is not really related to my change. It seems that the bug is triggered while the window is inactive.

@L-TChen
Copy link
Contributor Author

L-TChen commented Oct 6, 2019

Now they are all squashed and should be ready to merge. :-)

@L-TChen L-TChen changed the title [re #766] Add an event for OS appearance change [fix #766] Add an event for OS appearance change Oct 6, 2019
@ychin ychin merged commit 0b679a4 into macvim-dev:master Oct 8, 2019
@ychin
Copy link
Member

ychin commented Oct 8, 2019

Merged. Thanks for the work.

@erikw
Copy link

erikw commented Apr 25, 2021

Thanks @L-TChen for this feature!

It works perfectly in graphical MacVim.app to listen for the OS appearance change event and change background accordingly.

Question: could it be made to work in cli vim as well? For me, this seems to not work in cli vim.

I tried using cli vim installed from the macvim homebrew formula:

$ which vim
/usr/local/bin/vim
$ ls -l /usr/local/bin/vim
/usr/local/bin/vim -> ../Cellar/macvim/8.2-171/bin/vim

but the event callback is not called (like it is in MacVim.app)

@g6ai
Copy link

g6ai commented Apr 25, 2021

@erikw I managed to control CLI Vim appearance on enter to align to OS appearance, but yet to create a listener for automatic switching. You could refer to my Vim config here to implement it:
https://github.com/g6ai/dotfiles/blob/61ec0e0b9894960e158467f9c221bd5620411cf6/vim/vimrc#L281-L295

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

5 participants