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

Conversation

Projects
None yet
2 participants
@sunshinejr
Copy link
Contributor

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

@sindresorhus sindresorhus changed the title QuickLook Add Quick Look shortcut to the conversion completed view May 8, 2019

Show resolved Hide resolved Gifski/MainWindowController.swift Outdated
Show resolved Hide resolved Gifski/MainWindowController.swift Outdated
Show resolved Hide resolved Gifski/Base.lproj/MainMenu.xib Outdated
Show resolved Hide resolved Gifski/MainWindowController.swift Outdated
Show resolved Hide resolved Gifski/MainWindowController.swift Outdated
@sindresorhus

This comment has been minimized.

Copy link
Owner

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.

@sunshinejr

This comment has been minimized.

Copy link
Contributor Author

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).

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

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

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus May 11, 2019

Owner

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).

This comment has been minimized.

Copy link
@sunshinejr

sunshinejr May 14, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@sunshinejr

sunshinejr May 18, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus May 21, 2019

Owner

@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?

This comment has been minimized.

Copy link
@sunshinejr

sunshinejr May 21, 2019

Author Contributor

@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.

Show resolved Hide resolved Gifski/ConversionCompletedView.swift
@sunshinejr

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

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

@@ -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"/>

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus May 21, 2019

Owner

Just curious, what changed here?

This comment has been minimized.

Copy link
@sunshinejr

sunshinejr May 21, 2019

Author Contributor

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

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

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented May 21, 2019

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

@sunshinejr sunshinejr deleted the sunshinejr:feature/quick_look branch May 21, 2019

@sunshinejr

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

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

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