Fix unsteady progress when installing updates (#2072)

Also add -showInstallingUpdateWithApplicationTerminated: to SPUUserDriver which replaces -showInstallingUpdate and -showSendingTerminationSignal
sparkle-project · Jan 22, 2022 · fa9bfc1 · fa9bfc1
1 parent 05abb2f
commit fa9bfc1
Show file tree

Hide file tree

Showing 10 changed files with 86 additions and 104 deletions.
diff --git a/Autoupdate/SUDiskImageUnarchiver.m b/Autoupdate/SUDiskImageUnarchiver.m
@@ -191,13 +191,13 @@ - (void)extractDMGWithNotifier:(SUUnarchiverNotifier *)notifier
             NSString *fromPath = [mountPoint stringByAppendingPathComponent:item];
             NSString *toPath = [[self.archivePath stringByDeletingLastPathComponent] stringByAppendingPathComponent:item];
 
-            // We skip any files in the DMG which are not readable.
+            itemsCopied += 1.0;
+            [notifier notifyProgress:0.5 + itemsCopied/(totalItems*2.0)];
+
+            // We skip any files in the DMG which are not readable but include the item in the progress
             if (![manager isReadableFileAtPath:fromPath]) {
                 continue;
             }
-
-            itemsCopied += 1.0;
-            [notifier notifyProgress:0.5 + itemsCopied/(totalItems*2.0)];
 
             SULog(SULogLevelDefault, @"copyItemAtPath:%@ toPath:%@", fromPath, toPath);
 

diff --git a/Sparkle/SPUCoreBasedUpdateDriver.h b/Sparkle/SPUCoreBasedUpdateDriver.h
@@ -38,12 +38,10 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (void)coreDriverDidStartExtractingUpdate;
 
-- (void)installerDidStartInstalling;
+- (void)installerDidStartInstallingWithApplicationTerminated:(BOOL)applicationTerminated;
 
 - (void)installerDidExtractUpdateWithProgress:(double)progress;
 
-- (void)installerIsSendingAppTerminationSignal;
-
 - (void)installerDidFinishInstallationAndRelaunched:(BOOL)relaunched acknowledgement:(void(^)(void))acknowledgement;
 
 @end

diff --git a/Sparkle/SPUCoreBasedUpdateDriver.m b/Sparkle/SPUCoreBasedUpdateDriver.m
@@ -261,10 +261,10 @@ - (void)downloadDriverDidFailToDownloadFileWithError:(NSError *)error
     [self.delegate coreDriverIsRequestingAbortUpdateWithError:error];
 }
 
-- (void)installerDidStartInstalling
+- (void)installerDidStartInstallingWithApplicationTerminated:(BOOL)applicationTerminated
 {
-    if ([self.delegate respondsToSelector:@selector(installerDidStartInstalling)]) {
-        [self.delegate installerDidStartInstalling];
+    if ([self.delegate respondsToSelector:@selector(installerDidStartInstallingWithApplicationTerminated:)]) {
+        [self.delegate installerDidStartInstallingWithApplicationTerminated:applicationTerminated];
     }
 }
 
@@ -324,14 +324,6 @@ - (void)installerDidFinishInstallationAndRelaunched:(BOOL)relaunched acknowledge
     }
 }
 
-- (void)installerIsSendingAppTerminationSignal
-{
-    // If they don't respond or do anything, we'll just install after the user terminates the app anyway
-    if ([self.delegate respondsToSelector:@selector(installerIsSendingAppTerminationSignal)]) {
-        [self.delegate installerIsSendingAppTerminationSignal];
-    }
-}
-
 - (void)installerIsRequestingAbortInstallWithError:(nullable NSError *)error
 {
     [self.delegate coreDriverIsRequestingAbortUpdateWithError:error];

diff --git a/Sparkle/SPUInstallerDriver.h b/Sparkle/SPUInstallerDriver.h
@@ -15,11 +15,10 @@ NS_ASSUME_NONNULL_BEGIN
 
 @protocol SPUInstallerDriverDelegate <NSObject>
 
-- (void)installerDidStartInstalling;
+- (void)installerDidStartInstallingWithApplicationTerminated:(BOOL)applicationTerminated;
 - (void)installerDidStartExtracting;
 - (void)installerDidExtractUpdateWithProgress:(double)progress;
 - (void)installerDidFinishPreparationAndWillInstallImmediately:(BOOL)willInstallImmediately silently:(BOOL)willInstallSilently;
-- (void)installerIsSendingAppTerminationSignal;
 - (void)installerWillFinishInstallationAndRelaunch:(BOOL)relaunch;
 - (void)installerDidFinishInstallationAndRelaunched:(BOOL)relaunch acknowledgement:(void(^)(void))acknowledgement;
 

diff --git a/Sparkle/SPUInstallerDriver.m b/Sparkle/SPUInstallerDriver.m
@@ -43,7 +43,6 @@ @interface SPUInstallerDriver () <SUInstallerCommunicationProtocol>
 @property (nonatomic, readonly) NSBundle *applicationBundle;
 @property (nonatomic, weak, readonly) id<SPUInstallerDriverDelegate> delegate;
 @property (nonatomic) SPUInstallerMessageType currentStage;
-@property (nonatomic) BOOL startedInstalling;
 
 @property (nonatomic) id<SUInstallerConnectionProtocol> installerConnection;
 
@@ -70,7 +69,6 @@ @implementation SPUInstallerDriver
 @synthesize applicationBundle = _applicationBundle;
 @synthesize delegate = _delegate;
 @synthesize currentStage = _currentStage;
-@synthesize startedInstalling = _startedInstalling;
 @synthesize installerConnection = _installerConnection;
 @synthesize extractionAttempts = _extractionAttempts;
 @synthesize postponedOnce = _postponedOnce;
@@ -278,9 +276,6 @@ - (void)_handleMessageWithIdentifier:(int32_t)identifier data:(NSData *)data
         self.currentStage = identifier;
     } else if (identifier == SPUInstallationStartedStage1) {
         self.currentStage = identifier;
-        [self.delegate installerDidStartInstalling];
-        self.startedInstalling = YES;
-
     } else if (identifier == SPUInstallationFinishedStage1) {
         self.currentStage = identifier;
 
@@ -308,22 +303,14 @@ - (void)_handleMessageWithIdentifier:(int32_t)identifier data:(NSData *)data
     } else if (identifier == SPUInstallationFinishedStage2) {
         self.currentStage = identifier;
 
-        if (!self.startedInstalling) {
-            // It's possible we can start from resuming to stage 2 rather than doing stage 1 again, so we should notify to start installing if we haven't done so already
-            self.startedInstalling = YES;
-            [self.delegate installerDidStartInstalling];
-        }
-
         BOOL hasTargetTerminated = NO;
         if (data.length >= sizeof(uint8_t)) {
             hasTargetTerminated = (BOOL)*((const uint8_t *)data.bytes);
         }
 
         [self.delegate installerWillFinishInstallationAndRelaunch:self.relaunch];
 
-        if (!hasTargetTerminated) {
-            [self.delegate installerIsSendingAppTerminationSignal];
-        }
+        [self.delegate installerDidStartInstallingWithApplicationTerminated:hasTargetTerminated];
     } else if (identifier == SPUInstallationFinishedStage3) {
         self.currentStage = identifier;
 

diff --git a/Sparkle/SPUStandardUserDriver.m b/Sparkle/SPUStandardUserDriver.m
@@ -42,6 +42,9 @@ @interface SPUStandardUserDriver ()
 @property (nonatomic) SUStatusController *statusController;
 @property (nonatomic) SUUpdatePermissionPrompt *permissionPrompt;
 
+@property (nonatomic) uint64_t expectedContentLength;
+@property (nonatomic) uint64_t bytesDownloaded;
+
 @end
 
 @implementation SPUStandardUserDriver
@@ -56,6 +59,8 @@ @implementation SPUStandardUserDriver
 @synthesize activeUpdateAlert = _activeUpdateAlert;
 @synthesize statusController = _statusController;
 @synthesize permissionPrompt = _permissionPrompt;
+@synthesize expectedContentLength = _expectedContentLength;
+@synthesize bytesDownloaded = _bytesDownloaded;
 
 #pragma mark Birth
 
@@ -366,8 +371,12 @@ - (void)showDownloadInitiatedWithCancellation:(void (^)(void))cancellation
     self.cancellation = cancellation;
 
     [self createAndShowStatusController];
-    [self.statusController beginActionWithTitle:SULocalizedString(@"Downloading update...", @"Take care not to overflow the status window.") maxProgressValue:0.0 statusText:nil];
+
+    [self.statusController beginActionWithTitle:SULocalizedString(@"Downloading update...", @"Take care not to overflow the status window.") maxProgressValue:1.0 statusText:nil];
+    [self.statusController setProgressValue:0.0];
     [self.statusController setButtonTitle:SULocalizedString(@"Cancel", nil) target:self action:@selector(cancelDownload:) isDefault:NO];
+
+    self.bytesDownloaded = 0;
 }
 
 - (void)cancelDownload:(id)__unused sender
@@ -382,7 +391,10 @@ - (void)showDownloadDidReceiveExpectedContentLength:(uint64_t)expectedContentLen
 {
     assert(NSThread.isMainThread);
 
-    [self.statusController setMaxProgressValue:expectedContentLength];
+    self.expectedContentLength = expectedContentLength;
+    if (expectedContentLength == 0) {
+        [self.statusController setMaxProgressValue:0.0];
+    }
 }
 
 - (NSString *)localizedStringFromByteCount:(long long)value
@@ -419,18 +431,17 @@ - (void)showDownloadDidReceiveDataOfLength:(uint64_t)length
 {
     assert(NSThread.isMainThread);
 
-    double newProgressValue = [self.statusController progressValue] + (double)length;
+    self.bytesDownloaded += length;
 
-    // In case our expected content length was incorrect
-    if (newProgressValue > [self.statusController maxProgressValue]) {
-        [self.statusController setMaxProgressValue:newProgressValue];
+    if (self.expectedContentLength > 0.0) {
+        double newProgressValue = (double)self.bytesDownloaded / (double)self.expectedContentLength;
+
+        [self.statusController setProgressValue:MIN(newProgressValue, 1.0)];
+
+        [self.statusController setStatusText:[NSString stringWithFormat:SULocalizedString(@"%@ of %@", nil), [self localizedStringFromByteCount:(long long)self.bytesDownloaded], [self localizedStringFromByteCount:(long long)MAX(self.bytesDownloaded, self.expectedContentLength)]]];
+    } else {
+        [self.statusController setStatusText:[NSString stringWithFormat:SULocalizedString(@"%@ downloaded", nil), [self localizedStringFromByteCount:(long long)self.bytesDownloaded]]];
     }
-
-    [self.statusController setProgressValue:newProgressValue];
-    if ([self.statusController maxProgressValue] > 0.0)
-        [self.statusController setStatusText:[NSString stringWithFormat:SULocalizedString(@"%@ of %@", nil), [self localizedStringFromByteCount:(long long)self.statusController.progressValue], [self localizedStringFromByteCount:(long long)self.statusController.maxProgressValue]]];
-    else
-        [self.statusController setStatusText:[NSString stringWithFormat:SULocalizedString(@"%@ downloaded", nil), [self localizedStringFromByteCount:(long long)self.statusController.progressValue]]];
 }
 
 - (void)showDownloadDidStartExtractingUpdate
@@ -440,7 +451,8 @@ - (void)showDownloadDidStartExtractingUpdate
     self.cancellation = nil;
 
     [self createAndShowStatusController];
-    [self.statusController beginActionWithTitle:SULocalizedString(@"Extracting update...", @"Take care not to overflow the status window.") maxProgressValue:0.0 statusText:nil];
+    [self.statusController beginActionWithTitle:SULocalizedString(@"Extracting update...", @"Take care not to overflow the status window.") maxProgressValue:1.0 statusText:nil];
+    [self.statusController setProgressValue:0.0];
     [self.statusController setButtonTitle:SULocalizedString(@"Cancel", nil) target:nil action:nil isDefault:NO];
     [self.statusController setButtonEnabled:NO];
 }
@@ -449,18 +461,23 @@ - (void)showExtractionReceivedProgress:(double)progress
 {
     assert(NSThread.isMainThread);
 
-    if ([self.statusController maxProgressValue] == 0.0) {
-        [self.statusController setMaxProgressValue:1];
-    }
     [self.statusController setProgressValue:progress];
 }
 
-- (void)showInstallingUpdate
+- (void)showInstallingUpdateWithApplicationTerminated:(BOOL)applicationTerminated
 {
     assert(NSThread.isMainThread);
 
-    [self.statusController beginActionWithTitle:SULocalizedString(@"Installing update...", @"Take care not to overflow the status window.") maxProgressValue:0.0 statusText:nil];
-    [self.statusController setButtonEnabled:NO];
+    if (applicationTerminated) {
+        [self.statusController beginActionWithTitle:SULocalizedString(@"Installing update...", @"Take care not to overflow the status window.") maxProgressValue:0.0 statusText:nil];
+        [self.statusController setButtonEnabled:NO];
+    } else {
+        // The "quit" event can always be canceled or delayed by the application we're updating
+        // So we can't easily predict how long the installation will take or if it won't happen right away
+        // We close our status window because we don't want it persisting for too long and have it obscure other windows
+        [self.statusController close];
+        self.statusController = nil;
+    }
 }
 
 - (void)showUpdateInstalledAndRelaunched:(BOOL)relaunched acknowledgement:(void (^)(void))acknowledgement
@@ -504,17 +521,6 @@ - (void)showUpdateInstalledAndRelaunched:(BOOL)relaunched acknowledgement:(void
 
 #pragma mark Aborting Everything
 
-- (void)showSendingTerminationSignal
-{
-    assert(NSThread.isMainThread);
-
-    // The "quit" event can always be canceled or delayed by the application we're updating
-    // So we can't easily predict how long the installation will take or if it won't happen right away
-    // We close our status window because we don't want it persisting for too long and have it obscure other windows
-    [self.statusController close];
-    self.statusController = nil;
-}
-
 - (void)dismissUpdateInstallation
 {
     assert(NSThread.isMainThread);

diff --git a/Sparkle/SPUUIBasedUpdateDriver.m b/Sparkle/SPUUIBasedUpdateDriver.m
@@ -310,9 +310,24 @@ - (void)coreDriverDidStartExtractingUpdate
     [self.userDriver showDownloadDidStartExtractingUpdate];
 }
 
-- (void)installerDidStartInstalling
+- (void)installerDidStartInstallingWithApplicationTerminated:(BOOL)applicationTerminated
 {
-    [self.userDriver showInstallingUpdate];
+    if ([self.userDriver respondsToSelector:@selector(showInstallingUpdateWithApplicationTerminated:)]) {
+        [self.userDriver showInstallingUpdateWithApplicationTerminated:applicationTerminated];
+    } else {
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+        if ([self.userDriver respondsToSelector:@selector(showInstallingUpdate)]) {
+            [self.userDriver showInstallingUpdate];
+        }
+
+        if (!applicationTerminated) {
+            if ([self.userDriver respondsToSelector:@selector(showSendingTerminationSignal)]) {
+                [self.userDriver showSendingTerminationSignal];
+            }
+        }
+#pragma clang diagnostic pop
+    }
 }
 
 - (void)installerDidExtractUpdateWithProgress:(double)progress
@@ -331,11 +346,6 @@ - (void)installerDidFinishPreparationAndWillInstallImmediately:(BOOL)willInstall
     }
 }
 
-- (void)installerIsSendingAppTerminationSignal
-{
-    [self.userDriver showSendingTerminationSignal];
-}
-
 - (void)installerDidFinishInstallationAndRelaunched:(BOOL)relaunched acknowledgement:(void(^)(void))acknowledgement
 {
     if ([self.userDriver respondsToSelector:@selector(showUpdateInstalledAndRelaunched:acknowledgement:)]) {

diff --git a/Sparkle/SPUUserDriver.h b/Sparkle/SPUUserDriver.h
@@ -197,15 +197,6 @@ SU_EXPORT @protocol SPUUserDriver <NSObject>
  */
 - (void)showExtractionReceivedProgress:(double)progress;
 
-/**
- * Show the user that the update is installing
- *
- * Let the user know that the update is currently installing. Sparkle uses this to show an indeterminate progress bar.
- *
- * Before this point, `-showExtractionReceivedProgress:` may be called.
- */
-- (void)showInstallingUpdate;
-
 /**
  * Show the user that the update is ready to install & relaunch
  *
@@ -218,24 +209,25 @@ SU_EXPORT @protocol SPUUserDriver <NSObject>
  *
  * A reply of `SPUUserUpdateChoiceSkip` cancels the current update that has begun installing and dismisses the update. In this circumstance, the update is canceled but this update version is not skipped in the future.
  *
- * Before this point, `-showInstallingUpdate` will be called.
+ * Before this point, `-showExtractionReceivedProgress:` or  `-showUpdateFoundWithAppcastItem:state:reply:` may be called.
  *
  * @param reply The reply which indicates if the update should be installed, dismissed, or skipped. See above discussion for more details.
  */
 - (void)showReadyToInstallAndRelaunch:(void (^)(SPUUserUpdateChoice))reply;
 
 /**
- * Show or dismiss progress while a termination signal is being sent to the application from Sparkle's installer
+ * Show the user that the update is installing
  *
- * Terminating and relaunching the application (if requested to be relaunched) may happen quickly,
- * or it may take some time to perform the final installation, or the termination signal can be canceled or delayed by the application or user.
+ * Let the user know that the update is currently installing.
  *
- * It is up to the implementor whether or not to decide to continue showing installation progress
- * or dismissing UI that won't remain obscuring other parts of the user interface.
+ * Before this point, `-showReadyToInstallAndRelaunch:` or  `-showUpdateFoundWithAppcastItem:state:reply:` will be called.
  *
- * This will not be invoked if the application that is being updated is already terminated.
+ * @param applicationTerminated Indicates if the application has been terminated already.
+ * If the application hasn't been terminated, a quit event is sent to the running application before installing the update.
+ * If the application or user delays or cancels termination, there may be an indefinite period of time before the application fully quits.
+ * It is up to the implementor whether or not to decide to continue showing installation progress in this case.
  */
-- (void)showSendingTerminationSignal;
+- (void)showInstallingUpdateWithApplicationTerminated:(BOOL)applicationTerminated;
 
 /**
  * Show the user that the update installation finished
@@ -246,7 +238,7 @@ SU_EXPORT @protocol SPUUserDriver <NSObject>
  * the updater's lifetime is tied to the application it is updating. This implementation must not try to reference
  * the old bundle prior to the installation, which will no longer be around.
  *
- * Before this point, `-showSendingTerminationSignal` or `-showReadyToInstallAndRelaunch:` may be called.
+ * Before this point, `-showInstallingUpdateWithApplicationTerminated:` will be called.
  *
  * @param relaunched Indicates if the update was relaunched.
  * @param acknowledgement Acknowledge to the updater that the finished installation was shown.
@@ -284,7 +276,11 @@ SU_EXPORT @protocol SPUUserDriver <NSObject>
 
 - (void)showUpdateInstallationDidFinishWithAcknowledgement:(void (^)(void))acknowledgement __deprecated_msg("Implement -showUpdateInstalledAndRelaunched:acknowledgement: instead");
 
-- (void)dismissUserInitiatedUpdateCheck __deprecated_msg("Transition to new UI appropriately when a new update is shown, when no update is found, or when an update error occurs.");;
+- (void)dismissUserInitiatedUpdateCheck __deprecated_msg("Transition to new UI appropriately when a new update is shown, when no update is found, or when an update error occurs.");
+
+- (void)showInstallingUpdate __deprecated_msg("Implement -showInstallingUpdateWithApplicationTerminated: instead.");
+
+- (void)showSendingTerminationSignal __deprecated_msg("Implement -showInstallingUpdateWithApplicationTerminated: instead.");
 
 @end
 

diff --git a/TestApplication/SUPopUpTitlebarUserDriver.m b/TestApplication/SUPopUpTitlebarUserDriver.m
@@ -251,15 +251,14 @@ - (void)showExtractionReceivedProgress:(double)progress
     [self addUpdateButtonWithTitle:[NSString stringWithFormat:@"Extracting (%d%%)…", (int)(progress * 100)]];
 }
 
-- (void)showInstallingUpdate
+- (void)showInstallingUpdateWithApplicationTerminated:(BOOL)applicationTerminated
 {
-    [self addUpdateButtonWithTitle:@"Installing…"];
-}
-
-- (void)showSendingTerminationSignal
-{
-    // In case our termination request fails or is delayed
-    [self removeUpdateButton];
+    if (applicationTerminated) {
+        [self addUpdateButtonWithTitle:@"Installing…"];
+    } else {
+        // In case our termination request fails or is delayed
+        [self removeUpdateButton];
+    }
 }
 
 - (void)showUpdateInstalledAndRelaunched:(BOOL)__unused relaunched acknowledgement:(void (^)(void))acknowledgement