Merge pull request #5914 from nextcloud/backport/5468/stable-3.9

[stable-3.9] Improve macOS Sparkle updater

src/gui/generalsettings.cpp

@@ -300,9 +300,15 @@ void GeneralSettings::slotUpdateInfo()
                                       ocupdater->downloadState() != OCUpdater::DownloadComplete);
     }
 #if defined(Q_OS_MAC) && defined(HAVE_SPARKLE)
-    else if (auto sparkleUpdater = qobject_cast<SparkleUpdater *>(updater)) {
+    else if (const auto sparkleUpdater = qobject_cast<SparkleUpdater *>(updater)) {
+        connect(sparkleUpdater, &SparkleUpdater::statusChanged, this, &GeneralSettings::slotUpdateInfo, Qt::UniqueConnection);
         _ui->updateStateLabel->setText(sparkleUpdater->statusString());
         _ui->restartButton->setVisible(false);
+
+        const auto updaterState = sparkleUpdater->state();
+        const auto enableUpdateButton = updaterState == SparkleUpdater::State::Idle ||
+                                        updaterState == SparkleUpdater::State::Unknown;
+        _ui->updateButton->setEnabled(enableUpdateButton);
     }
 #endif
 

src/gui/updater/sparkleupdater.h

@@ -24,22 +24,40 @@ namespace OCC {
 class SparkleUpdater : public Updater
 {
     Q_OBJECT
+    Q_PROPERTY(QString statusString READ statusString NOTIFY statusChanged)
+    Q_PROPERTY(State state READ state NOTIFY statusChanged)
+
 public:
-    SparkleUpdater(const QUrl &appCastUrl);
+    class SparkleInterface;
+    enum class State {
+        Unknown = 0,
+        Idle,
+        Working,
+        AwaitingUserInput
+    };
+
+    explicit SparkleUpdater(const QUrl &appCastUrl);
     ~SparkleUpdater() override;
 
-    void setUpdateUrl(const QUrl &url);
+    [[nodiscard]] static bool autoUpdaterAllowed();
 
-    // unused in this updater
+    [[nodiscard]] bool handleStartup() override { return false; }
+    [[nodiscard]] QString statusString() const;
+    [[nodiscard]] State state() const;
+
+public slots:
+    void setUpdateUrl(const QUrl &url);
     void checkForUpdate() override;
     void backgroundCheckForUpdate() override;
-    bool handleStartup() override { return false; }
 
-    QString statusString();
+signals:
+    void statusChanged();
 
 private:
-    class Private;
-    Private *d;
+    std::unique_ptr<SparkleInterface> _interface;
+    QString _statusString;
+    State _state = State::Unknown;
+    friend class SparkleInterface;
 };
 
 } // namespace OCC

src/gui/updater/sparkleupdater_mac.mm

@@ -22,16 +22,81 @@
 #include "configfile.h"
 #include "updater/sparkleupdater.h"
 
-@interface DelegateObject : NSObject <SUUpdaterDelegate>
+@class NCSparkleUpdaterDelegate;
+
+class Q_DECL_HIDDEN OCC::SparkleUpdater::SparkleInterface
+{
+public:
+    explicit SparkleInterface(SparkleUpdater *parent)
+        : q(parent)
+    {
+    }
+
+    ~SparkleInterface()
+    {
+        [updater release];
+        [delegate release];
+    }
+
+    void statusChanged(const OCC::SparkleUpdater::State state, const QString &statusString = {})
+    {
+        q->_state = state;
+        q->_statusString = statusString;
+        emit q->statusChanged();
+    }
+
+    SUUpdater* updater;
+    NCSparkleUpdaterDelegate *delegate;
+
+private:
+    SparkleUpdater * const q;
+};
+
+
+@interface NCSparkleUpdaterDelegate : NSObject <SUUpdaterDelegate>
+
+@property (readwrite, assign) OCC::SparkleUpdater::SparkleInterface *owner;
+
+- (instancetype)initWithOwner:(OCC::SparkleUpdater::SparkleInterface *)owner;
 - (BOOL)updaterMayCheckForUpdates:(SUUpdater *)bundle;
+
 @end
-@implementation DelegateObject //(SUUpdaterDelegateInformalProtocol)
+
+@implementation NCSparkleUpdaterDelegate
+
+- (instancetype)initWithOwner:(OCC::SparkleUpdater::SparkleInterface *)owner
+{
+    self = [super init];
+    if (self) {
+        _owner = owner;
+    }
+    return self;
+}
+
+- (BOOL)backgroundUpdateChecksAllowed
+{
+    BOOL allowUpdateCheck = OCC::ConfigFile().skipUpdateCheck() ? NO : YES;
+    qCDebug(OCC::lcUpdater) << "Updater may check for updates:" << (allowUpdateCheck ? "YES" : "NO");
+    return allowUpdateCheck;
+}
 
 - (BOOL)updaterMayCheckForUpdates:(SUUpdater *)bundle
 {
     Q_UNUSED(bundle)
-    qCDebug(OCC::lcUpdater) << "Updater may check for updates: YES";
-    return YES;
+    return [self backgroundUpdateChecksAllowed];
+}
+
+- (BOOL)updaterShouldShowUpdateAlertForScheduledUpdate:(SUUpdater *)updater forItem:(SUAppcastItem *)item
+{
+    Q_UNUSED(updater)
+    Q_UNUSED(item)
+    return [self backgroundUpdateChecksAllowed];
+}
+
+- (void)notifyStateChange:(const OCC::SparkleUpdater::State)state displayStatus:(const QString&)statusString
+{
+    qCDebug(OCC::lcUpdater) << statusString;
+    _owner->statusChanged(state, statusString);
 }
 
 // Sent when a valid update is found by the update driver.
@@ -39,89 +104,176 @@
 {
     Q_UNUSED(updater)
     Q_UNUSED(update)
-    qCDebug(OCC::lcUpdater) << "Found a valid update.";
+
+    const auto versionQstring = QString::fromNSString(update.displayVersionString);
+    const auto message = QObject::tr("Found a valid update: version %1", "%1 is version number").arg(versionQstring);
+
+    [self notifyStateChange:OCC::SparkleUpdater::State::AwaitingUserInput
+              displayStatus:message];
 }
 
 // Sent when a valid update is not found.
-- (void)updaterDidNotFindUpdate:(SUUpdater *)update
+- (void)updaterDidNotFindUpdate:(SUUpdater *)updater
 {
-    Q_UNUSED(update)
-    qCDebug(OCC::lcUpdater) << "No valid update found.";
+    Q_UNUSED(updater)
+    [self notifyStateChange:OCC::SparkleUpdater::State::Idle
+              displayStatus:QObject::tr("No valid update found.")];
 }
 
 // Sent immediately before installing the specified update.
 - (void)updater:(SUUpdater *)updater willInstallUpdate:(SUAppcastItem *)update
 {
     Q_UNUSED(updater)
-    Q_UNUSED(update)
-    qCDebug(OCC::lcUpdater) << "About to install update.";
+
+    const auto versionQstring = QString::fromNSString(update.displayVersionString);
+    const auto message = QObject::tr("About to install version %1 update.", "%1 is version number").arg(versionQstring);
+
+    [self notifyStateChange:OCC::SparkleUpdater::State::Working
+              displayStatus:message];
 }
 
-- (void) updater:(SUUpdater *)updater didAbortWithError:(NSError *)error
+- (void)updater:(SUUpdater *)updater didAbortWithError:(NSError *)error
 {
     Q_UNUSED(updater)
-    qCDebug(OCC::lcUpdater) << error.description;
+
+    const auto errorQstring = QString::fromNSString(error.localizedDescription);
+    const auto message = QObject::tr("Aborted with error: %1", "%1 is version number").arg(errorQstring);
+    [self notifyStateChange:OCC::SparkleUpdater::State::Idle
+              displayStatus:message];
 }
 
 - (void)updater:(SUUpdater *)updater didFinishLoadingAppcast:(SUAppcast *)appcast
 {
     Q_UNUSED(updater)
     Q_UNUSED(appcast)
-    qCDebug(OCC::lcUpdater) << "Finished loading appcast.";
+    [self notifyStateChange:OCC::SparkleUpdater::State::Working
+              displayStatus:QObject::tr("Finished loading appcast.")];
 }
 
+- (void)updater:(SUUpdater *)updater didDismissUpdateAlertPermanently:(BOOL)permanently forItem:(nonnull SUAppcastItem *)item
+{
+    Q_UNUSED(updater)
 
-@end
+    const auto permanencyString = permanently ? QObject::tr("Permanently") : QObject::tr("Temporarily");
+    const auto versionQstring = QString::fromNSString(item.displayVersionString);
+    const auto message = QObject::tr("%1 dismissed %2 update", "%1 is permanently or temporarily, %2 is version number").arg(permanencyString, versionQstring);
 
+    [self notifyStateChange:OCC::SparkleUpdater::State::Idle
+              displayStatus:message];
+}
 
-namespace OCC {
+- (void)updater:(nonnull SUUpdater *)updater userDidSkipThisVersion:(nonnull SUAppcastItem *)item
+{
+    Q_UNUSED(updater)
 
-class SparkleUpdater::Private
+    const auto versionQstring = QString::fromNSString(item.displayVersionString);
+    const auto message = QObject::tr("Update version %1 will not be applied as it was chosen to be skipped.",  "%1 is version number").arg(versionQstring);
+
+    [self notifyStateChange:OCC::SparkleUpdater::State::Idle
+              displayStatus:message];
+}
+
+- (void)updater:(nonnull SUUpdater *)updater willDownloadUpdate:(nonnull SUAppcastItem *)item withRequest:(nonnull NSMutableURLRequest *)request
 {
-    public:
-        SUUpdater* updater;
-        DelegateObject *delegate;
-};
+    Q_UNUSED(updater)
+    Q_UNUSED(request)
+
+    const auto versionQstring = QString::fromNSString(item.displayVersionString);
+    const auto message = QObject::tr("Downloading version %1 update.", "%1 is version number").arg(versionQstring);
+
+    [self notifyStateChange:OCC::SparkleUpdater::State::Working
+              displayStatus:message];
+}
+
+- (void)updater:(nonnull SUUpdater *)updater didDownloadUpdate:(nonnull SUAppcastItem *)item
+{
+    Q_UNUSED(updater)
+
+    const auto versionQstring = QString::fromNSString(item.displayVersionString);
+    const auto message = QObject::tr("Downloaded version %1 update.", "%1 is version number").arg(versionQstring);
+
+    [self notifyStateChange:OCC::SparkleUpdater::State::Working
+              displayStatus:message];
+}
+
+- (void)updater:(nonnull SUUpdater *)updater failedToDownloadUpdate:(nonnull SUAppcastItem *)item error:(nonnull NSError *)error
+{
+    Q_UNUSED(updater)
+
+    const auto versionQstring = QString::fromNSString(item.displayVersionString);
+    const auto errorQstring = QString::fromNSString(error.localizedDescription);
+    const auto message = QObject::tr("Error downloading version %1 update: %2", "%1 is version number, %2 is error message").arg(versionQstring);
+
+    [self notifyStateChange:OCC::SparkleUpdater::State::Idle
+              displayStatus:message];
+}
+
+- (void)updater:(nonnull SUUpdater *)updater willExtractUpdate:(nonnull SUAppcastItem *)item
+{
+    Q_UNUSED(updater)
+
+    const auto versionQstring = QString::fromNSString(item.displayVersionString);
+    const auto message = QObject::tr("Extracting version %1 update.", "%1 is version number").arg(versionQstring);
+
+    [self notifyStateChange:OCC::SparkleUpdater::State::Working
+              displayStatus:message];
+}
+
+- (void)updater:(nonnull SUUpdater *)updater didExtractUpdate:(nonnull SUAppcastItem *)item
+{
+    Q_UNUSED(updater)
+
+    const auto versionQstring = QString::fromNSString(item.displayVersionString);
+    const auto message = QObject::tr("Extracted version %1 update.", "%1 is version number").arg(versionQstring);
+
+    [self notifyStateChange:OCC::SparkleUpdater::State::Working
+              displayStatus:message];
+}
+
+- (void)userDidCancelDownload:(SUUpdater *)updater
+{
+    Q_UNUSED(updater);
+    [self notifyStateChange:OCC::SparkleUpdater::State::Idle
+              displayStatus:QObject::tr("Update download cancelled.")];
+}
+
+@end
+
+
+namespace OCC {
 
 // Delete ~/Library//Preferences/com.owncloud.desktopclient.plist to re-test
 SparkleUpdater::SparkleUpdater(const QUrl& appCastUrl)
     : Updater()
+    , _interface(std::make_unique<SparkleInterface>(this))
 {
-    d = new Private;
+    _interface->delegate = [[NCSparkleUpdaterDelegate alloc] initWithOwner:_interface.get()];
+    [_interface->delegate retain];
 
-    d->delegate = [[DelegateObject alloc] init];
-    [d->delegate retain];
-
-    d->updater = [SUUpdater sharedUpdater];
-    [d->updater setDelegate:d->delegate];
-    [d->updater setAutomaticallyChecksForUpdates:YES];
-    [d->updater setAutomaticallyDownloadsUpdates:NO];
-    [d->updater setSendsSystemProfile:NO];
-    [d->updater resetUpdateCycle];
-    [d->updater retain];
+    _interface->updater = [SUUpdater sharedUpdater];
+    [_interface->updater setDelegate:_interface->delegate];
+    [_interface->updater setAutomaticallyChecksForUpdates:YES];
+    [_interface->updater setAutomaticallyDownloadsUpdates:NO];
+    [_interface->updater setSendsSystemProfile:NO];
+    [_interface->updater resetUpdateCycle];
+    [_interface->updater retain];
 
     setUpdateUrl(appCastUrl);
 
     // Sparkle 1.8 required
-    NSString *userAgent = [NSString stringWithUTF8String: Utility::userAgentString().data()];
-    [d->updater setUserAgentString: userAgent];
+    const auto userAgentString = QString::fromUtf8(Utility::userAgentString());
+    NSString *userAgent = userAgentString.toNSString();
+    [_interface->updater setUserAgentString: userAgent];
 }
 
-SparkleUpdater::~SparkleUpdater()
-{
-    [d->updater release];
-    delete d;
-}
+SparkleUpdater::~SparkleUpdater() = default;
 
 void SparkleUpdater::setUpdateUrl(const QUrl &url)
 {
-    NSURL* nsurl = [NSURL URLWithString:
-            [NSString stringWithUTF8String: url.toString().toUtf8().data()]];
-    [d->updater setFeedURL: nsurl];
+    [_interface->updater setFeedURL:url.toNSURL()];
 }
 
-// FIXME: Should be changed to not instantiate the SparkleUpdater at all in this case
-bool autoUpdaterAllowed()
+bool SparkleUpdater::autoUpdaterAllowed()
 {
     // See https://github.com/owncloud/client/issues/2931
     NSString *bundlePath = [[NSBundle mainBundle] bundlePath];
@@ -139,27 +291,32 @@ bool autoUpdaterAllowed()
     return true;
 }
 
-
 void SparkleUpdater::checkForUpdate()
 {
     qCDebug(OCC::lcUpdater) << "Checking for updates.";
     if (autoUpdaterAllowed()) {
-        [d->updater checkForUpdates: NSApp];
+        [_interface->updater checkForUpdates: NSApp];
     }
 }
 
 void SparkleUpdater::backgroundCheckForUpdate()
 {
-    qCDebug(OCC::lcUpdater) << "launching background check";
-    if (autoUpdaterAllowed()) {
-        [d->updater checkForUpdatesInBackground];
+    if (autoUpdaterAllowed() && !ConfigFile().skipUpdateCheck()) {
+        qCDebug(OCC::lcUpdater) << "launching background check";
+        [_interface->updater checkForUpdatesInBackground];
+    } else {
+        qCDebug(OCC::lcUpdater) << "not launching background check, auto updater not allowed or update check skipped in config";
     }
 }
 
-QString SparkleUpdater::statusString()
+QString SparkleUpdater::statusString() const
+{
+    return _statusString;
+}
+
+SparkleUpdater::State SparkleUpdater::state() const
 {
-    // FIXME Show the real state depending on the callbacks
-    return QString();
+    return _state;
 }
 
 } // namespace OCC

src/gui/updater/updater.cpp

@@ -57,7 +57,9 @@ QUrl Updater::updateUrl()
     auto urlQuery = getQueryParams();
 
 #if defined(Q_OS_MAC) && defined(HAVE_SPARKLE)
-    urlQuery.addQueryItem(QLatin1String("sparkle"), QLatin1String("true"));
+    if (SparkleUpdater::autoUpdaterAllowed()) {
+        urlQuery.addQueryItem(QLatin1String("sparkle"), QLatin1String("true"));
+    }
 #endif
 
 #if defined(Q_OS_WIN)
@@ -142,7 +144,11 @@ Updater *Updater::create()
     }
 
 #if defined(Q_OS_MACOS) && defined(HAVE_SPARKLE) && defined(BUILD_OWNCLOUD_OSX_BUNDLE)
-    return new SparkleUpdater(url);
+    if (SparkleUpdater::autoUpdaterAllowed()) {
+        return new SparkleUpdater(url);
+    }
+
+    return new PassiveUpdateNotifier(url);
 #elif defined(Q_OS_WIN32)
     // Also for MSI
     return new NSISUpdater(url);