Animated transition in/out of Quick View #13

jtbandes · 2015-09-01T09:09:10Z

As Quick View feels a lot like Quick Look, I thought it would be nice to have some smooth transitions. (It would be nice for the title bar to fade at the same time, but I didn't do that in this commit.)

Use 2 snapshot layers to animate the transition in/out of Quick View
Rename Document's _containerView to _mapContainerView, to make its purpose more evident
Link QuartzCore.framework

swisspol · 2015-09-01T15:10:30Z

GitUp/Application/Document.m

@@ -880,11 +886,11 @@ - (void)_showHelpWithIdentifier:(NSString*)identifier {
    XLOG_DEBUG_UNREACHABLE();
  }
  if (showHelp) {
-    NSRect contentBounds = [_mainWindow.contentView bounds];


I had problems with NSRect rect = view.bounds that doesn't compile in Xcode 6 or 7, I don't remember, so you have to do do [view bounds] instead.

I tried 6 and 7 and it seems to work.

window.contentView.bounds didn't work because contentView was id until the 10.11 SDK (when it becomes __kindof NSView*).

swisspol · 2015-09-01T15:38:08Z

Looks pretty good, that's a well written patch! 😄

Some high-level comments:

Something like this, which is a "real" contribution, cannot land until I finish http://forums.gitup.co/t/understanding-the-gitup-open-source-license/317
If this transition becomes animated, then the window toolbar contents, which also changes, should be animated as well: otherwise it just looks weird
GitUp is compatible with 10.8 and 10.9: does it work there as well?

The most important factor here is that GitUp does not animate its UI on purpose: it's all about speed and not having superfluous / distracting UI niceties.

Although you've made Quick View look nicer, it's definitely slower and therefore less responsive. I'm not fundamentally against such "enhancements" but they would need to be thought-through and most likely be disabled by default: maybe a "Enable superfluous animations" preference?

swisspol · 2015-09-01T15:59:57Z

GitUpKit/Views/GIMapViewController.h

@@ -37,6 +37,7 @@
 @property(nonatomic) BOOL forceShowAllTips;
 - (BOOL)selectCommit:(GCCommit*)commit;  // Also scrolls if needed to ensure commit is visible - Returns YES if commit was selected
 - (GINode*)nodeForCommit:(GCCommit*)commit;
+- (NSPoint)positionForCommit:(GCHistoryCommit*)historyCommit;


This should take a GCCommit for consistency with the other APIs and I think the name should reflect better what's the coordinate system e.g. -positionInViewForCommit:.

jtbandes · 2015-09-01T17:19:47Z

the window toolbar contents, which also changes, should be animated as well

Agreed; I didn't do it in this commit but I can add another.

GitUp is compatible with 10.8 and 10.9: does it work there as well?

Do you have recommendations for how to test this? The machines I have access to daily are 10.10 and 10.11 only.

As for a preference, I thought about that too. Something akin to OS X's

Happy to add that in another commit.

swisspol · 2015-09-01T17:21:47Z

When I (rarely) need to double-check that something works on older OSes, I use Parallels with 10.8 and 10.9 virtual machines. You can easily get the OS images if you are an Apple registered developer.

swisspol · 2015-09-01T17:23:00Z

For the preference, I think it would be OFF by default and since the animations should be OFF by default, that means its wording should be like "Enable visual effects".

jtbandes · 2015-09-02T08:22:50Z

Took a bit of effort to make them run in a VM, but I got there eventually 😃

After a "fun" debugging session I determined that the GIDualSplitView implementation of splitView:resizeSubviewsWithOldSize: is causing some perpetual layout-dirtying (because it calls setPosition:...) which makes it hang while trying to commit a transaction on 10.8. Since there's no comment it's hard to tell what this method is trying to achieve anyway. Any thoughts for an alternate implementation? Can this just be implemented with a holding priority? (It's hard to tell exactly what it's trying to do with no comment 😬)

swisspol · 2015-09-02T15:23:05Z

I thought about it afterwards, but I'm pretty sure you can also use the free Virtual Box to test with older OS X versions.

I wouldn't worry about 10.8 if things don't work there, just disable the code path entirely so restore the exact original behavior:

if (kCFCoreFoundationVersionNumber >= kCFCoreFoundationVersionNumber10_9) {
  // do animation
}

Regarding splitView:resizeSubviewsWithOldSize:, I don't recall exactly but it was working around some AppKit bug or issue - and yes, I should have written a comment!

jtbandes · 2015-09-03T04:30:26Z

Found it 😉 http://stackoverflow.com/a/30494691

jtbandes · 2015-09-03T06:40:14Z

Animating toolbar contents will be hard (lack of public API to get at the toolbar's view; and the fact that implicit animations don't work when hidden is involved). I think it's best left as a future improvement.

swisspol · 2015-09-03T14:42:56Z

Good find WRT to the splitview 😉 and glad you were able to find a workaround for 10.8!

swisspol · 2015-09-03T15:01:42Z

I think it's best left as a future improvement.

Well, that's an unfortunate tricky situation then. You have a very nice patch for sure, but the fact remains that from a UX perspective it's half-done because of the toolbar. I'd like to see this landing in some form, but it can't right now.

Can you think of any other way to smooth the toolbar change of appearance?

jtbandes · 2015-09-04T05:17:14Z

Turns out it wasn't as hard as I envisioned 😃

swisspol · 2015-09-04T05:54:02Z

Nice! What about 10.8 too? 😄

swisspol · 2015-09-04T05:55:25Z

The pref commit needs an updated title and it should be before the 2 animation commits.

jtbandes · 2015-09-04T06:15:59Z

Yep, tested in 10.8 and 10.9. Updating the commits now...

swisspol · 2015-09-04T19:23:04Z

I just tested the final version and it looks pretty sweet! Great work!

One minor thing: the status text rendering changes briefly during the animation:

Normal:

During the animation:

Looks like the text labels become their own layer and subpixel font smoothing gets disabled or something.

swisspol · 2015-09-04T19:52:09Z

Also when you have a minute, please accept the new GitUp CLA at http://forums.gitup.co/t/gitup-contributor-license-agreement/323.

swisspol · 2015-09-05T18:57:31Z

This is all ready to land now. The very last thing is that text rendering I was pointing out? Do you have an idea what is causing this and if it can be avoided? If not, I don't have a problem landing it as-is.

jtbandes · 2015-09-05T19:19:08Z

Yes, I modified the commit to include a fix: jtbandes@fd4681c#diff-9640dac2231f3c7e87bbda41062cc757R289

I noticed there's a similar issue with the "Show" button, but it looks like it's already rendering incorrectly (no subpixel AA) in the release version of GitUp. For some reason, during the animation it does have subpixel AA. I don't think there's a similar fix since there is no drawsBackground property on that button. Also, Interface Builder lists the Bevel button style under "Discouraged Styles", so I'm inclined to think it may be somewhat unsupported...

Animated transition in/out of Quick View and added visual effects preference

swisspol · 2015-09-05T20:02:20Z

All right, it's merged, congrats!

jtbandes · 2015-09-05T20:06:40Z

🎊

rudolf-adamkovic · 2015-09-08T08:51:07Z

Looks awesome, thanks @jtbandes!

jtbandes force-pushed the anim branch from 36b885e to 26a8891 Compare September 1, 2015 09:57

swisspol added the enhancement label Sep 1, 2015

swisspol reviewed Sep 1, 2015
View reviewed changes

jtbandes force-pushed the anim branch from 26a8891 to 0b620cb Compare September 3, 2015 06:37

jtbandes force-pushed the anim branch 2 times, most recently from bc169d9 to 76d78a8 Compare September 4, 2015 05:15

jtbandes force-pushed the anim branch from 76d78a8 to 4713420 Compare September 4, 2015 05:21

jtbandes force-pushed the anim branch 2 times, most recently from c46e687 to e56bdea Compare September 4, 2015 06:38

jtbandes added 5 commits September 4, 2015 21:17

Rename Document’s containerView to mapContainerView

48543f5

Changed GIDualSplitView implementation to play nice with Core Animation

c72fe8e

Added “Enable visual effects” bool preference

445dcc6

Animated transition in/out of Quick View

fd4681c

Animated title/toolbar transition alongside Quick View

f43d3ea

jtbandes force-pushed the anim branch from e56bdea to f43d3ea Compare September 5, 2015 06:11

swisspol added a commit that referenced this pull request Sep 5, 2015

Merge pull request #13 from jtbandes/anim

08f7aa2

Animated transition in/out of Quick View and added visual effects preference

swisspol merged commit 08f7aa2 into git-up:master Sep 5, 2015

jtbandes deleted the anim branch September 5, 2015 20:11

tgunr mentioned this pull request Jul 28, 2016

Added check for _lastHEADBranch to be not NSNull. #182

Closed

swisspol removed bug enhancement invalid labels Jun 13, 2017

Animated transition in/out of Quick View #13

Animated transition in/out of Quick View #13

Uh oh!

Conversation

jtbandes commented Sep 1, 2015

Uh oh!

swisspol Sep 1, 2015

Choose a reason for hiding this comment

Uh oh!

jtbandes Sep 1, 2015

Choose a reason for hiding this comment

Uh oh!

swisspol commented Sep 1, 2015

Uh oh!

swisspol Sep 1, 2015

Choose a reason for hiding this comment

Uh oh!

jtbandes commented Sep 1, 2015

Uh oh!

swisspol commented Sep 1, 2015

Uh oh!

swisspol commented Sep 1, 2015

Uh oh!

jtbandes commented Sep 2, 2015

Uh oh!

swisspol commented Sep 2, 2015

Uh oh!

jtbandes commented Sep 3, 2015

Uh oh!

jtbandes commented Sep 3, 2015

Uh oh!

swisspol commented Sep 3, 2015

Uh oh!

swisspol commented Sep 3, 2015

Uh oh!

jtbandes commented Sep 4, 2015

Uh oh!

swisspol commented Sep 4, 2015

Uh oh!

swisspol commented Sep 4, 2015

Uh oh!

jtbandes commented Sep 4, 2015

Uh oh!

swisspol commented Sep 4, 2015

Uh oh!

swisspol commented Sep 4, 2015

Uh oh!

swisspol commented Sep 5, 2015

Uh oh!

jtbandes commented Sep 5, 2015

Uh oh!

swisspol commented Sep 5, 2015

Uh oh!

jtbandes commented Sep 5, 2015

Uh oh!

rudolf-adamkovic commented Sep 8, 2015

Uh oh!

Uh oh!