-
-
Notifications
You must be signed in to change notification settings - Fork 686
MBP Touchbar Support (take 2) #715
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
Conversation
- addressed review feedbacks - documentations
Enabled states now work. E.g. a mapped Touch Bar button using "vmenu TouchBar.DoStuff <nop>" will not show up in normal mode. Also support specifying default Apple template icons. E.g. "an icon=NSTouchBarListViewTemplate TouchBar.ShowList <Nop>" Remove default TouchBar buttons as there were too many of them and most of them are unlikely to be heavily used as there are direct Vim command equivalent. Instead just add a single fullscreen toggle button. This can be changed later.
@@ -9415,6 +9415,7 @@ tgetent Compiled with tgetent support, able to use a termcap | |||
timers Compiled with |timer_start()| support. | |||
title Compiled with window title support |'title'|. | |||
toolbar Compiled with support for |gui-toolbar|. | |||
touchbar Compiled with support for Touch Bar in MacVim. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not 100% if we should expose a feature test for this, but I guess if toolbar is already one, why not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you that you've exposed it as Tool bar and Touch Bar are different. One can be enabled without other
@@ -1287,4 +1287,27 @@ if has("gui_macvim") | |||
macm Help.MacVim\ Website action=openWebsite: | |||
endif | |||
|
|||
if has("touchbar") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the previous review there was a comment about potentially having too many default Touch Bar buttons. I addressed this in the pull request descriptions, but I basically removed all the original ones and just added a full screen button that can toggle icon depending on what state it's in. I think most users will prefer a close to empty Touch Bar for customization so it's not worth having too many defaults (I just think having a couple that works well is a good thing to entice users to learn how to customize it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good; let me know what you think about the comment regarding the availability macros.
src/MacVim/MMVimController.h
Outdated
@@ -28,6 +28,12 @@ | |||
// TODO: Move all toolbar code to window controller? | |||
NSToolbar *toolbar; | |||
NSMutableDictionary *toolbarItemDict; | |||
#if MAC_OS_X_VERSION_MAX_ALLOWED > MAC_OS_X_VERSION_10_12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done in a few places and I'm not sure it is correct (it was also in the original PR; I missed it then). NSTouchBar
is in the 10.12.2+ SDK and AvailabilityMacros.h
has entries that are granular to the second decimal place. In other words, MAC_OS_X_VERSION_10_12_1
is a thing. That SDK won't have TouchBar support, per Apple's docs, but will be greater than MAC_OS_X_VERSION_10_12
and thus cause a compiler error inside all these guards.
#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_12_2
seems like what we'd want here, and in all the other cases, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could do that, but now I'm just wondering whether we even should check for this, or just remove the macro usage. This check only helps people build (not use) MacVim on older platforms but I presume most people who want to contribute to or build MacVim would want to have recent versions of macOS and Xcode. Are we going to keep supporting older versions of macOS development tools? I did notice the fullscreen code still checks for MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7
which seems crazy to me. (Most other code checks for MIN_ALLOWED, not MAX_ALLOWED)
Another important thing to check is runtime checking to make sure the program won't crash on users with older devices. I think the current implementation actually doesn't check that… addMenuItemWithDescriptor
will call addTouchbarItemWithLabel
which will start initializing NSCustomTouchBarItem
which I think will crash in pre-Sierra machines (I don't have one to check and I'm not super familiar with Cocoa / Objective-C). I think we just need to inject some NSClassFromString(@"NSTouchBar")
checks before we start initializing the touch bar items, or delay making the touch bar items until we are in makeTouchBar
which will only get called in systems supporting the touch bar as a delegate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could do that, but now I'm just wondering whether we even should check for this, or just remove the macro usage.
I'd like to, but I don't think we can. There are users out there who build MacVim from source on older versions of the OS, for a variety of reasons (such as not really liking some of Apple's changes in later versions, et cetera). MacVim has long supported those users, much like vim itself.
Similar arguments pro and con can be made towards MacVim's support of legacy features (the old text renderer, the custom fullscreen mode) as well: these legacy features increase MacVim's maintenance cost, but there are users out there who really love them.
I don't think it is feasible to drop that support, as painful as it is to work with right now.
However, it would be great to come up with an alternative. Branches were floated as an idea a while back, but it didn't really go anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I just added the better compile + runtime checks and pushed a change.
Make sure to check MAC_OS_X_VERSION_10_12_2 so older Xcode / SDK versions will still be able to build MacVim, albeit without Touch Bar. Also make sure to use `NSClassFromString(@"NSTouchBar")` to check during runtime to avoid MacVim crashing for users using older versions of macOS when they by mistake bind a menu item with TouchBar as main menu (nothing will show up instead).
A.W.E.S.O.M.E. |
Can we see the statusline on the touchbar now? |
@vitaly-zdanevich No. The current TouchBar implementation allows you to add buttons using the misc Vim menu commands (e.g. Maybe it’s worth investigating it at some point but it’s non-trivial to get something like a statusline display to work. We will need to split out the statusline code in Vim to generate display commands for a new target and modify the CoreText renderer in MacVim to render specifically to TouchBar. Doable, but will require some work and I argue the utility from doing that is limited outside the initial cool factor as statusline already works in Vim. TouchBar is really designed and ideal for interactive displays like buttons rather than a status display. |
Sorry - complete newbie to Macvim and the touchbar in general. Could someone give me a simple example on what I could add to my .vimrc (or do i need to use a .gvimrc? I don't know the difference) to add a simple button to the touch bar? Thanks! |
Type Here's the docs:
|
Thank you! Never used menus before, didn't even know where to start looking or what they were called. Down the rabbit hole now... |
This finishes up the previous TouchBar pull request (#568). It addresses feedbacks from the code review as well as adding documentation and features.
Changes on top of the original pull request (which adds support for TouchBar via a special menu item
TouchBar
similar to how toolbars work):Enabled states now work properly. For example if you define a visual only button (e.g.
vmenu TouchBar.Copy "+y
), the button will now only show up in visual mode, and will not take up space otherwise.You can specify default Apple template Touch Bar icons. E.g.
:an icon=NSTouchBarListViewTemplate TouchBar.ShowList <Nop>
. For full list see https://developer.apple.com/design/human-interface-guidelines/macos/touch-bar/touch-bar-icons-and-images/Remove the old defaults Touch Bar buttons. They were mostly just copies of the ones from the standard toolbar and I doubt most users will actually use them (since there are direct Vim commands you can use to begin with). They also use the toolbar icons which don't work as well on the toolbar. Given how little space there is it's a good idea to not put too many default buttons there. Instead just put a single button there that allows the user to toggle fullscreen mode. Can add more later.
aunmenu TouchBar.
in their gvimrc to clear all the default ones.Added documentations.
The current implementation is sort of the bare minimum, but there are other rooms for expansion in the future. Some ideas:
Allow submenus, e.g.
:an TouchBar.Debugger.StepOver <nop>
. Requires some refactoring to support making use of popup items.Change how the buttons are display or even widget types. I'm thinking hijacking the existing
:macmenu
command is the safest choice as it prevents us having to hack new syntax and options into the miscellaneous:menu
commands. We would need to first make the menu using standard Vim command, then annotate it usingmacmenu
with the properties we want. Things we can support includeClose #568