Skip to content

Support fixing "menu not found" error when started through mvim with default menus disabled. #1231

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

Closed
wants to merge 1 commit into from

Conversation

jpetrie
Copy link
Contributor

@jpetrie jpetrie commented Jan 25, 2022

While investigating issue #1230, I added let did_install_default_menus = 1 to my .vimrc. This disables the generation of Vim's default menus, per gui.txt. It also causes an error to be generated when using mvim to start MacVim from the Terminal:

Error detected while processing /Users/Josh/Documents/Dotfiles/vim/vimrc[6]../Applications/MacVim.app/Contents/Resources/vim/runtime/syntax/syntax.vim[25]../Applications/MacVim.app/Contents/Resources/vim/runtime/filetype.vim[2366]../Applications/MacVim.app/Contents/Resources/vim/runtime/menu.vim:
line 1306:
E334: Menu not found: File.New\ Window

When did_install_default_menus is used this way, menu.vim's calls to macmenu 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 the macmenu 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 sets did_install_default_menus to make the script reentrant. We could set the variable after all the macmenu 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 that macmenu! 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: if menu.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.

Previously, if you used `did_install_default_menus` to skip default
menu generation, you'd get an error at startup about macmenu not
finding the menus it expected to.

This changes lets you set `did_install_default_mac_menus` after
setting `did_install_default_menus` to fix the error, or simply to
disable all of MacVim's default command key and action mappings.
@ychin
Copy link
Member

ychin commented Aug 6, 2022

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.

@ychin ychin added this to the snapshot-174 milestone Aug 6, 2022
@ychin ychin reopened this Aug 6, 2022
ychin added a commit to ychin/macvim that referenced this pull request Aug 9, 2022
The `macaction` blocks were previously not respecting the
`did_install_default_menus` setting. If you set it to 1 in your vimrc,
MacVim would throw startup errors about macmenu not finding the menu
items it expected to. Move the `macaction` setup to the correct place so
they would only be called if `did_install_default_menus` is not 1.

Close macvim-dev#1231. Originally pull request by @jpetrie.
@ychin ychin closed this in #1267 Aug 9, 2022
@ychin
Copy link
Member

ychin commented Aug 9, 2022

Ok I just did the change that I mentioned. Should be fixed now.

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.

2 participants