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

Add Quick Look shortcut to the conversion completed view #79

Merged
merged 12 commits into from May 21, 2019
Merged

Add Quick Look shortcut to the conversion completed view #79

merged 12 commits into from May 21, 2019

Conversation

@sunshinejr
Copy link
Collaborator

@sunshinejr sunshinejr commented May 7, 2019

Resolves #66.

Hey @sindresorhus, first of all thanks for this awesome tool. I'm using it on a daily basis and finally I'm able to help out. I'm gonna do more improvements some time in the future as well.

This PR introduces QuickLook addition and fixes one crash (I've added a comment on the line). See some screenshots below and play with the branch yourself!

Also, let me know what documentation would you like me to add in this PR as well.

QuickLook disabled as there is no GIF yet

Zrzut ekranu 2019-05-7 o 12 33 52

QuickLook disabled as the GIF is converting still

Zrzut ekranu 2019-05-7 o 12 34 14

QuickLook enabled as we finished converting

Zrzut ekranu 2019-05-7 o 12 34 37

QuickLook opened

Zrzut ekranu 2019-05-7 o 12 35 43

Gifski/MainWindowController.swift Outdated Show resolved Hide resolved
Loading
@sindresorhus sindresorhus changed the title QuickLook Add Quick Look shortcut to the conversion completed view May 8, 2019
Gifski/MainWindowController.swift Outdated Show resolved Hide resolved
Loading
Gifski/MainWindowController.swift Outdated Show resolved Hide resolved
Loading
Gifski/Base.lproj/MainMenu.xib Outdated Show resolved Hide resolved
Loading
Gifski/MainWindowController.swift Outdated Show resolved Hide resolved
Loading
Gifski/MainWindowController.swift Outdated Show resolved Hide resolved
Loading
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented May 8, 2019

Do you know any apps other than Finder that has a Quick Look menu item? I'm curious where they place it.

Loading

@sunshinejr
Copy link
Collaborator Author

@sunshinejr sunshinejr commented May 8, 2019

@sindresorhus Hmm, not yet, I was trying to copy what Finder is using (without using CMD + Y for the preview shortcut).

Loading

Gifski/MainWindowController.swift Outdated Show resolved Hide resolved
Loading
Gifski/ConversionCompletedView.swift Show resolved Hide resolved
Loading
Gifski/ConversionCompletedView.swift Outdated Show resolved Hide resolved
Loading
@@ -301,6 +305,14 @@ final class MainWindowController: NSWindowController {
}
}

@IBAction private func quickLook(_ sender: Any) {
Copy link
Owner

@sindresorhus sindresorhus May 11, 2019

Choose a reason for hiding this comment

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

Instead of having this here to merely forward the call. I was thinking we instead only have func quickLook(_ sender: Any) { in ConversionCompletedView and somehow inject ConversionCompletedView into the responder chain so ConversionCompletedView can respond directly to the quickLook message. That way we wouldn't need https://github.com/sindresorhus/gifski-app/pull/79/files#diff-53ab4064437ff40d094178968ddb4d1fR335 either. (You might have to change the view to be removed/added instead of shown/hidden so it doesn't respond when it's not visible. I'm not sure whether it would respond if it's hidden or if there's anyway to prevent that).

Loading

Copy link
Collaborator Author

@sunshinejr sunshinejr May 14, 2019

Choose a reason for hiding this comment

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

Yeah, the problem with responder chain is currently beyond me. I think we would need a controller, but I don't have the knowledge needed to make the connection between the window and a conversion view in a responder chain. I would use some help here.

Loading

Copy link
Collaborator Author

@sunshinejr sunshinejr May 18, 2019

Choose a reason for hiding this comment

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

Alright, spent the day on learning how does exactly responder chain work in AppKit and fixed the problem with a proper solution.

Loading

Copy link
Owner

@sindresorhus sindresorhus May 21, 2019

Choose a reason for hiding this comment

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

@sunshinejr Awesome! This is exactly what I conceptually had in mind, but I didn't know how to do it in practice either.

Any recommended reading material?

Loading

Copy link
Collaborator Author

@sunshinejr sunshinejr May 21, 2019

Choose a reason for hiding this comment

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

@sindresorhus I had lots of problems as some of the methods of the responder chain in AppKit are different than the ones in UIKit (e.g. becomeFirstResponder() is quite the opposite, lol). But yeah basically reading the docs of responder chain on apple doc archive (this one for instance).

I also played with all the properties to make sure I understand the differences (e.g. initialFirstResponder vs firstResponder etc).

I have lots of material that I want to convert into a blog post as I didn't found one that goes into what I experienced while implementing this one.

Loading

Gifski/ConversionCompletedView.swift Show resolved Hide resolved
Loading
@sunshinejr
Copy link
Collaborator Author

@sunshinejr sunshinejr commented May 18, 2019

@sindresorhus fixed the problem with responder chain, ready for another review 👍

Loading

@@ -70,7 +70,7 @@
<menuItem title="Quick Look" keyEquivalent=" " id="Wfh-bM-KvX">
<modifierMask key="keyEquivalentModifierMask"/>
<connections>
<action selector="quickLook:" target="-1" id="Zr4-ny-Pim"/>
<action selector="quickLook:" target="-1" id="IQy-U1-llq"/>
Copy link
Owner

@sindresorhus sindresorhus May 21, 2019

Choose a reason for hiding this comment

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

Just curious, what changed here?

Loading

Copy link
Collaborator Author

@sunshinejr sunshinejr May 21, 2019

Choose a reason for hiding this comment

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

Unpinned & pinned the action again and so the id regenerated 🤕

Loading

@sindresorhus sindresorhus merged commit ece132d into sindresorhus:master May 21, 2019
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented May 21, 2019

This looks great! @sunshinejr Thanks for taking the time to do this feature. Really nice work 🙌

Loading

@sunshinejr sunshinejr deleted the feature/quick_look branch May 21, 2019
@sunshinejr
Copy link
Collaborator Author

@sunshinejr sunshinejr commented May 21, 2019

thanks @sindresorhus, a pleasure to contribute to such a great project 😉

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants