From 4f1a18f1024c5bfc4cdef46a856ec4fe42011c71 Mon Sep 17 00:00:00 2001 From: Nino van Hooff Date: Fri, 26 Jun 2020 11:37:01 +0200 Subject: [PATCH 1/5] Add Install Pending Updates button to Account popup CURA-7473 --- cura/API/Account.py | 31 +++++++++++++++++-- .../src/CloudSync/CloudPackageChecker.py | 17 ++++++---- resources/qml/Account/SyncState.qml | 28 +++++++++++++++-- 3 files changed, 64 insertions(+), 12 deletions(-) diff --git a/cura/API/Account.py b/cura/API/Account.py index e190fe9b42..991a2ef967 100644 --- a/cura/API/Account.py +++ b/cura/API/Account.py @@ -1,7 +1,7 @@ # Copyright (c) 2018 Ultimaker B.V. # Cura is released under the terms of the LGPLv3 or higher. from datetime import datetime -from typing import Optional, Dict, TYPE_CHECKING, Union +from typing import Optional, Dict, TYPE_CHECKING, Callable from PyQt5.QtCore import QObject, pyqtSignal, pyqtSlot, pyqtProperty, QTimer, Q_ENUMS @@ -56,6 +56,7 @@ class Account(QObject): lastSyncDateTimeChanged = pyqtSignal() syncStateChanged = pyqtSignal(int) # because SyncState is an int Enum manualSyncEnabledChanged = pyqtSignal(bool) + updatePackagesEnabledChanged = pyqtSignal(bool) def __init__(self, application: "CuraApplication", parent = None) -> None: super().__init__(parent) @@ -66,6 +67,8 @@ class Account(QObject): self._logged_in = False self._sync_state = SyncState.IDLE self._manual_sync_enabled = False + self._update_packages_enabled = False + self._update_packages_action = None # type: Callable self._last_sync_str = "-" self._callback_port = 32118 @@ -143,6 +146,18 @@ class Account(QObject): if not self._update_timer.isActive(): self._update_timer.start() + def setUpdatePackagesAction(self, action: Callable): + """ Set the callback which will be invoked when the user clicks the update packages button + + Should be invoked after your service sets the sync state to SYNCING and before setting the + sync state to SUCCESS. + + Action will be reset to None when the next sync starts + """ + self._update_packages_action = action + self._update_packages_enabled = True + self.updatePackagesEnabledChanged.emit(self._update_packages_enabled) + def _onAccessTokenChanged(self): self.accessTokenChanged.emit() @@ -185,6 +200,9 @@ class Account(QObject): sync is currently running, a sync will be requested. """ + self._update_packages_action = None + self._update_packages_enabled = False + self.updatePackagesEnabledChanged.emit(self._update_packages_enabled) if self._update_timer.isActive(): self._update_timer.stop() elif self._sync_state == SyncState.SYNCING: @@ -251,6 +269,10 @@ class Account(QObject): def manualSyncEnabled(self) -> bool: return self._manual_sync_enabled + @pyqtProperty(bool, notify=updatePackagesEnabledChanged) + def updatePackagesEnabled(self) -> bool: + return self._update_packages_enabled + @pyqtSlot() @pyqtSlot(bool) def sync(self, user_initiated: bool = False) -> None: @@ -259,11 +281,14 @@ class Account(QObject): self._sync() + @pyqtSlot() + def update_packages(self): + if self._update_packages_action is not None: + self._update_packages_action() + @pyqtSlot() def popupOpened(self) -> None: self._setManualSyncEnabled(True) - self._sync_state = SyncState.IDLE - self.syncStateChanged.emit(self._sync_state) @pyqtSlot() def logout(self) -> None: diff --git a/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py b/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py index 7c39354317..ac1f4ba6ab 100644 --- a/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py +++ b/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py @@ -95,10 +95,6 @@ class CloudPackageChecker(QObject): user_subscribed_packages = {plugin["package_id"] for plugin in subscribed_packages_payload} user_installed_packages = self._package_manager.getAllInstalledPackageIDs() - if user_subscribed_packages == self._last_notified_packages: - # already notified user about these - return - # We need to re-evaluate the dismissed packages # (i.e. some package might got updated to the correct SDK version in the meantime, # hence remove them from the Dismissed Incompatible list) @@ -109,7 +105,15 @@ class CloudPackageChecker(QObject): # We check if there are packages installed in Web Marketplace but not in Cura marketplace package_discrepancy = list(user_subscribed_packages.difference(user_installed_packages)) + if package_discrepancy: + account = self._application.getCuraAPI().account + account.setUpdatePackagesAction(lambda: self._onSyncButtonClicked(None, None)) + + if user_subscribed_packages == self._last_notified_packages: + # already notified user about these + return + Logger.log("d", "Discrepancy found between Cloud subscribed packages and Cura installed packages") self._model.addDiscrepancies(package_discrepancy) self._model.initialize(self._package_manager, subscribed_packages_payload) @@ -144,7 +148,8 @@ class CloudPackageChecker(QObject): self._message.hide() self._message = None - def _onSyncButtonClicked(self, sync_message: Message, sync_message_action: str) -> None: - sync_message.hide() + def _onSyncButtonClicked(self, sync_message: Optional[Message], sync_message_action: Optional[str]) -> None: + if sync_message is not None: + sync_message.hide() self._hideSyncMessage() # Should be the same message, but also sets _message to None self.discrepancies.emit(self._model) diff --git a/resources/qml/Account/SyncState.qml b/resources/qml/Account/SyncState.qml index 4e5543f751..77547e48f8 100644 --- a/resources/qml/Account/SyncState.qml +++ b/resources/qml/Account/SyncState.qml @@ -49,7 +49,7 @@ Row // Sync state icon + message width: 20 * screenScaleFactor height: width - source: Cura.API.account.manualSyncEnabled ? UM.Theme.getIcon("update") : UM.Theme.getIcon("checked") + // source is determined by State color: UM.Theme.getColor("account_sync_state_icon") RotationAnimator @@ -80,14 +80,36 @@ Row // Sync state icon + message Label { id: stateLabel - text: catalog.i18nc("@state", catalog.i18nc("@label", "Account synced")) + // text is determined by State color: UM.Theme.getColor("text") font: UM.Theme.getFont("medium") renderType: Text.NativeRendering width: contentWidth + UM.Theme.getSize("default_margin").height height: contentHeight verticalAlignment: Text.AlignVCenter - visible: !Cura.API.account.manualSyncEnabled + visible: !Cura.API.account.manualSyncEnabled && !Cura.API.account.updatePackagesEnabled + } + + Label + { + id: updatePackagesButton + text: catalog.i18nc("@button", "Install pending updates") + color: UM.Theme.getColor("secondary_button_text") + font: UM.Theme.getFont("medium") + renderType: Text.NativeRendering + verticalAlignment: Text.AlignVCenter + height: contentHeight + width: contentWidth + UM.Theme.getSize("default_margin").height + visible: Cura.API.account.updatePackagesEnabled + + MouseArea + { + anchors.fill: parent + onClicked: Cura.API.account.update_packages() + hoverEnabled: true + onEntered: updatePackagesButton.font.underline = true + onExited: updatePackagesButton.font.underline = false + } } Label From 294afdb7ca762c0cd174f9a25e5c8be4c157d0ad Mon Sep 17 00:00:00 2001 From: Nino van Hooff Date: Fri, 26 Jun 2020 16:09:33 +0200 Subject: [PATCH 2/5] Connect sync timer to sync() for consistency and add unit tests. Also rename a function to camelCase CURA-7473 --- cura/API/Account.py | 4 +-- resources/qml/Account/SyncState.qml | 2 +- tests/API/TestAccount.py | 53 +++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 3 deletions(-) diff --git a/cura/API/Account.py b/cura/API/Account.py index 991a2ef967..3ef95834d2 100644 --- a/cura/API/Account.py +++ b/cura/API/Account.py @@ -94,7 +94,7 @@ class Account(QObject): self._update_timer.setInterval(int(self.SYNC_INTERVAL * 1000)) # The timer is restarted explicitly after an update was processed. This prevents 2 concurrent updates self._update_timer.setSingleShot(True) - self._update_timer.timeout.connect(self.syncRequested) + self._update_timer.timeout.connect(self.sync) self._sync_services = {} # type: Dict[str, int] """contains entries "service_name" : SyncState""" @@ -282,7 +282,7 @@ class Account(QObject): self._sync() @pyqtSlot() - def update_packages(self): + def onUpdatePackagesClicked(self): if self._update_packages_action is not None: self._update_packages_action() diff --git a/resources/qml/Account/SyncState.qml b/resources/qml/Account/SyncState.qml index 77547e48f8..66e7f7877b 100644 --- a/resources/qml/Account/SyncState.qml +++ b/resources/qml/Account/SyncState.qml @@ -105,7 +105,7 @@ Row // Sync state icon + message MouseArea { anchors.fill: parent - onClicked: Cura.API.account.update_packages() + onClicked: Cura.API.account.onUpdatePackagesClicked() hoverEnabled: true onEntered: updatePackagesButton.font.underline = true onExited: updatePackagesButton.font.underline = false diff --git a/tests/API/TestAccount.py b/tests/API/TestAccount.py index 09091ba7e0..6780e50b81 100644 --- a/tests/API/TestAccount.py +++ b/tests/API/TestAccount.py @@ -3,6 +3,7 @@ from unittest.mock import MagicMock, patch import pytest from cura.API import Account +from cura.API.Account import SyncState from cura.OAuth2.Models import UserProfile @@ -117,3 +118,55 @@ def test_userProfile(user_profile): mocked_auth_service.getUserProfile = MagicMock(return_value=None) assert account.userProfile is None + + +def test_sync_success(): + account = Account(MagicMock()) + + service1 = "test_service1" + service2 = "test_service2" + + account.setSyncState(service1, SyncState.SYNCING) + assert account.syncState == SyncState.SYNCING + + account.setSyncState(service2, SyncState.SYNCING) + assert account.syncState == SyncState.SYNCING + + account.setSyncState(service1, SyncState.SUCCESS) + # service2 still syncing + assert account.syncState == SyncState.SYNCING + + account.setSyncState(service2, SyncState.SUCCESS) + assert account.syncState == SyncState.SUCCESS + + +def test_sync_update_action(): + account = Account(MagicMock()) + + service1 = "test_service1" + + mockUpdateCallback = MagicMock() + + account.setSyncState(service1, SyncState.SYNCING) + assert account.syncState == SyncState.SYNCING + + account.setUpdatePackagesAction(mockUpdateCallback) + account.onUpdatePackagesClicked() + mockUpdateCallback.assert_called_once_with() + account.setSyncState(service1, SyncState.SUCCESS) + + account.sync() # starting a new sync resets the update action to None + + account.setSyncState(service1, SyncState.SYNCING) + assert account.syncState == SyncState.SYNCING + + account.onUpdatePackagesClicked() # Should not be connected to an action anymore + mockUpdateCallback.assert_called_once_with() # No additional calls + assert account.updatePackagesEnabled is False + account.setSyncState(service1, SyncState.SUCCESS) + + + + + + From c815b098d498a71dac303e21acd8429590ce2d9f Mon Sep 17 00:00:00 2001 From: Nino van Hooff Date: Fri, 26 Jun 2020 16:28:03 +0200 Subject: [PATCH 3/5] Fix a bug where a package sync dialog would incorrectly not be shown 1. user subscribes to a package 2. dismisses the license/unsubscribes 3. subscribes to the same packafe again in this scenario we want to notify the user again after step 3. This was not the case because situations in step 1 and 3 are equal and thus the user was considered notified. CURA-7473 --- plugins/Toolbox/src/CloudSync/CloudPackageChecker.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py b/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py index ac1f4ba6ab..5fec8529e2 100644 --- a/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py +++ b/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py @@ -106,6 +106,16 @@ class CloudPackageChecker(QObject): # We check if there are packages installed in Web Marketplace but not in Cura marketplace package_discrepancy = list(user_subscribed_packages.difference(user_installed_packages)) + if user_subscribed_packages != self._last_notified_packages: + # scenario: + # 1. user subscribes to a package + # 2. dismisses the license/unsubscribes + # 3. subscribes to the same packafe again + # in this scenario we want to notify the user again. To capture that there was a change during + # step 2, we clear the last_notified after step 2. This way, the user will be notified after + # step 3 even though the list of packages for step 1 and 3 are equal + self._last_notified_packages = None + if package_discrepancy: account = self._application.getCuraAPI().account account.setUpdatePackagesAction(lambda: self._onSyncButtonClicked(None, None)) From 8e62a6d6d7054043c19f89ee7fb53a4b0fb5a3b1 Mon Sep 17 00:00:00 2001 From: Nino van Hooff Date: Mon, 29 Jun 2020 10:35:49 +0200 Subject: [PATCH 4/5] Apply suggestions from code review Typing suggestions by Jaime CURA-7473 Co-authored-by: Jaime van Kessel --- cura/API/Account.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cura/API/Account.py b/cura/API/Account.py index 3ef95834d2..17a90f0055 100644 --- a/cura/API/Account.py +++ b/cura/API/Account.py @@ -146,7 +146,7 @@ class Account(QObject): if not self._update_timer.isActive(): self._update_timer.start() - def setUpdatePackagesAction(self, action: Callable): + def setUpdatePackagesAction(self, action: Callable) -> None: """ Set the callback which will be invoked when the user clicks the update packages button Should be invoked after your service sets the sync state to SYNCING and before setting the @@ -282,7 +282,7 @@ class Account(QObject): self._sync() @pyqtSlot() - def onUpdatePackagesClicked(self): + def onUpdatePackagesClicked(self) -> None: if self._update_packages_action is not None: self._update_packages_action() From 979407eddfd532e439e9320741ab06d60cdc8908 Mon Sep 17 00:00:00 2001 From: Nino van Hooff Date: Mon, 29 Jun 2020 11:01:55 +0200 Subject: [PATCH 5/5] Additional typing fixes CURA-7473 --- cura/API/Account.py | 2 +- plugins/Toolbox/src/CloudSync/CloudPackageChecker.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cura/API/Account.py b/cura/API/Account.py index 17a90f0055..15bccb71e1 100644 --- a/cura/API/Account.py +++ b/cura/API/Account.py @@ -68,7 +68,7 @@ class Account(QObject): self._sync_state = SyncState.IDLE self._manual_sync_enabled = False self._update_packages_enabled = False - self._update_packages_action = None # type: Callable + self._update_packages_action = None # type: Optional[Callable] self._last_sync_str = "-" self._callback_port = 32118 diff --git a/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py b/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py index 5fec8529e2..ae8b2f40af 100644 --- a/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py +++ b/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py @@ -110,11 +110,11 @@ class CloudPackageChecker(QObject): # scenario: # 1. user subscribes to a package # 2. dismisses the license/unsubscribes - # 3. subscribes to the same packafe again + # 3. subscribes to the same package again # in this scenario we want to notify the user again. To capture that there was a change during # step 2, we clear the last_notified after step 2. This way, the user will be notified after # step 3 even though the list of packages for step 1 and 3 are equal - self._last_notified_packages = None + self._last_notified_packages = set() if package_discrepancy: account = self._application.getCuraAPI().account