MBP Touchbar Support #568

wiomoc · 2017-10-21T10:46:14Z

Proof of Concept
This needs some work

wiomoc · 2017-10-31T09:14:02Z

jpetrie · 2017-11-04T00:24:48Z

Thanks for this!

Apart from some minor style nitpicks (I'll go through and leave comments on the specific lines), this mostly looks reasonable. I don't have the hardware to test it. When you say "this needs some work," what sort of work are you thinking of?

jpetrie

As I said in the PR comment, most of my notes here are really minor nitpicky stuff about indenting consistency. I'm mostly concerned about the discussion in menu.vim and the else structure in enableMenuItemWithDescriptor... plus having some idea of what other "work" you think remains here.

jpetrie · 2017-11-04T00:41:49Z

runtime/menu.vim

@@ -1266,4 +1266,21 @@ if has("gui_macvim")
  macm Help.MacVim\ Website			action=openWebsite:
 endif

+if has("touchbar")


Is there a better place to do this setup? I'm specifically thinking that customizing the Touch Bar items will likely be something people will want to do quite a lot, and that having to first go through and delete these menu items is somewhat tedious. It might be better if the initial setup of the Touch Bar menu could be deferred somewhere so users could set a global variable in their vimrc to suppress default menu generation and instead generate the items themselves.

jpetrie · 2017-11-04T00:42:33Z

src/MacVim/MMVimController.h

@@ -28,6 +28,11 @@
    // 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


The indenting on this preprocessor line (and the corresponding #endif) is off.

jpetrie · 2017-11-04T00:47:20Z

src/MacVim/MMVimController.m

@@ -129,7 +129,11 @@ - (id)initWithBackend:(id)backend pid:(int)processIdentifier
        [[MMWindowController alloc] initWithVimController:self];
    backendProxy = [backend retain];
    popupMenuItems = [[NSMutableArray alloc] init];
+	#if MAC_OS_X_VERSION_MAX_ALLOWED > MAC_OS_X_VERSION_10_12


The indent on this #if/#endif pair is off.

jpetrie · 2017-11-04T00:47:48Z

src/MacVim/MMVimController.m

@@ -178,6 +182,11 @@ - (void)dealloc

    [toolbarItemDict release];  toolbarItemDict = nil;
    [toolbar release];  toolbar = nil;
+	#if MAC_OS_X_VERSION_MAX_ALLOWED > MAC_OS_X_VERSION_10_12


The indent on this #if/#endif pair is off.

jpetrie · 2017-11-04T00:49:51Z

src/MacVim/MMVimController.m

+#if MAC_OS_X_VERSION_MAX_ALLOWED > MAC_OS_X_VERSION_10_12
+- (NSTouchBar *)makeTouchBar
+{
+	    touchbar = [[NSTouchBar alloc] init];


Lines 503 - 506 have mixed tab/space indenting and are too far in.

jpetrie · 2017-11-04T00:52:37Z

src/MacVim/MMVimController.m

@@ -1081,7 +1108,10 @@ - (void)addMenuWithDescriptor:(NSArray *)desc atIndex:(int)idx

        return;
    }
-
+	#if MAC_OS_X_VERSION_MAX_ALLOWED > MAC_OS_X_VERSION_10_12


The indent of this #if/#endif block is off.

jpetrie · 2017-11-04T00:53:52Z

src/MacVim/MMVimController.m

@@ -1132,7 +1162,13 @@ - (void)addMenuItemWithDescriptor:(NSArray *)desc
            [self addToolbarItemWithLabel:title tip:tip icon:icon atIndex:idx];
        return;
    }
-
+#if MAC_OS_X_VERSION_MAX_ALLOWED > MAC_OS_X_VERSION_10_12
+    if ([rootName isEqual:@"TouchBar"]) {


At this point it might be worth writing isTouchBarMenuName or similar, or at least making the name a constant.

jpetrie · 2017-11-04T00:59:34Z

src/MacVim/MMVimController.m

@@ -1228,7 +1273,11 @@ - (void)enableMenuItemWithDescriptor:(NSArray *)desc state:(BOOL)on
            NSString *title = [desc lastObject];
            [[toolbar itemWithItemIdentifier:title] setEnabled:on];
        }
-    } else {


For clarity, this should be written in a way that doesn't inject the preprocessor check between the else and the if; this kind of construct is error-prone and can be difficult to follow (I realize it might be elsewhere in MacVim's codebase, but I'd rather we don't propagate it). I'd rather see something more like:

} else { #if MAC_OS_X_VERSION_MAX_ALLOWED > MAC_OS_X_VERSION_10_12 if ([rootName isEqual:@"TouchBar")) { return; } #endif // Use tag to set...

jpetrie · 2017-11-04T01:01:33Z

src/MacVim/MMVimController.m

+        } else {
+            label = NSTouchBarItemIdentifierFixedSpaceLarge;
+        }
+    } else {


This whole else block has mixed tab/space indenting and seemingly overly deep indents.

jpetrie · 2017-11-04T01:02:20Z

src/MacVim/MMWindowController.m

+#if MAC_OS_X_VERSION_MAX_ALLOWED > MAC_OS_X_VERSION_10_12
+- (NSTouchBar *)makeTouchBar
+{
+	return [vimController makeTouchBar];


A tab is used to indent here instead of spaces.

taiyangc · 2018-01-09T20:02:53Z

Awesome idea! Is it possible to further user-customize the touch bar shortcuts to specific macros?

getaaron · 2018-05-04T20:47:13Z

@wiomoc @jpetrie any updates on this? would love to see it merged

ychin · 2018-07-27T06:34:18Z

I have the same question on updates. I'm quite interested in having TouchBar support as right now it doesn't do anything when I have MacVim up :(. (macOS allows you to force it to only show function keys when MacVim is up but if you do that it's hard to adjust volume, etc)

I don't mind taking this over and help push this through if the original author is no longer interested. I think this at least needs some documentation in gui_mac.txt in addition to the existing comments.

Played around with it a little bit, and some additional potential improvements below. These are really just random ideas. Could be future pull requests after this is in:

You can't pan through the touch bar items if there are too many. Ideally you should be able to do that.

An alternative is expose the "Customize TouchBar…" option to give native TouchBar customization. Not sure how much work that is.

Different modes are not respected right now. E.g. vnoremenu TouchBar.VisualOnly <Nop> would still be active in normal mode. Normal menus and toolbars do respect the modes and gray out the items. I think for touch bar we don't even need to gray out the menu items. Just hide them to save space.
Nesting is not allowed right now. I.e. you cannot do :an TouchBar.Grouping.Item1 <Nop>. I'm thinking it could surface "Grouping" and when you tap on it it will show the individual items under it. This is useful as TouchBar's real estate is pretty small. E.g. you can use this to group debugger controls.

Another idea is to show all the subitems, but grouped together. This saves the extra tap and allow you to see what the options are.

Color support. Touchbar items can have customized colors rather than the default ones.
Kind of extra credit, but support other GUI widgets. Maybe this gets a little too outside of what Vim usually is like, but examples range from using the standard template images (https://developer.apple.com/design/human-interface-guidelines/macos/touch-bar/touch-bar-icons-and-images/) to supporting scrubber. You can kind of go down a rabbit hole on this one though when trying to shoehorn support for native TouchBar APIs onto Vim APIs.

jpetrie · 2018-07-27T14:44:24Z

@ychin

I don't mind taking this over and help push this through if the original author is no longer interested.

Given the amount of time this PR has sat here I think it would be fine for you to do so. As I noted in my initial review, most of what I'd want to see fixed before merging are just style issues to try and keep the code consistent. If you can build a PR that incorporates this original change and fixes those, it would probably get merged pretty quickly.

I don't have hardware to test it on, and I'm not personally that interested in the feature, but I would like to see more contributors since I don't have much time to devote myself. So by all means, if you want to pick this up and run with it, please do so!

ychin · 2018-07-29T09:55:56Z

I just submitted a new PR (#715) which takes this current patch and fixed up misc issues and added features.

jpetrie · 2018-07-29T15:26:12Z

Cool; I will take a look at it today.

basic touchbarsupport

1cfd28d

wiomoc mentioned this pull request Oct 21, 2017

Any plans for Touch Bar support? 😁 #435

Closed

Trixi1977 approved these changes Oct 28, 2017

View reviewed changes

jpetrie suggested changes Nov 4, 2017

View reviewed changes

jpetrie mentioned this pull request Apr 18, 2018

How about adding an option for changing the titlebar's color? #266

Closed

ychin mentioned this pull request Jul 29, 2018

MBP Touchbar Support (take 2) #715

Merged

jpetrie closed this in #715 Jul 30, 2018

ychin added Input Issues related to keyboard or other types of input UI Issues related to UI elements, tabs, scrollbars, window resizing, etc. and removed Input Issues related to keyboard or other types of input labels Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBP Touchbar Support #568

MBP Touchbar Support #568

wiomoc commented Oct 21, 2017

wiomoc commented Oct 31, 2017

jpetrie commented Nov 4, 2017 •

edited

Loading

jpetrie left a comment

jpetrie Nov 4, 2017

jpetrie Nov 4, 2017

jpetrie Nov 4, 2017

jpetrie Nov 4, 2017

jpetrie Nov 4, 2017

jpetrie Nov 4, 2017

jpetrie Nov 4, 2017

jpetrie Nov 4, 2017

jpetrie Nov 4, 2017

jpetrie Nov 4, 2017

taiyangc commented Jan 9, 2018

getaaron commented May 4, 2018

ychin commented Jul 27, 2018

jpetrie commented Jul 27, 2018

ychin commented Jul 29, 2018

jpetrie commented Jul 29, 2018

MBP Touchbar Support #568

MBP Touchbar Support #568

Conversation

wiomoc commented Oct 21, 2017

wiomoc commented Oct 31, 2017

jpetrie commented Nov 4, 2017 • edited Loading

jpetrie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taiyangc commented Jan 9, 2018

getaaron commented May 4, 2018

ychin commented Jul 27, 2018

jpetrie commented Jul 27, 2018

ychin commented Jul 29, 2018

jpetrie commented Jul 29, 2018

jpetrie commented Nov 4, 2017 •

edited

Loading