search

sindresorhus / Gifski

🌈 Convert videos to high-quality GIFs on your Mac
https://sindresorhus.com/gifski
MIT License
7.72k stars 291 forks source link

Use NSViewController.present() with custom animator #108 #241

Closed hazeycode closed 3 years ago

hazeycode commented 3 years ago

While this still changes the contentViewController, presenting view controllers behave exactly like a stack. Works flawlessly (in my testing) with dismiss:

Fixes #108

IssueHunt Summary ### Referenced issues This pull request has been submitted to: - [#108: Use `NSViewController.present()` with custom animator](https://issuehunt.io/repos/119822304/issues/108) ---
sindresorhus commented 3 years ago

Thanks for working on this.

You have a lot of trailing whitespace. Make sure swiftlint passes without any warnings.

sindresorhus commented 3 years ago

Cancelling a conversion no longer correctly works:

Screen Shot 2021-05-06 at 16 14 02

Make sure you thoroughly test all possible states and scenarios.

hazeycode commented 3 years ago

Sorry about that @sindresorhus, I have fixed the cancellation state bug that I introduced and tested the conversion state cancelations and completions more thoroughly.

sindresorhus commented 3 years ago

If you drag and drop a video, then drag and drop another video onto the editor, and then press "Cancel", you end up with this:

Screen Shot 2021-05-07 at 00 18 52
sindresorhus commented 3 years ago

This may or may not be the same issue as the above: If you drag and drop a video, convert it, and on the success screen, drag and drop another video, and then press Cancel, you end up with:

Screen Shot 2021-05-07 at 00 22 44
sindresorhus commented 3 years ago

Have you tested this change on macOS 10.15 or 10.14?

hazeycode commented 3 years ago

I only have macOS version 11.2.3 for testing at this time

sindresorhus commented 3 years ago

Can you push commits instead of squashing and force pushing? Force-pushing makes it hard to follow what changed and I cannot comment on individual commits.

sindresorhus commented 3 years ago

If I drag in a video, then drag another one into the editor, the editor animates slightly in size. This is new behavior. It should only fade between the view. Try out the App Store version to see the behavior difference.

You have some swiftlint warnings.

sindresorhus commented 3 years ago

Can you use the Xcode memory graph to ensure there are no retain cycles?

https://medium.com/zendesk-engineering/ios-identifying-memory-leaks-using-the-xcode-memory-graph-debugger-e84f097b9d15

I'm a little bit paranoid that this change introduces subtle retain cycles. I spent years fixing retain cycles in Gifski.

hazeycode commented 3 years ago

I think moving the transition code out of the NSViewController extension in Utilities into a RootViewController would resolve the issues here. RootViewController would take responsibilty for the lifetimes and transitions between view controllers and would be the only window contentViewController.

sindresorhus commented 3 years ago

I think moving the transition code out of the NSViewController extension in Utilities into a RootViewController would resolve the issues here.

Why would it have to be moved out the extension? We could still have a root view controller that manages everything, while having the reusable transition logic in the extension.

sindresorhus commented 3 years ago

I'm closing this. This is taking forever and the current code works fine. This was just meant to clean up the code, but it's really not worth the risk of breaking things more. We'll eventually move the whole app to SwiftUI at some point anyway.