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

Generate GIF without asking for location #94

Merged
merged 71 commits into from Jul 26, 2019

Conversation

@sunshinejr
Copy link
Collaborator

commented Jun 23, 2019

Fixes #72. This needs us to rebuild our current architecture to use controllers as well so it will be another big PR.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Jun 24, 2019

Keep in mind that all the stages need to be drop targets. The user should be able to drop a video at the initial view, when showing the settings, and when the conversion is completed.

@sunshinejr sunshinejr force-pushed the feature/remove_savepanel branch from e9bb15e to 185b6c6 Jun 24, 2019

@sunshinejr

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2019

False alarm, when removed the context it appeared anyways e.g. in the conversion completed view.

@sunshinejr

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2019

@sindresorhus I give up on the objc_weak_error in the console, tested the app for many days now and it doesn't seem to affect anything. I removed unpublish and I can't seem to reproduce the crash either.

I also removed some relict Save Panel hacks, fixed a bug when you selected "percent" based gif size and added focus on dimensions drop down in the Edit VC. Please take a look and see what we're missing.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Jul 18, 2019

I give up on the objc_weak_error in the console, tested the app for many days now and it doesn't seem to affect anything.

Yeah, doesn't matter if it has no effect on anything. I have a feeling it's some kind of Swift bug.

I removed unpublish and I can't seem to reproduce the crash either.

👌

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Jul 18, 2019

There's still

I noticed now that the progress circle get's less opacity/color when it shows the checkmark (before fading out). I don't think it was like this before.

and

For the conversion completed view, the buttons fade in must earlier and faster than the thumbnail. In the current Gifski version, it's better synced.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Jul 18, 2019

Quick Look is no longer working. Nothing happens when I press Space and if I select "Quick Look" in the menu, I get:

Screen Shot 2019-07-18 at 17 10 47

@sunshinejr

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2019

@sindresorhus oh interesting, do you always have this problem with QuickLook? I cannot reproduce it on my videos 🤔

@sunshinejr

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2019

Hmm, actually, I can still reproduce the crash - after a successful GIF generation, I try another one and Cancel at 4% and it always crashes. Gonna see if I can figure out why is this happening.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Jul 18, 2019

oh interesting, do you always have this problem with QuickLook? I cannot reproduce it on my videos 🤔

Tested a bit more now. It happens if the window is not focused when the "conversion completed" view is presented.

@sunshinejr

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2019

Fixed the cancel crash (I think)! So we were double cancelling and now if the second cancel was fast enough or not, the data was retained or not, and then crashed or not 😄

We were double cancelling because cancelling of the progress cancelled the generator -> thus Gifski reported cancelled state and we then cancelled again... I could've skipped the cancelling on Gifski response.cancelled, but I just added a guard to cancel only if it's not cancelled yet, because I'm not sure if there are other cases when we could get cancel event from Gifski.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Jul 18, 2019

Nice catch!

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Jul 18, 2019

I noticed one other small problem. One the conversion completed view, if I Command+H to hide the window and the show the window again, the thumbnail animation plays each time. It should only animate in when the view is presented, not when the window is made visible.

@sunshinejr

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 26, 2019

@sindresorhus

  • Conversion completed view should have better animation sync
  • the bug with second animation on minimize should be fixed
  • the bug with progress circle opacity should be fixed as well

I can't reproduce the issue with QuickLook so no idea how to approach this one...

@sindresorhus sindresorhus merged commit eb8c6cb into master Jul 26, 2019

@sindresorhus sindresorhus deleted the feature/remove_savepanel branch Jul 26, 2019

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Jul 26, 2019

Looks great now. Super nice work, @sunshinejr! Gifski is so much more usable now. 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.