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/port update relaunched API #1891

Closed
wants to merge 2 commits into from
Closed

Add/port update relaunched API #1891

wants to merge 2 commits into from

Conversation

zorgiepoo
Copy link
Member

@zorgiepoo zorgiepoo commented Jul 12, 2021

Add/port update relaunched API from #1115.

This inserts an environment variable on relaunch rather than mutating the host bundle defaults and favors exposing a class property in 2.x over exposing a delegate call in 1.x

I decided to port the ability to detect this as there may be legitimate use cases, but with a doc note that it should not be used in cases where manual updates must be handled.

Removed some dated __has_feature(objc_generics) checks I noticed too.

Checklist:

  • My change is being tested and reviewed against the Sparkle 2.x branch. New changes must be developed on the 2.x development branch first.
  • My change is being backported to master branch (Sparkle 1.x). Please create a separate pull request for 1.x, should it be backported. Note 1.x is feature frozen and is only taking bug fixes, localization updates, and critical OS adoption enhancements.
  • I have reviewed and commented my code, particularly in hard-to-understand areas.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • My change is or requires a documentation or localization update

Testing

I tested and verified my change by using one or multiple of these methods:

  • Sparkle Test App
  • Unit Tests (UI tests in this case)
  • My own app
  • Other (please specify)

Adjust UI test to take this API in account, tested it (and SUUpdater stub) with the test app.

macOS version tested: 11.5 Beta (20G5042c)

@kornelski
Copy link
Member

mainBundleRelaunchedFromUpdate is a footgun. Could it be kept private?

Or alternatively, make it always work even if relaunch wasn't from Sparkle by storing last known application version in nsuserdefaults and notifying when it changes.

@zorgiepoo
Copy link
Member Author

zorgiepoo commented Jul 13, 2021

Or alternatively, make it always work even if relaunch wasn't from Sparkle by storing last known application version in nsuserdefaults and notifying when it changes.

At this point, I don't think this is Sparkle's duty. On second thought though, this sounds like a right approach for an app to handle themselves. I was considering adopting this because 1.x had support for it but I think I'm going to end up declining this. The way 1.x mutates a marker in the user defaults has its own set of issues (sandboxing if set/read are from different components, failure between set/read, external updater overriding values..) so I don't want Sparkle to maintain responsibility in such a way.

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.

2 participants