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

Update controller bug fixes #2402

Merged
merged 10 commits into from Nov 6, 2017
Merged

Update controller bug fixes #2402

merged 10 commits into from Nov 6, 2017

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Oct 16, 2017

This is a follow up to #1596.

I fixed the bugs I ran into while testing it out. I wanted to let @tiennou look over these changes before going to master.

On that last commit, yes we’ve got a strong reference to the update controller in the block, but

  1. It’s a singleton, so keeping a reference to it shouldn’t be a problem.
  2. I think we want it to stay around for the life of the block, so it can do what the user asks if needed.

skurfer added 6 commits Oct 16, 2017
Now: Skip update check if automatic updates are disabled and the check wasn't explicitly requested by the user.
The relaunch boolean defaults to the value of updateWithoutAsking. So, if you want updates without asking, the relaunch will take place. If not, the user will be asked if they want to relaunch.
@skurfer
Copy link
Member Author

skurfer commented Oct 16, 2017

Bah! I forgot about the autolayout exceptions being thrown. We should probably do something about that before release.

@skurfer skurfer requested a review from tiennou Oct 16, 2017
@skurfer
Copy link
Member Author

skurfer commented Oct 21, 2017

The exceptions have something to do with the NSAlert that gets passed to QSAlertManager. They’re being created on the update controller’s background thread, but then the alert manager’s main thread block sends messages to it, then it continues to exist in the background thread’s scope. (Maybe the error happens when it’s freed?)

We don’t need it to exist after the alert manager gets it, but since it isn’t declared locally with __block, we can’t do anything to it there.

Do you think we need to refactor the alert manager so it creates the NSAlert in the same block where we use it instead of allowing them to be created in other scopes? Other ideas?

@tiennou
Copy link
Member

tiennou commented Oct 22, 2017

I kinda like the API I've come up with on QSAlertManager (though I'm open to suggestions), namely the completionHandler part.

One hacky-workaround I can think of is copying the alert in +[QSAlertManager beginAlert:…] before doing the beginSheet… calls, so the actual alert we display is owned by the main-thread. Sadly NSAlert isn't -copy-able, so it would need to be done manually…

shouldInstallApp = YES;
else if (response == QSAlertResponseCancel)
shouldInstallApp = NO;
[self installAppUpdate];
Copy link
Member

@tiennou tiennou Oct 22, 2017

Choose a reason for hiding this comment

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

Heh, you managed to completely sidestep the queuing problem I had with QSURLDownload by doing that.

Note that it will move back most of the installation process back under the main thread, which will cause it to lock again, but between beachballs and crashes and rewriting QSURLDownload, I'm not sure what's best…

@tiennou
Copy link
Member

tiennou commented Oct 22, 2017

I've pushed some parts I was working on here. Of relevance are the first few commits, and the last, which has the workaround discussed above, feel free to drop the WIP one for WIPedness (but that's the start of the changes needed to QSURLDownload).

I've tested without the WIP commit, and the download started, and cancelling it worked — that's the extent I've done. HTH !

@skurfer
Copy link
Member Author

skurfer commented Oct 23, 2017

Thanks, I’ll look at those. Feel free to push things straight to this branch if they’re related.

@tiennou
Copy link
Member

tiennou commented Oct 24, 2017

As a side note, my comment on QSURLDownload above is wrong : what was beachballing were the synchronous calls the updater does when checking its status, and the network is flaky. The actual download will be done on some of NSURLDownload's queues.

@skurfer
Copy link
Member Author

skurfer commented Oct 25, 2017

Copying the alert seems to work. I had actually tried that using copy, but that obviously isn’t an option. I didn’t think of doing it the hard way.

I’m a little surprised that it works. You’re still sending messages to the original alert on the main thread to get its properties. Maybe reading properties doesn’t count as “accessing the autolayout engine”?

I’ll look at the other commits later.

@tiennou
Copy link
Member

tiennou commented Oct 25, 2017

I think that's the gist of it, yes. After the -begin… calls, the NSAlert is bumped around by the layout engine, which is what makes it fails. What completely baffled me was that the layout code actually runs in a queue when the exception happens, not on the main thread. So it's complaining about a non-main-thread-owned NSAlert while running in a non-main thread 😜 .

@pjrobertson
Copy link
Member

pjrobertson commented Oct 26, 2017

@skurfer
Copy link
Member Author

skurfer commented Oct 26, 2017

Could a notification be used instead? I have a feeling Rob's battled with them before and using them for making choices isn't great, but I thought I' suggest it just in case.

They can be suitable for offering choices, if the user has things configured for Alerts instead of Banners, but that’s unlikely since it would make every notification from QS require manual dismissal.

@pjrobertson
Copy link
Member

pjrobertson commented Oct 27, 2017

Aha, I thought there was some kind of horrible usability problem with notifications. Damn.

Could we at least replace the NSAlerts that don't need to be NSAlerts with notifications? E.g. we could replace the "You're up-to-date!" one with a notification. If there are any other ones that don't require user input then they should also be notifications.

I just checked and that'd get rid of 2 alerts: the 'You're up-to-date!' one and the 'Internet Connection Error' one.

Thoughts?

@skurfer
Copy link
Member Author

skurfer commented Nov 3, 2017

I’ve added all of the commits here except the WIP. Planing to release unless someone objects soonish. 😃

@skurfer skurfer merged commit 2a5d1ba into master Nov 6, 2017
2 checks passed
@skurfer skurfer deleted the updater branch Nov 6, 2017
skurfer added a commit that referenced this issue Nov 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants