Closed jpetrie closed 2 years ago
Hi @jpetrie sorry I did not get around to reviewing this PR. Can you reopen it? (Edit: i just reopened it)
I think we have a simpler change that we could make though: just move all the macm
(macmenu) calls to be within the did_install_default_menus
block.
This way we don't have to move the setting of did_install_default_menus
till later. It still makes merging from upstream slightly harder since the macmenu
call will be somewhere in the middle of the file instead of all the way in the end but I think it's unlikely it's going to cause issues. menu.vim
isn't a frequently updated file anyway, and a giant chunk of code addition like this is easy to merge and resolve.
We would also not have to define a new variable which seems much cleaner to me.
Ok I just did the change that I mentioned. Should be fixed now.
While investigating issue #1230, I added
let did_install_default_menus = 1
to my.vimrc
. This disables the generation of Vim's default menus, pergui.txt
. It also causes an error to be generated when usingmvim
to start MacVim from the Terminal:When
did_install_default_menus
is used this way,menu.vim
's calls tomacmenu
fail to find any menus, since they were never generated. This change introduces a similar global variable,did_install_default_mac_menus
that can be set similarly to skip themacmenu
calls and avoid the errors. This more easily supports the use-case where somebody wants to completely redefine the menu structure, which isn't common, but is what led me down this rabbit hole in the first place.I considered a few other approaches. They all have pros and cons, this seemed like the simplest overall:
menu.vim
aggressively setsdid_install_default_menus
to make the script reentrant. We could set the variable after all themacmenu
commands finish, letting one variable control both. However, this would make managing upstream merges harder, especially if anything now or in the future causes the script to return early.macmenu
could be changed, adding!
support, so thatmacmenu!
would silently ignore missing menus. This would probably introduce more upstream merge challenges, and wouldn't support the "I'd like to completely skip generating these menus" scenario.I explicitly did not update
delmenu.vim
to reset the new variable. It would be more correct to do so, but would create an upstream merge conflict for no appreciable benefit: ifmenu.vim
is resourced at runtime,macmenu
has no effect anyway. That could be fixed, but that's a much bigger change.This is a fairly niche change, so I'm entirely convinced it's worth pulling in, but I did the work so I figured I'd see what folks thought.