Skip to content
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

AppKitNavigation - Common #209

Merged

Conversation

Mx-Iris
Copy link
Contributor

@Mx-Iris Mx-Iris commented Aug 19, 2024

This is some basic components, with UIKit is basically the same, I think the back can be AppKit animation and UIKit animation together, AppKit animation generally use a certain property of the animator trigger, for example, view.animator().alphaValue = 1

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Mx-Iris! Thanks for starting with a smaller step.

I think this looks mostly good, just a few things that can be cleaned up and removed for now. Wanna take a pass and ping us when it's ready for another review?

Sources/AppKitNavigation/Internal/AssumeIsolated.swift Outdated Show resolved Hide resolved
Sources/AppKitNavigation/Internal/ErrorMechanism.swift Outdated Show resolved Hide resolved
Sources/AppKitNavigation/Internal/ToOptionalUnit.swift Outdated Show resolved Hide resolved
Package.swift Outdated Show resolved Hide resolved
Package.swift Outdated Show resolved Hide resolved
[email protected] Outdated Show resolved Hide resolved
[email protected] Outdated Show resolved Hide resolved
[email protected] Outdated Show resolved Hide resolved
Sources/AppKitNavigation/AppKitAnimation.swift Outdated Show resolved Hide resolved
Sources/AppKitNavigation/Internal/AssociatedKeys.swift Outdated Show resolved Hide resolved
@Mx-Iris
Copy link
Contributor Author

Mx-Iris commented Aug 19, 2024

@stephencelis I understand, moving them into Swift Navigation is a good idea. I have read all your comments, and I will recommit them once I have time tomorrow. It’s already late at night here. Thank you for your patience in reviewing.

@stephencelis
Copy link
Member

Thanks! No rush!

Oh, and your comment here:

I think the back can be AppKit animation and UIKit animation together, AppKit animation generally use a certain property of the animator trigger, for example, view.animator().alphaValue = 1

I think property animators may not support the new spring animations UIKit added support for in the most recent UIView.animate function. I could be wrong, though! I didn't dig too deeply into the problem.

@Mx-Iris
Copy link
Contributor Author

Mx-Iris commented Aug 20, 2024

@stephencelis The code is ready. You can look at it again.

@Mx-Iris
Copy link
Contributor Author

Mx-Iris commented Aug 20, 2024

I think property animators may not support the new spring animations UIKit added support for in the most recent UIView.animate function. I could be wrong, though! I didn't dig too deeply into the problem.

You're right, AppKit's animations aren't as rich as UIKit's, and Apple hasn't added more APIs, so let's keep it that way

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing that! I think there are just 2 more files to delete and this should be good to go!

Sources/AppKitNavigationShim/include/shim.h Outdated Show resolved Hide resolved
Sources/AppKitNavigationShim/shim.m Outdated Show resolved Hide resolved
@Mx-Iris
Copy link
Contributor Author

Mx-Iris commented Aug 20, 2024

It's done.

@Mx-Iris
Copy link
Contributor Author

Mx-Iris commented Aug 22, 2024

@stephencelis Hey bro,still here?

@acosmicflamingo
Copy link
Contributor

@Mx-Iris that's one way of saying "Just a friendly ping"

@stephencelis
Copy link
Member

@Mx-Iris Your experiment has made us reconsider some of the internals, so we've been hard at work making observe and UITransaction more extensible. See this PR for more details: #212

If you give @mbrandonw and me write access to your fork we can try incorporating the changes once we merge. We're trying to be mindful and careful as we incorporate a new platform like AppKit into the library, so it's going to take us some time to work out some of these new questions and details that come up.

Thanks for your patience, though! We're excited for what you've accomplished so far!

@Mx-Iris
Copy link
Contributor Author

Mx-Iris commented Aug 22, 2024

@stephencelis Oh, that's great, I've sent you and @mbrandonw an invite.

@Mx-Iris
Copy link
Contributor Author

Mx-Iris commented Aug 22, 2024

@Mx-Iris that's one way of saying "Just a friendly ping"

☺️ Yes, that's what it means

@stephencelis
Copy link
Member

@Mx-Iris Pushed some changes and will discuss with @mbrandonw tomorrow or Monday. Let me know if things look good to you or if I messed anything up 😅

@Mx-Iris
Copy link
Contributor Author

Mx-Iris commented Aug 23, 2024

@stephencelis It's okay. No problem.

Comment on lines +36 to +38
context.allowsImplicitAnimation = true
context.duration = animation.duration
context.timingFunction = animation.timingFunction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mx-Iris I've familiarized myself a bit more with this API by adding a configurable timing function, and it also occurred to me that because withAppKitAnimation is intended to be called from the model, where view.animator is not available, that it probably makes sense to always set allowsImplicitAnimation to true. Does this sound right to you?

Copy link
Contributor Author

@Mx-Iris Mx-Iris Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's right. NSView/NSWindow implements the NSAnimatablePropertyContainer protocol. When the view's frame, size, or position changes, it can produce animated effects. Through its animator, these properties can be used transparently to generate animations.

Setting NSAnimationContext's allowsImplicitAnimation to true, directly modifying the view's properties will produce animations. This is kind of like UIKit's UIView.animate method. But only the frame, frameSize, and frameOrigin properties are supported. More property animations require a layer-backed view, that is, the view's wantsLayer = true.

Package.swift Outdated Show resolved Hide resolved
[email protected] Outdated Show resolved Hide resolved
@stephencelis stephencelis merged commit b4c658a into pointfreeco:main Aug 26, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants