Add Touch Bar Support #987

Bi11 · 2017-01-08T08:00:31Z

Add Touch Bar support by mirroring buttons for each dialog.

kornelski · 2017-01-08T11:42:01Z

Thank you for the pull request. This is a good addition, we've been discussing it in issue #922

kornelski · 2017-01-08T11:47:31Z

Sparkle/SUAutomaticUpdateAlert.m

+        if ([control isKindOfClass:[NSButton class]]) {
+            NSButton *button = (NSButton *)control;
+            if (button.action == @selector(installNow:)) {
+                installButton = button;


Could you use outlets, like this?

https://github.com/sparkle-project/Sparkle/blob/1.17.x/Sparkle/SUUpdateAlert.m#L47,L49

In the 1.17.x branch I've added one for the automatic update alert as well:
https://github.com/sparkle-project/Sparkle/blob/1.17.x/Sparkle/SUUpdateAlert.m#L47,L49

you could add the other two (it's OK if you change only the English xib, I'll update the rest).

I did use outlets in my initial draft, but I was horrified by the xibs...
So I decided to use a 'just working' way by iterating through subviews, and change them to outlets in the future.

I think I can update them later.

I have a script that copies changes between xibs, so it's enough if you just modify English one.

kornelski · 2017-01-08T11:52:13Z

Sparkle/SUTouchBar.m

+
+#import "SUTouchBar.h"
+
+#if __MAC_OS_X_VERSION_MAX_ALLOWED >= 101202


I'm not sure if that will work on older systems. This checks for the SDK version, rather than the OS version Sparkle is running on.

Yes, it is a SDK version check.
If SDK version is lower than 10.12.2, there are not definitions for NSTouchBar and it's related APIs. So I added fake implementation to make sure the file can be compiled
That means, if Sparkle compile with SDK < 10.12.2, there will be no Touch Bar support.

Does the latest SDK link the NSTouchBar symbol weakly? I wonder if some class-from-string trickery is needed for gracefully failing on older systems.

Last looking at the code, I don't think we need class-from-string trickery because of hooking into the responder chain. The SDK check is more about compiling with older Xcode versions which shouldn't be the highest priority right now (In fact I'd be more interested currently in seeing if Travis can compile with touch bar support after upgrading Xcode 8 image).

NSTouchBar is marked NS_CLASS_AVAILABLE_MAC(10_12_2), so it will be weakly linked.
To support touch bar with compiling with old SDK, we need a forward declare and NSClassFromString trick.

zorgiepoo · 2017-01-08T14:45:44Z

Thanks for the pull request!

Few observations:

Please avoid subclassing NSTouchBar. -makeTouchBar won't be called on systems that don't support the touch bar. I've also been wanting to ban [most] subclassing in general for the project. Is there a reason you're doing a SDK version check (Travis, perhaps?)
Agree with using outlets rather than iterating through subviews
Is it possible to use @selector()'s and convert them to strings rather than hardcoding "title" and "enabled" when binding properties? Just to get some compile-time assurance. Bindings are a clever approach here.
Minor point: for new code we should: a) use nonatomic for properties, b) @synthesize x = _x, c) use instance variable access in initializers rather than property setters.

Thanks for testing the UI tests!

zorgiepoo · 2017-01-08T15:06:23Z

We may want and need to change osx_image in .travis.yml from xcode8 to xcode8.2 (reference)

Bi11 · 2017-01-08T16:20:45Z

Agree with avoiding subclassing. I used a property named items and boom, the touch bar buttons show in wrong position. I'd like to use NSTouchBarDelegate instead, but it requires a strong reference to hold it.
Is there a relation between avoiding subclassing and -makeTouchBar won't be called?
SDK version check is used to make sure the project can be compile with older SDK.
Agree with outlets.
I think cocoa binding is a different concept from @selector. Sample codes listed in Cocoa Bindings Programming Topics are using strings. See https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CocoaBindings/Concepts/HowDoBindingsWork.html or https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CocoaBindings/Concepts/NSUserDefaultsController.html#//apple_ref/doc/uid/TP40001092-BCICADHC
Agree. But it may look weird if two styles mixed in the same file.

zorgiepoo · 2017-01-08T19:22:34Z

Agree with avoiding subclassing. I used a property named items and boom, the touch bar buttons show in wrong position. I'd like to use NSTouchBarDelegate instead, but it requires a strong reference to hold it.

I've set the delegate before in -makeTouchBar so I'm not following.

Is there a relation between avoiding subclassing and -makeTouchBar won't be called?

I don't think so. You have two different subclass implementations based on a SDK check. Instead, you could surround the SDK check around -makeTouchBar (as long as you avoid subclassing NSTouchBar..)

I use a macro like this in one of my projects for obtaining strings from property names (uses the comma operator, and sizeof operator doesn't evaluate the expression but makes sure the symbol exists). We could stick it in a new header file.

#define SU_SELECTOR_STRING(object, name) (sizeof(object.name), @#name)

SU_SELECTOR_STRING(self.actionButton, title)

Bi11 · 2017-01-09T00:28:21Z

For old SDK, it is lacking the definition of NSTouchBar and NSTouchBarDelegate. A complete forward declaration is required, e.g.,
https://chromium.googlesource.com/chromium/src/+/master/ui/base/cocoa/touch_bar_forward_declarations.h
So,

Do we plan to support compiling with old SDK?
Do we plan to support Touch Bar if compiling with old SDK?

For binding keypath compile time check, I found a solution in ReactiveObjC:
https://github.com/ReactiveCocoa/ReactiveObjC/blob/70b87eec14119315a220ef9f63923d8a4739f009/ReactiveObjC/extobjc/EXTKeyPathCoding.h
So we can use @keypath(self.button, title) instead of hardcoding strings.

zorgiepoo · 2017-01-09T00:37:52Z

I think we should focus on writing the code correctly first before worrying about older SDKs. Maybe it's reasonable to not support the touch bar when compiling with an old SDK.

My macro above seems to be similar and simpler than the one ReactiveCocoa uses.

Bi11 · 2017-01-10T01:09:43Z

I've add some updates. But currently:

Only the English xib is updated.
Binding key path macro check is not added. I'm still feel that the macro increases the complex, for checking two well-known keys only.

There is an issue in current commit, the default button (rightmost one) is not colored blue in touch bar. This issue happens in SUUpdatePermissionPrompt and SUAutomaticUpdateAlert only, but SUUpdateAlert and SUStatusController are working. The keyEquivalent is correct in windowDidLoad, but it is empty in -makeTouchBar. This is weird. Maybe the modal mode changed the default button's keyEquivalent.

If so, I have to move the SUTouchBarProvider setups into windowDidLoad. But I need more time to find out the reason why the keyEquivalent is missing.

zorgiepoo · 2017-01-10T01:50:21Z

There is an issue in current commit, the default button (rightmost one) is not colored blue in touch bar. This issue happens in SUUpdatePermissionPrompt and SUAutomaticUpdateAlert only..

Yeah, this is kind of weird. I can reproduce this defect with the update permission prompt 100% of the time, but only reproduced the defect with the automatic update alert the first couple times I tried.

Bi11 · 2017-01-10T10:25:01Z

I've found the reason, here is the stack trace:

  * frame #0: 0x00000001000ea6b7 Sparkle`-[SUButtonCell setKeyEquivalent:](self=0x00006080000e9b80, _cmd="setKeyEquivalent:", keyEquivalent=@"") + 39 at SUButton.m:18
    frame #1: 0x00007fffab1faea6 AppKit`-[NSWindow _disableEnablingKeyEquivalentForDefaultButtonCell] + 104
    frame #2: 0x00007fffab184ff6 AppKit`-[NSWindow _doWindowWillBeVisibleAsSheet:] + 202
    frame #3: 0x00007fffab8e9d7e AppKit`-[NSWindow _reallyDoOrderWindowAboveOrBelow:relativeTo:findKey:forCounter:force:isModal:] + 1514
    frame #4: 0x00007fffab8eb1c9 AppKit`__71-[NSWindow _doOrderWindow:relativeTo:findKey:forCounter:force:isModal:]_block_invoke.3263 + 105
    frame #5: 0x00007fffab17bddc AppKit`NSPerformVisuallyAtomicChange + 147
    frame #6: 0x00007fffab1837f3 AppKit`-[NSWindow _doOrderWindow:relativeTo:findKey:forCounter:force:isModal:] + 1190
    frame #7: 0x00007fffab29d23d AppKit`-[NSApplication _orderFrontModalWindow:relativeToWindow:] + 748
    frame #8: 0x00007fffab29cbe4 AppKit`-[NSApplication _commonBeginModalSessionForWindow:relativeToWindow:modalDelegate:didEndSelector:contextInfo:] + 629
    frame #9: 0x00007fffab29c969 AppKit`-[NSApplication beginModalSessionForWindow:] + 37
    frame #10: 0x00007fffab47ef51 AppKit`__35-[NSApplication runModalForWindow:]_block_invoke + 39
    frame #11: 0x00007fffab29c862 AppKit`-[NSApplication runModalForWindow:] + 137
    frame #12: 0x000000010010cf63 Sparkle`+[SUUpdatePermissionPrompt promptWithHost:systemProfile:reply:](self=SUUpdatePermissionPrompt, _cmd="promptWithHost:systemProfile:reply:", host=0x000060800002f240, profile=@"11 elements", reply=0x0000000100110b30) + 467 at SUUpdatePermissionPrompt.m:85
    frame #13: 0x0000000100110ad1 Sparkle`-[SUUpdater startUpdateCycle](self=0x0000600000081680, _cmd="startUpdateCycle") + 1281 at SUUpdater.m:218
    ...

The default button's keyEquivalent will be set to empty string by NSWindow.

I think we need to set the touch bar's default button's keyEquivalent explicitly.

zorgiepoo · 2017-01-10T11:08:59Z

Sparkle/SUTouchBarProvider.m

+    if (!(self = [super init]))
+        return self;
+
+    indentifier = [NSString stringWithFormat:@"%@.%@", [NSBundle mainBundle].bundleIdentifier, anIdentifier];


I don't feel easy about using the main bundle identifier. Maybe SPARKLE_BUNDLE_IDENTIFIER could be used instead if needed.

Main bundle identifier means system level unique. SPARKLE_BUNDLE_IDENTIFIER means application level unique. I'm not sure which is "globally-unique", but I think it is not really matters here.

zorgiepoo · 2017-01-10T11:09:27Z

Sparkle/SUTouchBarProvider.m

+        return self;
+
+    indentifier = [NSString stringWithFormat:@"%@.%@", [NSBundle mainBundle].bundleIdentifier, anIdentifier];
+    touchBarItems = [NSMutableArray arrayWithCapacity:6];


Don't hardcode a capacity.

A macro or a const integer?

zorgiepoo · 2017-01-10T11:10:00Z

Sparkle/SUTouchBarProvider.m

+}
+
+-(void)addSpace {
+    return nil;


Can't return nil here.

zorgiepoo · 2017-01-10T11:10:54Z

Sparkle/SUTouchBarProvider.m

+    NSCustomTouchBarItem *item = [[NSCustomTouchBarItem alloc] initWithIdentifier:itemId];
+
+    NSButton *buttonCopy = [NSButton buttonWithTitle:button.title target:button.target action:button.action];
+    buttonCopy.keyEquivalent = button.keyEquivalent;


Don't need this anymore, right?

I prefer to keep it, because the keyEquivalent set on returned NSButton is a fix for modal window.
I'd like to change the API to -addButtonWithButton:isDefault:, to avoid the keyEquivalent issue.

zorgiepoo · 2017-01-10T11:21:20Z

Setting the keyEquivalent explicitly does sound like a reasonable choice. 👍

Could we, at least temporarily, remove the SDK check, and see if the current Travis builds or not with the touch bar? Also we would want to update the travis file as I mentioned in a comment above.

I'm not keen yet on having a mutable touch bar provider (and ivar) as another level of indirection, but I can come back later to formulate my thoughts on that.

zorgiepoo · 2017-01-10T11:42:46Z

I submitted a PR for upgrading the continuous integration files. (#991)

zorgiepoo · 2017-01-10T12:40:06Z

So as I mentioned before we should not have an intermediate touch bar provider:

Make SUUpdateAlert, SUAutomaticUpdateAlert, SUStatusController implement NSTouchBarDelegate directly. Remove touch bar provider ivar.
Don't use NSGroupTouchBarItem. Create touch bar items separately. [edit: or is it needed on update alerts for centering?]
Don't dynamically construct strings for touch bar identifiers (via runtime). For each controller, define constant strings for touch bar identifiers, separately. Can concatenate the constant SPARKLE_BUNDLE_IDENTIFIER if desired.
Add a common helper function/method that returns a NSTouchBarItem* and takes a button to copy, identifier, and key equivalent as arguments.

In the grand scheme of things, the automatic and regular update alert's will be merged together (2.0) so don't worry about there being similar code among those. With these changes, we'd get rid of a strong reference to a variable we don't need, a mutable array we don't need, and it wouldn't feel like we are wrapping around an API that wasn't intended to be wrapped around.

Bi11 · 2017-01-10T16:30:28Z

@zorgiepoo, I think there is a different in design pattern between yours and mine.
Your idea is to provide touch bar for each dialog. So implement delegate directly, fixed identifier is the 'correct' choice.
But my idea is to provide a touch bar provider / helper, to mirroring all dialog buttons automatically. The host dialog is care about buttons only, and the helper did the rest.

My idea came from NSAlert. Touch bar is only a extension to existing dialogs, provides easy accesses to the dialog buttons. I would not like to make it feels like that to design touch bar for each dialog.

In another word, from the UI appearance, I think all Sparkle dialogs are subclasses of NSAlert.

And for my implementation, all NSTouchBar related APIs are used only in SUTouchBarProvider. This is easy for conditional compile for different SDK.

May be the name of SUTouchBarProvider is confusing.

As the centering of the buttons, I tried several combination. And I think the center position looks like NSAlert is the most comfortable one.

…valent

zorgiepoo · 2017-01-10T16:49:23Z

Yeah, I feel a bit strongly about this :). The current approach is creating another layer I'm not comfortable with having, and it has side effects (strong reference, mutable object).

NSAlert is fine because Apple provides it. The way Apple provides support for the touch bar is such that a controller implements its own logic for it. The automatic alert will be removed in the near future, and we'll have two dialogs not that similar to each other.

And I do not want a SDK check to drive a design decision like this, which is why I repeat saying to not worry about it initially (in fact I'm curious if Travis will fail with it off after the recent changes I pushed).

Bi11 · 2017-01-10T17:16:39Z

I don't like strong reference either, so in the first commit I used subclassing. When I changed to delegate, I must have a strong reference to the delegate.

I don't like the mutable array either. And in my very early draft, I used -initWithIndentifier:buttons: to setup buttons. But later, I found it is too difficult to use because we need insert spaces and change the copied buttons. So I used more simple API, like NSMenu.

Maybe rename SUTouchBarProvider into SUTouchBarBridger is more acculturate.

I also hate the SDK checking. But Sparkle minimal building environment is Xcode7. I don't want to push it to 8.2 right now. If NSTouchBar related API is used in serval files, it would be a mess to fix the compile.

Bi11 · 2017-01-11T07:12:09Z

I agree with you on subclassing. I don't want to use subclassing for removing the ivar either. So the only dispute is whether it is acceptable to use a helper as an indirect implementation.

Apple's documents and sample code can't list every cases. Even it is very common to set delegate to self, but it is not a must. If multiple window share same touch bar template, a helper or delegate is needed for creating touch bar from template. It is very common to reuse customized controls. Writing redundant setup codes in every window controller is much worse.

I don't know why you think the touch bar is different from each Sparkle dialog. As I see it, they are buttons grouped in center, and almost will not change in the future. If no such high similarity exists, I definitely go with your approach. This helper is used to reduce the redundancy. I think you might be accept if it is only a helper function like +makeTouchBarWithButtons:. So how about rename the helper class to SUTouchBarBuilder?

zorgiepoo · 2017-01-11T10:19:40Z

Writing redundant setup codes in every window controller is much worse.

Where we seem to differ is that I think writing and keeping an additional layer of builder code is worse than the little bit of redundant code (that can be put in re-usable functions where appropriate..). Possibly a different case if we were reusing the same view across the windows. [edit]: I'd be a little more accepting to your approach if you could do it without setting the touch bar delegate, i.e., without mixing the builder type of code so much with the declarative API.

It wasn't ever the naming of the class I was that concerned about.

zorgiepoo · 2017-01-11T10:48:22Z

Other things regarding the current code:

re: nonatomic, synthesize x = _x,
Agree. But it may look weird if two styles mixed in the same file.

This applies less so if we create a new module/class/file (SUTouchBarProvider)

Pad a space after the - or + in method signatures. This is one bit of consistency our codebase has.

"addButtonWithButton" doesn't sound properly named. Perhaps "addTouchBarButtonUsingButton:" or "addTouchBarButtonFromButton:"

I think you are right your class should be renamed. It's not a NSTouchBarProvider and probably shouldn't be called a touch bar provider.

New headers should use NS_ASSUME_NONNULL_BEGIN / NS_ASSUME_NONNULL_END (e.g. see SUAppcast.h)

A comment should be added why passing a separate flag for setting the key equivalent is necessary.

I'd prefer SDK checking to be inlined in the methods and have multiple checks, so you don't make a mistake like returning nil from a void method again, or rename one method in one place and forget to rename in other, etc.

zorgiepoo · 2017-01-11T11:01:25Z

Sparkle/SUAutomaticUpdateAlert.m

+    [self.touchBarProvider addButtonWithButton:self.laterButton isDefault:NO];
+    [self.touchBarProvider addButtonWithButton:self.installButton isDefault:YES];
+    [self.touchBarProvider addSpace];
+    [self.touchBarProvider addSpace];


I don't think two spaces as extra padding should be added. It seems kind of hacky/maybe fragile to me, and I think it looks visually fine without them.

Bi11 · 2017-01-11T18:23:56Z

I have more new questions now...

Button Size and Position. I compared the button size from NSAlert. It's button layout looks better. The buttons have more padding space inside, and the button is equal sized if available. I added extra 70pt on width, it looks better for "Install Update", but worse for "Don't Check" / "Check Automatically".
SDK checking. I tried to change checking macro inlined in the methods, but the macro reduces the readability. Since once we decided to support old SDK or not, the fake implementation will be removed.
SDK TouchBar declaration. I'm using SDK 10.12.2, NSTouchBar is marked NS_CLASS_AVAILABLE_MAC(10_12_2). But a forward declaration header file found in chromium is marked NS_CLASS_AVAILABLE_MAC(10_12_1). I don't have SDK 10.12.1 right now.
objc_setAssociatedObject. It look like that I can set the touch bar delegate as associated object for NSTouchBar object. I feel wrong that SUTouchBarBuilder implements the touch bar delegate, since it was renamed to SUTouchBarBuilder. When the builder gone, delegate gone. This is bad.
Since subclassing NSTouchBar is not advised, what about objc_setAssociatedObject?
Or a global shared touch bar delegate in SUTouchBarBuilder?

If only three button is supported, just like NSAlert. I may write a new helper to create a SUButtonGroupTouchBarItem. And the rest part will just follow @zorgiepoo 's approach naturally. And the ivar holding the builder can be removed.

It might be a 'good' design.

Bi11 · 2017-01-11T19:53:32Z

In the next step, I might change the primary item into a NSCustomTouchBarItem with buttons auto layout in it's view. SUButtonGroupTouchBarItem will become SUButtonGroupTouchBarItemView.

zorgiepoo · 2017-01-11T21:43:53Z

Button Size and Position. I compared the button size from NSAlert. It's button layout looks better. The buttons have more padding space inside, and the button is equal sized if available. I added extra 70pt on width, it looks better for "Install Update", but worse for "Don't Check" / "Check Automatically".

Are you talking about the touch bar buttons, or the buttons on the window? If the latter, then that should belong and be discussed in a separate pull request (and changes shouldn't be made here).

Injecting state viaobjc_setAssociatedObject and global state (singleton) should be avoided at all costs, even more-so than subclassing.

The issues I see with the current code:

Don't subclass NSTouchBarItem as I mentioned above about the dangers of subclassing classes not documented to be subclassed [edit: at least not in the way you did]. That, and I want to get rid of subclassing wherever possible (composition should be preferred when possible as a general rule). Create functions if you must (not classes -- because functions restrict you to more sensible and not complex code).
The status window controller should not use a group touch bar item.
I feel like wrong abstractions are being built over the course of this thread. Code duplication is not the worst thing.

Bi11 · 2017-01-12T08:14:40Z

Agree with all you mentioned above. I just have no time yesterday to complete it all. So I decided to change it step by step without breaking it's usability.

As I mentioned in last comment, I will drop the NSGroupTouchBarItem and the subclassing, by replacing it with a view or view controller to layout the buttons.

It cost too much to build a helper to handle touch bar completely, with delegate patten.

Bi11 · 2017-01-12T17:47:54Z

Two more things to do:

Adjust touch bar buttons width padding in -updateConstraints of the view controller.
Add forward declaration for old SDKs.

zorgiepoo · 2017-01-12T17:53:40Z

The thing about a new view is that we still add another dependency. I feel the payoff has to be really worth it to justify doing that. Haven't taken look at the current code yet though.

Bi11 · 2017-01-12T19:02:02Z

Sometimes it is hard to tell which implementation is better. They both have pros and cons. I found NSAlert is using a private NSButtonGroupTouchBarItem as it's touch bar item. I think NSButtonGroupTouchBarItem may be also used in Open/Save panel, or other similar dialogs. If NSButtonGroupTouchBarItem is public, I think we will all go with it. But sadly, it is private.

What we should do if we want to add touch bar for our own application's alert/save dialog? There will be three level that can be reused, TouchBar, TouchBarItem and TouchBarItemView. For my own project, I prefer to reuse TouchBarItem, i.e., write a similar MyButtonGroupTouchBarItem, and wish the original one can be public in the future SDK. I think reusing view is a compromise to avoid subclassing TouchBarItem, and it fits the SDK.

Interface Builder treats NSTouchBar in xib more like a control, which made me want to subclass NSTouchBar in the beginning. It builds touch bar by a private NSTouchBarStaticConfiguration. But for programming, only delegate is available.

zorgiepoo · 2017-01-12T21:47:37Z

The thing about a new view is that we still add another dependency. I feel the payoff has to be really worth it to justify doing that. Haven't taken look at the current code yet though.

So I typed this without looking at the current code and made a bad guess of what the code was going to look like.

Having this being written such that it can be used easily by other projects should not be a concern for us; that should not be our aim. Discussing about an SDK seems to be kind of a red flag to me :/.

SUTouchBarButtonGroup is inappropriately named. It really should be like SUTouchBarAlertViewController or something.

I don't see why the status window controller uses this class (as an aside: I don't consider the status window to be very much like an alert).

One concern I have now is we're now writing a custom layout instead of letting the touch bar handle the layout for us.

The approach I suggested is still the simplest (less dependencies / creating additional layers), not to be confused by highest code-reuse.

Bi11 · 2017-01-12T22:10:30Z

I don't see why the status window controller uses this class (as an aside: I don't consider the status window to be very much like an alert).

I plan to adjust all touch bar buttons with extra 70pt padding on width, including the single button in Status window.

Using a layout is the only idea currently that I can set the button width equally.

If layout is not involved. The only code left in SUTouchBarButtonGroup is only button copy. Surely it should be downgraded to a helper function. But if layout is involved, I'd like to keep it as a helper class.

SUTouchBarButtonGroup may be renamed to SUTouchBarButtonGroupViewController but it seems too long. It is a general view for button group in touch bar, designed for all four dialogs.

zorgiepoo · 2017-01-12T22:22:12Z

Using a layout is the only idea currently that I can set the button width equally.

I don't think the widths should be equal. They aren't equal in our windows, and they aren't even equal in standard NSAlert's, right?

I plan to adjust all touch bar buttons with extra 70pt padding on width, including the single button in Status window.

I'm not even sure about this; not even sure it'll look better, and somewhat doubting that will be worth the cost for having a custom layout.

I had the feeling SUTouchBarButtonGroup was very specific to an alert like layout.

zorgiepoo · 2017-01-12T23:10:44Z

OK, I see they are equal in the touch bar for NSAlert's (interesting), and am now understanding your private mention of NSButtonGroupTouchBarItem and probably the padding suggestion (similar to NSButtonGroupTouchBarItem, yes?). Sorry for doubting :), I think the custom layout may be justified then.

I'll try to come back later to review other changes.

Bi11 · 2017-01-13T00:14:48Z

A guidance from 'Design for the Touch Bar' of macOS Human Interface Guidelines:

Strive to match the look of the physical keyboard.

Equal width make the button more like a physical key.
Padding is actually for matching the touch bar buttons from system dialogs.

I think it still looks good without extra padding currently. We may add padding in another pull request.

zorgiepoo · 2017-01-13T03:13:52Z

Sparkle/SUTouchBarButtonGroup.m

+    if (!(self = [super init]))
+        return self;
+
+    NSView *buttonGroup = [NSView new];


+new is the same as [[NSView alloc] init]. In general, objc programmers don't use +new. An actual issue though is that -init is not documented for NSView so I think I'd be hesitant with invoking it. NSView's designated initializer is '-initWithFrame:' -- you can try using that, even if it means supplying an empty rect, I suppose.

zorgiepoo · 2017-01-13T03:17:09Z

Sparkle/SUTouchBarButtonGroup.m

+
+@synthesize buttons = _buttons;
+
+- (instancetype)initByReferencingButtons:(NSArray<NSButton *> *)buttons


I think the API may be more readable / sensible if it took up to three button parameters (a couple being nullable) instead of an array of buttons. We only need up to 3 in the project, and it'd give actual names instead of relying on button indices. The code currently applies very specific logic for buttons at certain indices.

It may be looks like

- (instancetype)initByReferencingDefaultButton:(NSButton *)defaultButton alternateButton:(nullable NSButton *)alternateButton otherButton:(nullable NSButton *)otherButton;

But it conflicts with the property buttons. It may need to three properties for holding three buttons.
The view places buttons from right to left, just like NSAlert. It is designed to rely on indices. Also the view does not care about the purpose of the buttons, it cares about layout only. I think it is good to keep the button array.

It may be looks like

I think that's good.

But it conflicts with the property buttons. It may need to three properties for holding three buttons.

Indeed. Although, there is no need to add properties if they're currently being unused by Sparkle, so you may be able to get away with one property.

I think that the layout cares is a good enough reason. It's not as simple as "right to left" -- padding differs in a certain spot, and one button has a return mapping.

I don't know whether you still want to change the API. If needed, I prefer to make it looks like NSAlert's API:

- (instancetype)init; - (NSButton *)addButtonWithTitle:(NSString *)title; @property (readonly, copy) NSArray<NSButton *> *buttons;

Named button API is deprecated in a different reason:

+ (NSAlert *)alertWithMessageText:(nullable NSString *)message defaultButton:(nullable NSString *)defaultButton alternateButton:(nullable NSString *)alternateButton otherButton:(nullable NSString *)otherButton informativeTextWithFormat:(NSString *)format, ... NS_FORMAT_FUNCTION(5,6) NS_DEPRECATED_MAC(10_3, 10_10, "Use -init instead");

And no similar API was add back.

We shouldn't imitate NSAlert exactly (for that reason alone). NSAlert is also not a view controller. I'd stick with the single initializer and not go for more mutability (which should be avoided when possible). This is also a private API for us, not one that we are exposing, so we don't have to worry about certain future implications. This is also why we don't need to expose properties for alternative and other buttons, or support more than three buttons.

You mean

- (instancetype)initByReferencingDefaultButton:(NSButton *)defaultButton alternateButton:(nullable NSButton *)alternateButton otherButton:(nullable NSButton *)otherButton;

with one property defaultButton only?
So the SUTouchBarButtonGroup will not be the correct name, since ButtonGroup mean any number of buttons. It may need to change to SUTouchBarAlertView as you suggest.

I'm very sorry than I still can't accept your suggestion now. Could you please merge this pull request and change the API later if you insist?
I am fine with the API as you suggest, but I don't want to change it in my commit because it is against my will. I hope you can change it in your commit, to keep the fact that we have different designs.

That is OK :). Are you planning on creating more PR's for this? I think the SDK checking was all left out. If you do and if @pornel doesn't, I'd appreciate it if you added the comments/docs I requested.

zorgiepoo · 2017-01-13T20:15:53Z

Equal width make the button more like a physical key.

I don't think this is the right justification. Physicals key don't all have equal widths. But I presume that they should be equal for some other reason, else they wouldn't be for NSAlert.

I would add comments/documentation on why we need to set the key equivalent instead of referencing the button, and why we have a custom layout (mentioning equal widths and mimicking the private NSButtonGroupTouchBarItem behavior that NSAlert uses). Just a measure preventing someone in the future for removing such code unknowingly.

Bi11 · 2017-01-14T04:40:43Z

To my understands, physical keys layout is symmetry, which implies equal width.
I have no Mac to use at this weekend and unable to do any updates...

kornelski · 2017-01-16T12:12:36Z

I don't have any objections to merging it. We can see if the current implementation works for us, and tweak the API later if it turns out necessary.

kornelski · 2017-01-16T12:13:13Z

Thank you @Bi11!

zorgiepoo · 2017-01-16T19:01:04Z

Please add the comments/docs then @pornel Too important to forget and delay later. Should be easier for you since you've direct commit access as well.

Bi11 · 2017-01-17T15:38:41Z

Thank you @pornel and @zorgiepoo . And sorry for the delay, I am really busy with my work these days.

I'm downloading the old versions of Xcode (8, 8.1 and 8.2). I'll create a pull request once I've confirmed the API availability and added the forward declarations.

Bi11 added 2 commits January 8, 2017 15:17

Add Touch Bar support

d44a122

Fix UITests multiple matches issue introduced in touch bar

6902b80

kornelski reviewed Jan 8, 2017

View reviewed changes

Bi11 added 4 commits January 10, 2017 07:22

Use NSTouchBarDelegate to avoid subclassing NSTouchBar

f795802

Bind enabled and title for touch bar buttons by default

07599c2

Merge branch 1.17.x into touchbar

762d527

Use outlets instead of iterating through subviews

6e01dd2

kornelski changed the base branch from master to 1.17.x January 10, 2017 02:03

Fix default button on touch bar

b16d881

zorgiepoo requested changes Jan 10, 2017

View reviewed changes

Bi11 added 2 commits January 11, 2017 00:42

Fix compile issue with older SDK < 10.12.2

7ae0e71

Set touch bar default button directly instead of changing its keyEqui…

d95f420

…valent

Change touch bar identifier prefix to sparkle bundle identifier

6ca4b05

zorgiepoo reviewed Jan 11, 2017

View reviewed changes

Keep consistent coding style

d5f4c31

Set up touch bar in each window controller

6f11c1c

Bi11 added 2 commits January 13, 2017 01:22

Add touch bar button group view to replace NSGroupTouchBarItem

118c72e

Add constrains to set touch bar buttons width equally

e242678

zorgiepoo requested changes Jan 13, 2017

View reviewed changes

Bi11 added 2 commits January 13, 2017 20:33

Use NSView’s designated initializer

65bd11d

Merge branch '1.17.x' into touchbar

aa29a75

kornelski merged commit ecc72f8 into sparkle-project:1.17.x Jan 16, 2017


		#import "SUTouchBar.h"

		#if __MAC_OS_X_VERSION_MAX_ALLOWED >= 101202


		@synthesize buttons = _buttons;

		- (instancetype)initByReferencingButtons:(NSArray<NSButton > )buttons

Add Touch Bar Support #987

Add Touch Bar Support #987

Conversation

Bi11 commented Jan 8, 2017

kornelski commented Jan 8, 2017

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

kornelski Jan 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zorgiepoo commented Jan 8, 2017 • edited

zorgiepoo commented Jan 8, 2017

Bi11 commented Jan 8, 2017 • edited

zorgiepoo commented Jan 8, 2017 • edited

Bi11 commented Jan 9, 2017

zorgiepoo commented Jan 9, 2017

Bi11 commented Jan 10, 2017

zorgiepoo commented Jan 10, 2017

Bi11 commented Jan 10, 2017

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

zorgiepoo commented Jan 10, 2017 • edited

zorgiepoo commented Jan 10, 2017

zorgiepoo commented Jan 10, 2017 • edited

Bi11 commented Jan 10, 2017

zorgiepoo commented Jan 10, 2017 • edited

Bi11 commented Jan 10, 2017 • edited

Bi11 commented Jan 11, 2017

zorgiepoo commented Jan 11, 2017 • edited

zorgiepoo commented Jan 11, 2017

Choose a reason for hiding this comment

Bi11 commented Jan 11, 2017 • edited

Bi11 commented Jan 11, 2017

zorgiepoo commented Jan 11, 2017 • edited

Bi11 commented Jan 12, 2017

Bi11 commented Jan 12, 2017 • edited

zorgiepoo commented Jan 12, 2017

Bi11 commented Jan 12, 2017

zorgiepoo commented Jan 12, 2017 • edited

Bi11 commented Jan 12, 2017 • edited

zorgiepoo commented Jan 12, 2017

zorgiepoo commented Jan 12, 2017

Bi11 commented Jan 13, 2017 • edited

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

zorgiepoo commented Jan 13, 2017

Bi11 commented Jan 14, 2017

kornelski commented Jan 16, 2017

kornelski commented Jan 16, 2017

zorgiepoo commented Jan 16, 2017 • edited

Bi11 commented Jan 17, 2017 • edited

kornelski Jan 10, 2017 •

edited

zorgiepoo commented Jan 8, 2017 •

edited

Bi11 commented Jan 8, 2017 •

edited

zorgiepoo commented Jan 8, 2017 •

edited

zorgiepoo commented Jan 10, 2017 •

edited

zorgiepoo commented Jan 10, 2017 •

edited

zorgiepoo commented Jan 10, 2017 •

edited

Bi11 commented Jan 10, 2017 •

edited

zorgiepoo commented Jan 11, 2017 •

edited

Bi11 commented Jan 11, 2017 •

edited

zorgiepoo commented Jan 11, 2017 •

edited

Bi11 commented Jan 12, 2017 •

edited

zorgiepoo commented Jan 12, 2017 •

edited

Bi11 commented Jan 12, 2017 •

edited

Bi11 commented Jan 13, 2017 •

edited

zorgiepoo commented Jan 16, 2017 •

edited

Bi11 commented Jan 17, 2017 •

edited