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

Fix delta updates edge cases #1838

Merged
merged 4 commits into from
Apr 26, 2021
Merged

Fix delta updates edge cases #1838

merged 4 commits into from
Apr 26, 2021

Conversation

zorgiepoo
Copy link
Member

@zorgiepoo zorgiepoo commented Apr 25, 2021

This fixes two issues / edge cases with delta updates:

  1. -bestValidUpdateInAppcast:forUpdater: did not ever properly support delta updates. All the top-level items of the appcast are non-delta items, so -copyWithoutDeltaUpdates: didn't make sense. Instead of asking delegate of delta updates, we just ask them for regular updates, and we retrieve the delta update ourself.
  2. When resuming an update for later (instead of downloading/installing it immediately, eg for update that requires auth), we saved the appcast item, but did not save the secondary (non-delta) appcast item. So if the primary delta appcast item failed to extract, we couldn't fallback to regular update in that case. We now pass around and preserve the secondary update item 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
  • My own app
  • Other (please specify)

Tested success & failed delta update for regular installation and resumed installation (requiring auth). Tested test app works.

macOS version tested: 11.2.3 (20D91)

@kornelski
Copy link
Member

Passing it explicitly is definitely better than smuggling through another property.

However, two args seem repetitive. Maybe change SUAppcastItem arg to SUSelectedAppcastItems object with primary and secondary fields?

@zorgiepoo
Copy link
Member Author

zorgiepoo commented Apr 26, 2021

Agree but lots of refactoring and requires re-testing the changes. Will merge this for now and maybe see if I can refactor it in another change. (Consequently this may either lead to me accessing the primary item most of time or mental burden of me figuring out where to only pass the primary item, in this complex driver & delegation system : \ )

@zorgiepoo zorgiepoo merged commit 84cdb0c into 2.x Apr 26, 2021
@zorgiepoo zorgiepoo deleted the fix-delta-updates branch April 26, 2021 04:39
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