Update RBLClipView to use a display link for animated scrolling #64

Closed
wants to merge 13 commits into
from

Projects

None yet

4 participants

@jwilling
Contributor

This implements animated scrolling of the clip view when triggered by keyboard presses and explicit scrollRectToVisible:animated: calls.

Contributor

Can you explain a bit more what this is actually for? Why would a scroll wheel need deceleration?

Contributor

Sorry, I should have been more clear. This isn't for a trackpad or a scroll wheel. This only kicks into action when the current event is from a keyboard (e.g. arrow down), or if the user requests an animated scrollRectToVisible:animated:.

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
//
#import "RBLClipView.h"
#import "NSColor+RBLCGColorAdditions.h"
+const CGFloat RBLClipViewDecelerationRate = 0.88;
+
+@interface RBLClipView()
+@property (nonatomic) CVDisplayLinkRef displayLink;
+@property (nonatomic) BOOL animate;
+@property (nonatomic) CGPoint destination;
+@property (nonatomic, readonly, getter = isScrolling) BOOL scrolling;
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Please document and add memory management attributes to all of these properties.

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
return self;
}
+- (void)dealloc {
+ CVDisplayLinkRelease(_displayLink);
+ [[NSNotificationCenter defaultCenter] removeObserver:self];
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Please use dot syntax here.

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
return self;
}
+- (void)dealloc {
+ CVDisplayLinkRelease(_displayLink);
+ [[NSNotificationCenter defaultCenter] removeObserver:self];
+}
+
+
+#pragma mark Display link
+
+static CVReturn RBLScrollingCallback(CVDisplayLinkRef displayLink, const CVTimeStamp* now, const CVTimeStamp* outputTime, CVOptionFlags flagsIn, CVOptionFlags* flagsOut, void* displayLinkContext) {
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Asterisks for pointers should be associated with the variable, not the type (e.g., const CVTimeStamp *now).

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+ CVDisplayLinkRelease(_displayLink);
+ [[NSNotificationCenter defaultCenter] removeObserver:self];
+}
+
+
+#pragma mark Display link
+
+static CVReturn RBLScrollingCallback(CVDisplayLinkRef displayLink, const CVTimeStamp* now, const CVTimeStamp* outputTime, CVOptionFlags flagsIn, CVOptionFlags* flagsOut, void* displayLinkContext) {
+ __block CVReturn status;
+ @autoreleasepool {
+ RBLClipView *clipView = (__bridge id)displayLinkContext;
+ dispatch_async(dispatch_get_main_queue(), ^{
+ status = [clipView updateOrigin];
+ });
+ }
+ return status;
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Incorrect indentation here.

@jspahrsummers jspahrsummers and 1 other commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
return self;
}
+- (void)dealloc {
+ CVDisplayLinkRelease(_displayLink);
+ [[NSNotificationCenter defaultCenter] removeObserver:self];
+}
+
+
+#pragma mark Display link
+
+static CVReturn RBLScrollingCallback(CVDisplayLinkRef displayLink, const CVTimeStamp* now, const CVTimeStamp* outputTime, CVOptionFlags flagsIn, CVOptionFlags* flagsOut, void* displayLinkContext) {
+ __block CVReturn status;
+ @autoreleasepool {
+ RBLClipView *clipView = (__bridge id)displayLinkContext;
+ dispatch_async(dispatch_get_main_queue(), ^{
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

If this is asynchronous, status won't be updated before this function returns.

On the other hand, it seems silly to block here.

jwilling
jwilling Jan 16, 2013 Contributor

This is one area I wasn't entirely sure about. What would you like me to do here?

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+
+static CVReturn RBLScrollingCallback(CVDisplayLinkRef displayLink, const CVTimeStamp* now, const CVTimeStamp* outputTime, CVOptionFlags flagsIn, CVOptionFlags* flagsOut, void* displayLinkContext) {
+ __block CVReturn status;
+ @autoreleasepool {
+ RBLClipView *clipView = (__bridge id)displayLinkContext;
+ dispatch_async(dispatch_get_main_queue(), ^{
+ status = [clipView updateOrigin];
+ });
+ }
+ return status;
+}
+
+- (CVDisplayLinkRef)displayLink {
+ if (_displayLink == NULL) {
+ CVDisplayLinkCreateWithActiveCGDisplays(&_displayLink);
+ CVDisplayLinkSetOutputCallback(_displayLink, &RBLScrollingCallback, (__bridge void *)(self));
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

No parentheses needed around the value being casted.

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+ __block CVReturn status;
+ @autoreleasepool {
+ RBLClipView *clipView = (__bridge id)displayLinkContext;
+ dispatch_async(dispatch_get_main_queue(), ^{
+ status = [clipView updateOrigin];
+ });
+ }
+ return status;
+}
+
+- (CVDisplayLinkRef)displayLink {
+ if (_displayLink == NULL) {
+ CVDisplayLinkCreateWithActiveCGDisplays(&_displayLink);
+ CVDisplayLinkSetOutputCallback(_displayLink, &RBLScrollingCallback, (__bridge void *)(self));
+ [self updateCVDisplay];
+ }
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Please add a blank line after this. Vertical whitespace is cheap.

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+ return status;
+}
+
+- (CVDisplayLinkRef)displayLink {
+ if (_displayLink == NULL) {
+ CVDisplayLinkCreateWithActiveCGDisplays(&_displayLink);
+ CVDisplayLinkSetOutputCallback(_displayLink, &RBLScrollingCallback, (__bridge void *)(self));
+ [self updateCVDisplay];
+ }
+ return _displayLink;
+}
+
+- (void)updateCVDisplay {
+ NSScreen *screen = self.window.screen;
+ if (screen) {
+ NSDictionary* screenDictionary = [[NSScreen mainScreen] deviceDescription];
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Style: asterisk, dot syntax

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+}
+
+- (CVDisplayLinkRef)displayLink {
+ if (_displayLink == NULL) {
+ CVDisplayLinkCreateWithActiveCGDisplays(&_displayLink);
+ CVDisplayLinkSetOutputCallback(_displayLink, &RBLScrollingCallback, (__bridge void *)(self));
+ [self updateCVDisplay];
+ }
+ return _displayLink;
+}
+
+- (void)updateCVDisplay {
+ NSScreen *screen = self.window.screen;
+ if (screen) {
+ NSDictionary* screenDictionary = [[NSScreen mainScreen] deviceDescription];
+ NSNumber *screenID = [screenDictionary objectForKey:@"NSScreenNumber"];
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Style: subscripting

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+ }
+ return status;
+}
+
+- (CVDisplayLinkRef)displayLink {
+ if (_displayLink == NULL) {
+ CVDisplayLinkCreateWithActiveCGDisplays(&_displayLink);
+ CVDisplayLinkSetOutputCallback(_displayLink, &RBLScrollingCallback, (__bridge void *)(self));
+ [self updateCVDisplay];
+ }
+ return _displayLink;
+}
+
+- (void)updateCVDisplay {
+ NSScreen *screen = self.window.screen;
+ if (screen) {
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Style: explicit comparison against nil

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+
+- (CVDisplayLinkRef)displayLink {
+ if (_displayLink == NULL) {
+ CVDisplayLinkCreateWithActiveCGDisplays(&_displayLink);
+ CVDisplayLinkSetOutputCallback(_displayLink, &RBLScrollingCallback, (__bridge void *)(self));
+ [self updateCVDisplay];
+ }
+ return _displayLink;
+}
+
+- (void)updateCVDisplay {
+ NSScreen *screen = self.window.screen;
+ if (screen) {
+ NSDictionary* screenDictionary = [[NSScreen mainScreen] deviceDescription];
+ NSNumber *screenID = [screenDictionary objectForKey:@"NSScreenNumber"];
+ CGDirectDisplayID displayID = [screenID unsignedIntValue];
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Style: dot syntax

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+ status = [clipView updateOrigin];
+ });
+ }
+ return status;
+}
+
+- (CVDisplayLinkRef)displayLink {
+ if (_displayLink == NULL) {
+ CVDisplayLinkCreateWithActiveCGDisplays(&_displayLink);
+ CVDisplayLinkSetOutputCallback(_displayLink, &RBLScrollingCallback, (__bridge void *)(self));
+ [self updateCVDisplay];
+ }
+ return _displayLink;
+}
+
+- (void)updateCVDisplay {
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

This method should be invoked any time the clip view moves between windows.

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+ if (self.animate && (self.window.currentEvent.type != NSScrollWheel)) {
+ self.destination = newOrigin;
+ [self beginScrolling];
+ } else {
+ [self endScrolling];
+ [super scrollToPoint:newOrigin];
+ }
+}
+
+- (BOOL)scrollRectToVisible:(NSRect)aRect animated:(BOOL)animated {
+ self.animate = animated;
+ return [super scrollRectToVisible:aRect];
+}
+
+- (void)beginScrolling {
+ if (self.isScrolling) {
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Style: dot syntax should not use is getters.

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
//
#import "RBLClipView.h"
#import "NSColor+RBLCGColorAdditions.h"
+const CGFloat RBLClipViewDecelerationRate = 0.88;
+
+@interface RBLClipView()
+@property (nonatomic) CVDisplayLinkRef displayLink;
+@property (nonatomic) BOOL animate;
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Can you please give this property a clearer name?

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
//
#import "RBLClipView.h"
#import "NSColor+RBLCGColorAdditions.h"
+const CGFloat RBLClipViewDecelerationRate = 0.88;
+
+@interface RBLClipView()
+@property (nonatomic) CVDisplayLinkRef displayLink;
+@property (nonatomic) BOOL animate;
+@property (nonatomic) CGPoint destination;
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Can you please give this property a clearer name?

@jspahrsummers jspahrsummers and 1 other commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+ NSNumber *screenID = [screenDictionary objectForKey:@"NSScreenNumber"];
+ CGDirectDisplayID displayID = [screenID unsignedIntValue];
+ CVDisplayLinkSetCurrentCGDisplay(_displayLink, displayID);
+ } else {
+ CVDisplayLinkSetCurrentCGDisplay(_displayLink, kCGDirectMainDisplay);
+ }
+}
+
+#pragma mark Scrolling
+
+- (void)scrollToPoint:(NSPoint)newOrigin {
+ if (self.animate && (self.window.currentEvent.type != NSScrollWheel)) {
+ self.destination = newOrigin;
+ [self beginScrolling];
+ } else {
+ [self endScrolling];
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

This will invoke endScrolling if the event came from a scroll wheel. Is that correct behavior?

jwilling
jwilling Jan 15, 2013 Contributor

Yes, because if the display link is still animating a deceleration and the user suddenly scrolls with the mouse/trackpad, we want to immediately stop any further scrolling done by the display link.

Even if we aren't scrolling, it's cheap to call this since we're just doing an early return in endScrolling.

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+
+- (BOOL)scrollRectToVisible:(NSRect)aRect animated:(BOOL)animated {
+ self.animate = animated;
+ return [super scrollRectToVisible:aRect];
+}
+
+- (void)beginScrolling {
+ if (self.isScrolling) {
+ return;
+ }
+
+ CVDisplayLinkStart(self.displayLink);
+}
+
+- (void)endScrolling {
+ if (!self.isScrolling)
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Style: braces, method without the is

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+}
+
+- (void)endScrolling {
+ if (!self.isScrolling)
+ return;
+
+ CVDisplayLinkStop(self.displayLink);
+ self.animate = NO;
+}
+
+- (BOOL)isScrolling {
+ return CVDisplayLinkIsRunning(self.displayLink);
+}
+
+- (CVReturn)updateOrigin {
+ if(self.window == nil) {
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Style: space after if

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+}
+
+- (BOOL)isScrolling {
+ return CVDisplayLinkIsRunning(self.displayLink);
+}
+
+- (CVReturn)updateOrigin {
+ if(self.window == nil) {
+ [self endScrolling];
+ return kCVReturnError;
+ }
+
+ CGPoint o = self.bounds.origin;
+ CGPoint lastOrigin = o;
+ o.x = o.x * RBLClipViewDecelerationRate + self.destination.x * (1 - RBLClipViewDecelerationRate);
+ o.y = o.y * RBLClipViewDecelerationRate + self.destination.y * (1 - RBLClipViewDecelerationRate);
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Can you please document the intended animation curve here?

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+- (BOOL)isScrolling {
+ return CVDisplayLinkIsRunning(self.displayLink);
+}
+
+- (CVReturn)updateOrigin {
+ if(self.window == nil) {
+ [self endScrolling];
+ return kCVReturnError;
+ }
+
+ CGPoint o = self.bounds.origin;
+ CGPoint lastOrigin = o;
+ o.x = o.x * RBLClipViewDecelerationRate + self.destination.x * (1 - RBLClipViewDecelerationRate);
+ o.y = o.y * RBLClipViewDecelerationRate + self.destination.y * (1 - RBLClipViewDecelerationRate);
+
+ [self setBoundsOrigin:o];
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Style: dot syntax

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+}
+
+- (CVReturn)updateOrigin {
+ if(self.window == nil) {
+ [self endScrolling];
+ return kCVReturnError;
+ }
+
+ CGPoint o = self.bounds.origin;
+ CGPoint lastOrigin = o;
+ o.x = o.x * RBLClipViewDecelerationRate + self.destination.x * (1 - RBLClipViewDecelerationRate);
+ o.y = o.y * RBLClipViewDecelerationRate + self.destination.y * (1 - RBLClipViewDecelerationRate);
+
+ [self setBoundsOrigin:o];
+
+ if((fabs(o.x - lastOrigin.x) < 0.1) && (fabs(o.y - lastOrigin.y) < 0.1)) {
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Style: space after if, parentheses around each part of the && unnecessary

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+- (CVReturn)updateOrigin {
+ if(self.window == nil) {
+ [self endScrolling];
+ return kCVReturnError;
+ }
+
+ CGPoint o = self.bounds.origin;
+ CGPoint lastOrigin = o;
+ o.x = o.x * RBLClipViewDecelerationRate + self.destination.x * (1 - RBLClipViewDecelerationRate);
+ o.y = o.y * RBLClipViewDecelerationRate + self.destination.y * (1 - RBLClipViewDecelerationRate);
+
+ [self setBoundsOrigin:o];
+
+ if((fabs(o.x - lastOrigin.x) < 0.1) && (fabs(o.y - lastOrigin.y) < 0.1)) {
+ [self endScrolling];
+ [self setBoundsOrigin:self.destination];
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

Style: dot syntax

@jspahrsummers jspahrsummers commented on an outdated diff Jan 15, 2013
Rebel/RBLClipView.m
+ if(self.window == nil) {
+ [self endScrolling];
+ return kCVReturnError;
+ }
+
+ CGPoint o = self.bounds.origin;
+ CGPoint lastOrigin = o;
+ o.x = o.x * RBLClipViewDecelerationRate + self.destination.x * (1 - RBLClipViewDecelerationRate);
+ o.y = o.y * RBLClipViewDecelerationRate + self.destination.y * (1 - RBLClipViewDecelerationRate);
+
+ [self setBoundsOrigin:o];
+
+ if((fabs(o.x - lastOrigin.x) < 0.1) && (fabs(o.y - lastOrigin.y) < 0.1)) {
+ [self endScrolling];
+ [self setBoundsOrigin:self.destination];
+ [(NSScrollView *)self.superview flashScrollers];
jspahrsummers
jspahrsummers Jan 15, 2013 Contributor

This should use -[NSView enclosingScrollView].

Contributor

Sorry, I should have been more clear. This isn't for a trackpad or a scroll wheel. This only kicks into action when the current event is from a keyboard (e.g. arrow down), or if the user requests an animated scrollRectToVisible:animated:.

Can you please document this in the interface, so it's clear to others as well?


Made a lot of style notes here. It may be helpful to review our Objective-C conventions.

@dannygreg @joshaber @alanjrogers Anyone else want to take a look at this?

Contributor

Apologies for the style issues, a lot of this was written before the conventions were released, and I did not go back and change them.

Contributor

I need to look into this some more and figure out exactly when a good time is for the scrolling to be enabled. I'll reopen once I feel it's ready for re-review.

@jwilling jwilling closed this Jan 15, 2013
@jwilling jwilling reopened this Jan 16, 2013
Contributor

Hopefully I didn't miss anything there. Please let me know if the documentation isn't explicit enough!

@jspahrsummers jspahrsummers commented on an outdated diff Jan 16, 2013
Rebel/RBLClipView.m
//
#import "RBLClipView.h"
#import "NSColor+RBLCGColorAdditions.h"
+const CGFloat RBLClipViewDecelerationRate = 0.88;
jspahrsummers
jspahrsummers Jan 16, 2013 Contributor

This should be static if it's not exported.

@jspahrsummers jspahrsummers commented on an outdated diff Jan 16, 2013
Rebel/RBLClipView.m
//
#import "RBLClipView.h"
#import "NSColor+RBLCGColorAdditions.h"
+const CGFloat RBLClipViewDecelerationRate = 0.88;
+
+@interface RBLClipView()
jspahrsummers
jspahrsummers Jan 16, 2013 Contributor

Style: RBLClipView ()

Can you please document all the properties in this interface?

@jspahrsummers jspahrsummers and 1 other commented on an outdated diff Jan 16, 2013
Rebel/RBLClipView.m
+- (void)dealloc {
+ CVDisplayLinkRelease(_displayLink);
+ [NSNotificationCenter.defaultCenter removeObserver:self];
+}
+
+#pragma mark View Heirarchy
+
+- (void)viewWillMoveToWindow:(NSWindow *)newWindow {
+ if (self.window != nil) {
+ [NSNotificationCenter.defaultCenter removeObserver:self name:NSWindowDidChangeScreenNotification object:self.window];
+ }
+
+ [super viewWillMoveToWindow:newWindow];
+
+ if (newWindow != nil) {
+ [NSNotificationCenter.defaultCenter addObserverForName:NSWindowDidChangeScreenNotification object:newWindow queue:nil usingBlock:^(NSNotification *note) {
jspahrsummers
jspahrsummers Jan 16, 2013 Contributor

The return value of this method needs to be saved if the observer is to be removed later. (removeObserver:name:object: doesn't include block observers by default.)

It'd be easier to just register a selector instead.

jwilling
jwilling Jan 16, 2013 Contributor

Thanks, I'll do that.

@jspahrsummers jspahrsummers and 1 other commented on an outdated diff Jan 16, 2013
Rebel/RBLClipView.m
+ [super viewWillMoveToWindow:newWindow];
+
+ if (newWindow != nil) {
+ [NSNotificationCenter.defaultCenter addObserverForName:NSWindowDidChangeScreenNotification object:newWindow queue:nil usingBlock:^(NSNotification *note) {
+ [self updateCVDisplay];
+ }];
+ }
+}
+
+#pragma mark Display link
+
+static CVReturn RBLScrollingCallback(CVDisplayLinkRef displayLink, const CVTimeStamp *now, const CVTimeStamp *outputTime, CVOptionFlags flagsIn, CVOptionFlags *flagsOut, void *displayLinkContext) {
+ __block CVReturn status;
+ @autoreleasepool {
+ RBLClipView *clipView = (__bridge id)displayLinkContext;
+ dispatch_async(dispatch_get_main_queue(), ^{
jspahrsummers
jspahrsummers Jan 16, 2013 Contributor

This still has the issue of asynchrony that I mentioned before (status not guaranteed to be set before returning).

jwilling
jwilling Jan 16, 2013 Contributor

So we turn this into sync, or just always return success by default? Not sure what you want me to do here.

jspahrsummers
jspahrsummers Jan 16, 2013 Contributor

Well, it's your implementation, so I thought you might have opinions on which is more correct. I guess returning success would make the most sense, so it doesn't choke the display link with a synchronous dispatch.

I'm also not clear what the error return code even does, since it doesn't seem to be documented.

jwilling
jwilling Jan 16, 2013 Contributor

I'm not entirely sure either. I think returning success will be fine.

@jspahrsummers jspahrsummers commented on an outdated diff Jan 16, 2013
Rebel/RBLClipView.m
+}
+
+- (CVDisplayLinkRef)displayLink {
+ if (_displayLink == NULL) {
+ CVDisplayLinkCreateWithActiveCGDisplays(&_displayLink);
+ CVDisplayLinkSetOutputCallback(_displayLink, &RBLScrollingCallback, (__bridge void *)self);
+ [self updateCVDisplay];
+ }
+
+ return _displayLink;
+}
+
+- (void)updateCVDisplay {
+ NSScreen *screen = self.window.screen;
+ if (screen == nil) {
+ NSDictionary *screenDictionary = [NSScreen.mainScreen deviceDescription];
jspahrsummers
jspahrsummers Jan 16, 2013 Contributor

Style: dot syntax

Contributor

⭐️

Contributor

One thing I had hoped to fix here was that currently the scrollers are just flashed after the animation completes. What would be ideal is if the scrollers show up constantly through the whole animation. I've attempted to fix this, but I cannot find a way. Either I'm missing something obvious, or this is just a shortcoming with NSScrollView that just can't be fixed.

@jspahrsummers jspahrsummers commented on an outdated diff Jan 16, 2013
Rebel/RBLClipView.m
//
#import "RBLClipView.h"
#import "NSColor+RBLCGColorAdditions.h"
+// The deceleration constant used for the ease-out curve in the animation.
+static const CGFloat RBLClipViewDecelerationRate = 0.88;
+
+@interface RBLClipView ()
+// Used to drive the animation through repeated callbacks.
+// A display link is used instead of a timer so that we don't get dropped frames and tearing.
+@property (nonatomic, assign) CVDisplayLinkRef displayLink;
jspahrsummers
jspahrsummers Jan 16, 2013 Contributor

Can you please document the memory management semantics of this property as well?

@jspahrsummers jspahrsummers and 1 other commented on an outdated diff Jan 16, 2013
Rebel/RBLClipView.m
+- (void)dealloc {
+ CVDisplayLinkRelease(_displayLink);
+ [NSNotificationCenter.defaultCenter removeObserver:self];
+}
+
+#pragma mark View Heirarchy
+
+- (void)viewWillMoveToWindow:(NSWindow *)newWindow {
+ if (self.window != nil) {
+ [NSNotificationCenter.defaultCenter removeObserver:self name:NSWindowDidChangeScreenNotification object:self.window];
+ }
+
+ [super viewWillMoveToWindow:newWindow];
+
+ if (newWindow != nil) {
+ [NSNotificationCenter.defaultCenter addObserver:self selector:@selector(updateCVDisplay) name:NSWindowDidChangeScreenNotification object:newWindow];
jspahrsummers
jspahrsummers Jan 16, 2013 Contributor

Notification selectors must accept exactly one argument of type NSNotification. Even if this works, it can't be depended upon.

jwilling
jwilling Jan 16, 2013 Contributor

😳

Contributor

@github/Mac Can someone else please take a look at this too?

Contributor

I've noticed that we're setting up the CVDisplayLink for the window's display device. We should tear down and recreate the display link in response to a - (void)windowDidChangeScreen:(NSNotification *)notification.

Contributor

@alanjrogers That does happen, but it uses the notification instead of the method.

Contributor

@jspahrsummers duh, I somehow missed that completely. Carry on, nothing to see here. 🍔

Contributor

🌔

I've tested this in a couple of apps and it doesn't seem to have any adverse affects, but there's always a possibility I'm missing something big here. I don't want this to break GHfM, either.

Contributor
jwilling commented Feb 2, 2013

Just a gentle bump here. I know you all are quite busy so take your time, but there are a couple things depending on this at the moment and it would be relatively nice to get this merged in when possible.

Contributor
jwilling commented Feb 2, 2013

Actually I take that back for now, I'd welcome some feedback on the state of this PR but I think I'm going to rewrite the deceleration formula to not use the logic from TwUI. I've gotten feedback from at least two people that it scrolls a little bit too slowly, and given the way it's written there's not a good way to change it without changing the feeling of the scroll. As a result, I would like to change the deceleration logic to create an interpolation curve based off of a duration instead of a distance.

Contributor

This must've slipped off my radar – sorry about that. Just let me know when you're doing with your refactoring and I'll take another look ASAP.

Contributor

I feel terrible about leaving this open for so long. Finally gave this some thought and concluded it'd just be better to make the deceleration rate a property, with a default that is slightly lower (meaning faster animations).

Thoughts on this are welcome. Also, thanks for your patience on the style issues earlier, @jspahrsummers.

I have one concern here, TwUI's deceleration is wrong in almost all cases. Scrolling a TUITableView for example, uses all the wrong rates and is way slower than scrolling an NSTableView.

AppKit has also clearly made the decision that it should not do any kind of deceleration when scrolling in cells, particularly when acting from keyborad input. I just spent a lot of time studying how Mail.app reacts to key presses in it's tableview in order to match it up with something I'm working on. It achieves a much more responsive feel by not adding any animation sugar, such as easing, of any kind. It in fact feels much "smoother" as a result.

I vote we leave this up to AppKit.

Contributor

@dannygreg I understand your reasoning here, but there is a problem with AppKit. I don't know if you've ever set your key repeat rate to high, but there are quite a few people that do have that setting on. If the repeat rate is high and you hold down an arrow key, things start to blow up. I'd encourage you to check it out in any app that uses a standard NSTableView.

Also, setting the deceleration to 1 (or 0) will achieve the exact effect as Mail, if you so desire.

@jwilling I am aware what you're referring to, we have a fix that I will PR. It has nothing to do with deceleration. The fix is to override "scroll to visible" to not try and scroll things into the middle of the view. Instead you make them scroll the least amount possible. Visibly "pinning" the selection to the top/bottom of the table.

That's what mail.app is doing and what we do in a forthcoming version of GitHub for Mac.

Contributor

@dannygreg What I meant was that the same effect you're talking about is possible by setting the deceleration in this implementation to 1 or 0.

Contributor

In any case, if this just doesn't work for you all that's okay, I'll just maintain my own fork of Rebel that has this capability.

I'm 👎 because it changes the visual appearance when there is another satisfactory fix. I'm really keen to leave stuff like that to AppKit, to avoid that feeling of it not being "quite right" that you get with TwUI and the like.

That's only me though, would be interested to hear @joshaber/@jspahrsummers/@alanjrogers' thoughts.

Contributor

@dannygreg's point is fair. Any AppKit changes made by Rebel should be consistent with the "default" UX, whatever that means.

@jwilling I'm gonna close this out – sorry about that. Hopefully the code review was helpful, though!

There is a valid problem with NSTableView's default selection behaviour though. I must remember to PR the fix @mdiep found.

Contributor
jwilling commented Apr 1, 2013

For what it's worth, here is a comparison of the three different ways of scrolling, vanilla NSClipView included. This isn't a NSTableView, it's my own scratch-built table view. The lag is screen-recording induced.

http://appjon.com/drop/scrolling_comparisons.mov

Contributor

@jwilling is that demo code available somewhere?

Contributor
jwilling commented Apr 1, 2013

@alanjrogers This was a weekend project for me but it's turned into something bigger, so I plan on releasing it in (hopefully) the near future. If it would help you out to test things right now, just let me know and I can send it over.

Contributor

@jwilling I was mostly just curious to try it locally, it's hard to judge the 'feel' from the video. But I do agree with @jspahrsummers and @dannygreg comments about Rebel not messing with the default OS X UX. It might be worth filing a radar with your demo app though, as it looks like a valid issue.

Contributor
jwilling commented Apr 2, 2013

@alanjrogers Here is the demo with the custom scrolling behavior. There's no way to toggle it off, but you can compare it against Mail.app, for example. http://appjon.com/drop/files-A89Rx0Zy9S.zip

It's pretty clear to me: there should be no easing when a scroll is done with anything other than a trackpad. I would reason that they came to the decision because there is no "momentum" unless you are using a trackpad/mouse. The metaphor completely breaks down.

I do think scrollToVisible: should be altered to match Mail/GitHub for Mac though.

@jwilling jwilling referenced this pull request in ButterKit/Butter Apr 16, 2014
Closed

Why aren't all of these enhancements part of Rebel? #15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment