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 cancel feature #50

Merged
merged 33 commits into from Jan 22, 2019

Conversation

Projects
None yet
3 participants
@boyvanamstel
Copy link
Collaborator

boyvanamstel commented Jan 17, 2019

Fixes #5

I experimented with a few solutions, before ending up with what I'm submitting now. (Feel free to squash the commit if that's more appropriate.)

The window maintains its minimalist look and feel until you hover over it. The cancel button fades in from the bottom, pushing the circular progress view up a bit. (That should be animating, but it's not. Wrapping the transform in a CATransaction does make it animate, but resets to its original position after it completes while autoreverses etc are not set. Ideas?)

If the mouse exits the window the cancel buttons fades out, restoring the UI.

As an expert feature, the Escape key can also be used to cancel the process.

Upon canceling the conversion the app switches back to its initial state: asking for a file to be dropped on it.

screenshot 2019-01-17 at 23 05 22

(Note: the latest master branch introduced an issue with the progress indicator. It seems to start the conversion in the background and takes quite a few moments before it starts indicating progress.)

@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 18, 2019

Thanks for working on this! 🙌

@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 18, 2019

The dock icon progress is not reset when cancelling.

Show resolved Hide resolved Gifski/Gifski.swift
Show resolved Hide resolved Gifski/MainWindowController.swift Outdated
Show resolved Hide resolved Gifski/util.swift Outdated
Show resolved Hide resolved Gifski/HoverView.swift Outdated
Show resolved Hide resolved Gifski/Gifski.swift
@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 18, 2019

What do you think about this idea for the cancel button? The idea is that the x button fades over the percentage label when you hover the window. Note that this is just a quick mockup. Might look better to have it filled, since the progress circle is already unfilled. The benefit of this is that the progress circle doesn't have to move. Having the progress circle turn into a cancel button is a common design choice. You can see this in the progress circle button when downloading a song in iTunes, for example.

gifski-cancel-button-mockup

@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 18, 2019

(Note: the latest master branch introduced an issue with the progress indicator. It seems to start the conversion in the background and takes quite a few moments before it starts indicating progress.)

I have noticed this too. I'll try to track down when it was introduced.

boyvanamstel added some commits Jan 18, 2019

Implement option to reset dock progress
I decided to keep the API consistent with the circular progress view
instead of monitoring cancellation in the dock progress indicator.
@boyvanamstel

This comment has been minimized.

Copy link
Collaborator Author

boyvanamstel commented Jan 18, 2019

The dock icon progress is not reset when cancelling.

Good catch. See commit message for reasoning behind implementation.

@boyvanamstel

This comment has been minimized.

Copy link
Collaborator Author

boyvanamstel commented Jan 18, 2019

What do you think about this idea for the cancel button? The idea is that the x button fades over the percentage label when you hover the window. Note that this is just a quick mockup. Might look better to have it filled, since the progress circle is already unfilled. The benefit of this is that the progress circle doesn't have to move. Having the progress circle turn into a cancel button is a common design choice. You can see this in the progress circle button when downloading a song in iTunes, for example.

I think that looks quite appealing. Let me see how it translates into code.

@boyvanamstel

This comment has been minimized.

Copy link
Collaborator Author

boyvanamstel commented Jan 18, 2019

circular

I think this looks pretty cool, and it works well. What do you think? :)

I've added reusable support for these types of buttons to CustomButton in the process.

@boyvanamstel

This comment has been minimized.

Copy link
Collaborator Author

boyvanamstel commented Jan 18, 2019

screenshot 2019-01-18 at 11 44 21

screenshot 2019-01-18 at 11 44 38

Reduced its size a little.

@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 18, 2019

Looks really good :)

The X looks a bit weird. It should be totally square. Either using an icon or a different font.

@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 18, 2019

Perfect! 🎉

@kornelski

This comment has been minimized.

Copy link
Collaborator

kornelski commented Jan 18, 2019

The new X is nice. It made the done checkmark look retro though, so maybe the checkmark could get an upgrade too?

@boyvanamstel

This comment has been minimized.

Copy link
Collaborator Author

boyvanamstel commented Jan 18, 2019

@kornelski 💁‍♂️

screenshot 2019-01-18 at 14 32 08

I think this one feels a lot less retro. It's not exactly in the same style as the X, but there's not that many glyphs to choose from unfortunately. Close enough, for now?

@boyvanamstel

This comment has been minimized.

Copy link
Collaborator Author

boyvanamstel commented Jan 19, 2019

Shall we merge this in @sindresorhus? 😊

@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 21, 2019

I noticed a problem. After having converted once, I can no longer drag another movie into the window.

@@ -27,20 +26,45 @@ final class MainWindowController: NSWindowController {
$0.centerInWindow(window)
}

private lazy var cancelButton = with(CustomButton.circularButton(title: "", size: 130)) {
$0.textColor = .appTheme
$0.backgroundColor = NSColor.appTheme.with(alpha: 0.1)

This comment has been minimized.

@sindresorhus

sindresorhus Jan 21, 2019

Owner

This does not update when the user changes their accent color in System Preferences. I know that sounds obscure for the user to do while Gifski is running, it's something Apple says you should handle correctly. I suggest we expose a onUpdateLayer callback on CustomButton class, which would be called in func CustomButton#updateLayer. That would be a powerful hook for users to customize behavior on layer changes.

This comment has been minimized.

@boyvanamstel

boyvanamstel Jan 21, 2019

Author Collaborator

That looks absolutely horrendous. The alpha prevents the background color from automatically updating. Should've checked that.

I agree with the updateLayer closure being a suitable workaround.

This comment has been minimized.

@sindresorhus

sindresorhus Jan 21, 2019

Owner

I really wish it was possible to create dynamic colors in user-land... So NSColor.appTheme.with(alpha: 0.1) returned a NSColor instance that updated itself, exactly as NSColor.appTheme would.

This comment has been minimized.

@boyvanamstel

boyvanamstel Jan 21, 2019

Author Collaborator

I went with a slightly different solution. Instead of providing an onUpdateLayer callback (that could cause some trouble with having to update properties that would in turn call updateLayer again), I've added a mechanism that allows you to set a 'color generator' for any of the key paths that return NSColor.

$0.setColorGenerator(for: \.backgroundColor) {
	NSColor.appTheme.with(alpha: 0.1)
}

The generator gets called in animateColor through a convenience method that returns either the generated color if it exists, or the standard property:

let backgroundColor = isOn ? color(for: \.activeBackgroundColor) : color(for: \.backgroundColor)
Show resolved Hide resolved Gifski/MainWindowController.swift
Show resolved Hide resolved Gifski/MainWindowController.swift Outdated
@boyvanamstel

This comment has been minimized.

Copy link
Collaborator Author

boyvanamstel commented Jan 21, 2019

I noticed a problem. After having converted once, I can no longer drag another movie into the window.

I might've misread this the first time. Do you mean after having cancelled, or should it also accept a new file after successfully completing a conversion?

This commit resolved starting a new conversion after cancelling one first for me: 1abb318

@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 21, 2019

should it also accept a new file after successfully completing a conversion?

Yes

@boyvanamstel

This comment has been minimized.

Copy link
Collaborator Author

boyvanamstel commented Jan 21, 2019

should it also accept a new file after successfully completing a conversion?

Yes

restart


public func setColorGenerator(for keyPath: KeyPath<CustomButton, NSColor>, handler: @escaping ColorGenerationHandler) {
colorGenerators[keyPath] = handler
}

This comment has been minimized.

@sindresorhus

sindresorhus Jan 22, 2019

Owner

What do you think about making this a setter/getter instead?

Then you could just use it like this:

// Add
$0.colorGenerator(for: \.backgroundColor) = {
	NSColor.appTheme.with(alpha: 0.1)
}

// Remove
$0.colorGenerator(for: \.backgroundColor) = nil

This comment has been minimized.

@boyvanamstel

boyvanamstel Jan 22, 2019

Author Collaborator

I've tried getting close to the syntax you came up with, but I'm not sure it's possible without adding something like $0.colorGenerator(for: \.backgroundColor).value. 🤔

I've created a PR that just makes the handler optional, allowing for $0.setColorGenerator(for: \.backgroundColor, nil).

This comment has been minimized.

@sindresorhus

sindresorhus Jan 23, 2019

Owner

Yeah, you're right. Too bad that doesn't work though. Would have been nice to have this. I should propose something like that on the Swift Forums.

Could make computed properties support arguments:

var unicorn(for: Magic): Int {
    get {  }
    set {  }
}

Used as:

.unicorn(for: .rainbow) = 2

Thoughts on this?

This comment has been minimized.

@boyvanamstel

boyvanamstel Jan 23, 2019

Author Collaborator

It's similar to the subscript solution you posted here: #54 (comment)

With the added benefit that the syntax looks more in line with what you'd expect. Would be nice to have.

I can see how this would be unusual for other reasons though. There's no single variable that it points too. There should always be another (private) property (or multiple properties/values) that it interacts with. I think that's what the subscript syntax solves.

@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 22, 2019

Very cool idea to use KeyPaths for this. I would never have thought of that.

@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 22, 2019

I also just discovered that it's possible to subclass NSColor: https://pfandrade.me/blog/adopting-dark-mode-and-older-macs/ Although, I have not been able to figure out how to do it in Swift and make it work with NSColor.white.with(alpha: 0.5). Would be super cool to make that work at some point though.

@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 22, 2019

And also, please do open issues here if you see anything that could be improved in the app or code, even if you don't intend to fix it yourself.

@sindresorhus sindresorhus merged commit 17ef52d into sindresorhus:master Jan 22, 2019

@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 22, 2019

Thank you so much for working on this, @boyvanamstel. It turned out really great! 🙌

@boyvanamstel

This comment has been minimized.

Copy link
Collaborator Author

boyvanamstel commented Jan 22, 2019

Thanks @sindresorhus! I really enjoyed working on this. 🙌

@boyvanamstel

This comment has been minimized.

Copy link
Collaborator Author

boyvanamstel commented Jan 22, 2019

I also just discovered that it's possible to subclass NSColor: https://pfandrade.me/blog/adopting-dark-mode-and-older-macs/ Although, I have not been able to figure out how to do it in Swift and make it work with NSColor.white.with(alpha: 0.5). Would be super cool to make that work at some point though.

Interesting, that might be worth looking into.

@boyvanamstel boyvanamstel deleted the boyvanamstel:cancel branch Jan 22, 2019

@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Jan 22, 2019

@boyvanamstel Any thoughts on #50 (comment) ?

@boyvanamstel

This comment has been minimized.

Copy link
Collaborator Author

boyvanamstel commented Jan 22, 2019

@boyvanamstel Any thoughts on #50 (comment) ?

Looking into that as we speak.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment