diff --git a/Furious/Service/SubscriptionManager.py b/Furious/Service/SubscriptionManager.py index 7862d4f..1b3aab3 100644 --- a/Furious/Service/SubscriptionManager.py +++ b/Furious/Service/SubscriptionManager.py @@ -102,6 +102,11 @@ class SubscriptionUpdateBatch: class SubscriptionManager(HttpGetManager): """Own subscription networking, decoding, reconciliation, and persistence.""" + # Presentation metadata changed for these stable subscription IDs. This + # deliberately does not imply that profile topology changed. + subscriptionStateChanged = QtCore.Signal(object) + + # Subscription groups or their derived profile topology changed. subscriptionsChanged = QtCore.Signal() subscriptionCommitted = QtCore.Signal(str) updateCompleted = QtCore.Signal(object) @@ -478,7 +483,20 @@ class SubscriptionManager(HttpGetManager): if not committedSuccess and not committedFailure: return - self.subscriptionsChanged.emit() + changedSubscriptions = tuple( + dict.fromkeys( + param.get('unique', '') + for param in (*committedSuccess, *committedFailure) + if param.get('unique') + ) + ) + + if changedSubscriptions: + self.subscriptionStateChanged.emit(changedSubscriptions) + + if committedSuccess: + self.subscriptionsChanged.emit() + self.updateCompleted.emit( SubscriptionUpdateBatch( tuple(committedSuccess), @@ -597,67 +615,57 @@ class SubscriptionManager(HttpGetManager): forwardSender=True, ) - def updateSubsByUnique(self, unique: str, **kwargs): - """Update one enabled subscription group by stable ID.""" - subscription = Storage.UserSubs().get(unique) + def updateSubscriptions(self, uniques, **kwargs): + """Start eligible stable IDs as one status and completion batch.""" + subscriptions = Storage.UserSubs() - if ( - not subscription - or not subscription.get('enabled', True) - or not subscription.get('webURL') - ): + batch = tuple( + (unique, subscriptions[unique]) + for unique in dict.fromkeys(uniques) + if unique in subscriptions + and subscriptions[unique].get('enabled', True) + and subscriptions[unique].get('webURL') + ) + + if not batch: return - group = Storage.SubscriptionGroup(unique) + changedSubscriptions = [] + + for unique, _subscription in batch: + group = Storage.SubscriptionGroup(unique) + + if group is None: + continue - if group is not None: group.lastSyncStatus = 'syncing' group.lastSyncError = '' Storage.upsertSubscriptionGroup(group) + changedSubscriptions.append(unique) - depthMap = kwargs.get('depthMap') - successArgs = kwargs.get('successArgs') - failureArgs = kwargs.get('failureArgs') + if changedSubscriptions: + self.subscriptionStateChanged.emit(tuple(changedSubscriptions)) - if depthMap is None: - depthMap = {'depth': 1} + depthMap = {'depth': len(batch)} + successArgs = [] + failureArgs = [] - if successArgs is None: - successArgs = list() - - if failureArgs is None: - failureArgs = list() - - kwargs.update( - depthMap=depthMap, - successArgs=successArgs, - failureArgs=failureArgs, - requestVersion=self._nextRequestVersion(unique), - ) - - self.updateSubsByWebGET(unique=unique, **subscription, **kwargs) - - def updateSubs(self, **kwargs): - """Update every enabled subscription as one completion batch.""" - enabledKeys = tuple( - key - for key, subscription in Storage.UserSubs().items() - if subscription.get('enabled', True) and subscription.get('webURL') - ) - - if not enabledKeys: - return - - depthMap = {'depth': len(enabledKeys)} - successArgs = list() - failureArgs = list() - - for key in enabledKeys: - self.updateSubsByUnique( - key, + for unique, subscription in batch: + self.updateSubsByWebGET( + unique=unique, + **subscription, + **kwargs, depthMap=depthMap, successArgs=successArgs, failureArgs=failureArgs, - **kwargs, + requestVersion=self._nextRequestVersion(unique), ) + + def updateSubsByUnique(self, unique: str, **kwargs): + """Update one enabled subscription through the canonical batch path.""" + self.updateSubscriptions((unique,), **kwargs) + + def updateSubs(self, **kwargs): + """Update every eligible subscription through the canonical batch path.""" + self.updateSubscriptions(tuple(Storage.UserSubs()), **kwargs) diff --git a/Furious/Widget/ServerTableView.py b/Furious/Widget/ServerTableView.py index 353e096..b1cfafd 100644 --- a/Furious/Widget/ServerTableView.py +++ b/Furious/Widget/ServerTableView.py @@ -1922,19 +1922,19 @@ class ServerTableView( def updateSubsByUnique(self, unique: str, httpProxy: Union[str, None], **kwargs): """Update subs by unique.""" + self.updateSubscriptions((unique,), httpProxy, **kwargs) + + def updateSubscriptions(self, uniques, httpProxy: Union[str, None], **kwargs): + """Update stable subscription IDs as one manager-owned batch.""" kwargs.pop('parent', None) self.subsManager.configureHttpProxy(httpProxy) - self.subsManager.updateSubsByUnique(unique, **kwargs) + self.subsManager.updateSubscriptions(uniques, **kwargs) def updateSubs(self, httpProxy: Union[str, None], **kwargs): """Update subs.""" self.selectionModel().clearSelection() - - kwargs.pop('parent', None) - - self.subsManager.configureHttpProxy(httpProxy) - self.subsManager.updateSubs(**kwargs) + self.updateSubscriptions(tuple(Storage.UserSubs()), httpProxy, **kwargs) @QtCore.Slot() def _handleSubscriptionsChanged(self): diff --git a/Furious/Widget/SubscriptionTableView.py b/Furious/Widget/SubscriptionTableView.py index e3e1a78..5e156fe 100644 --- a/Furious/Widget/SubscriptionTableView.py +++ b/Furious/Widget/SubscriptionTableView.py @@ -718,6 +718,28 @@ class SubscriptionTableView(Mixins.QTranslatable, AppQTableView): self.selectRow(target) self.groupsChanged.emit() + @QtCore.Slot(object) + def refreshSubscriptionState(self, uniques): + """Repaint metadata cells for stable subscription IDs only.""" + rows = {unique: row for row, unique in enumerate(Storage.UserSubs())} + + firstColumn, lastColumn = ( + self.ItemKey.index('lastSyncStatus'), + self.ItemKey.index('profiles'), + ) + + for unique in dict.fromkeys(uniques): + row = rows.get(unique) + + if row is None: + continue + + self.sourceModel.dataChanged.emit( + self.sourceModel.index(row, firstColumn), + self.sourceModel.index(row, lastColumn), + [], + ) + def flushItem(self, row, column, item): """Refresh item.""" if row < 0 or row >= self.sourceModel.rowCount(): diff --git a/Furious/Window/SubscriptionPage.py b/Furious/Window/SubscriptionPage.py index cfa5b63..939ba5a 100644 --- a/Furious/Window/SubscriptionPage.py +++ b/Furious/Window/SubscriptionPage.py @@ -290,7 +290,11 @@ class SubscriptionPage(Mixins.QTranslatable, Mixins.ThemeAware, QMainWindow): self.serverTable.subsManager.subscriptionsChanged.emit ) - self.serverTable.subsManager.subscriptionsChanged.connect(self.table.flushAll) + # The manager and table are persistent main-window children. This direct + # connection intentionally shares their process-lifetime ownership. + self.serverTable.subsManager.subscriptionStateChanged.connect( + self.table.refreshSubscriptionState + ) self.addButton.clicked.connect(self.addSubscription) self.editButton.clicked.connect(self.editSelected) @@ -508,21 +512,12 @@ class SubscriptionPage(Mixins.QTranslatable, Mixins.ThemeAware, QMainWindow): if not keys: return - depthMap = {'depth': len(keys)} - successArgs = [] - failureArgs = [] - httpProxy = self._selectedProxy() - - for key in keys: - self.serverTable.updateSubsByUnique( - key, - httpProxy, - depthMap=depthMap, - successArgs=successArgs, - failureArgs=failureArgs, - showMessageBox=True, - parent=self, - ) + self.serverTable.updateSubscriptions( + keys, + self._selectedProxy(), + showMessageBox=True, + parent=self, + ) @QtCore.Slot() def updateAll(self): diff --git a/tests/test_subscription_manager.py b/tests/test_subscription_manager.py index 4ec1546..3adefb8 100644 --- a/tests/test_subscription_manager.py +++ b/tests/test_subscription_manager.py @@ -340,8 +340,14 @@ class SubscriptionManagerTest(TestCase): ) completed = [] committedSubscriptions = [] + stateChanges = [] + structuralChanges = [] manager.updateCompleted.connect(completed.append) manager.subscriptionCommitted.connect(committedSubscriptions.append) + manager.subscriptionStateChanged.connect( + lambda uniques: stateChanges.append(tuple(uniques)) + ) + manager.subscriptionsChanged.connect(lambda: structuralChanges.append(True)) failed = {'unique': 'group-a', 'profiles': ()} successful = {'unique': 'group-b', 'profiles': ()} @@ -360,6 +366,37 @@ class SubscriptionManagerTest(TestCase): self.assertEqual(completed[0].failed[0]['unique'], 'group-a') self.assertIn('injected failure', completed[0].failed[0]['error']) self.assertEqual(committedSubscriptions, ['group-b']) + self.assertEqual(stateChanges, [('group-b', 'group-a')]) + self.assertEqual(structuralChanges, [True]) + + manager.deleteLater() + + def testFailedBatchPublishesStateWithoutStructuralChange(self): + """Present terminal failure metadata without resetting profile consumers.""" + manager = self._manager() + stateChanges = [] + structuralChanges = [] + completed = [] + + manager.subscriptionStateChanged.connect( + lambda uniques: stateChanges.append(tuple(uniques)) + ) + manager.subscriptionsChanged.connect(lambda: structuralChanges.append(True)) + manager.updateCompleted.connect(completed.append) + + with mock.patch( + 'Furious.Service.SubscriptionManager.Storage.SubscriptionGroup', + return_value=None, + ): + manager.handleSynchronizationResults( + successArgs=[], + failureArgs=[{'unique': 'group-a', 'error': 'offline'}], + ) + + self.assertEqual(stateChanges, [('group-a',)]) + self.assertEqual(structuralChanges, []) + self.assertEqual(len(completed), 1) + self.assertEqual(completed[0].failed[0]['error'], 'offline') manager.deleteLater() @@ -660,6 +697,142 @@ class SubscriptionManagerTest(TestCase): self.assertEqual(gettext('Updated', 'ZH'), '已更新') self.assertEqual(gettext('Update Failed', 'ZH'), '更新失败') + def testSubscriptionStateNotificationRepaintsOnlyAffectedMetadataRow(self): + """Resolve stable IDs at delivery and avoid a whole-table refresh.""" + subscriptions = { + 'group-a': self._subscription(), + 'group-b': self._subscription( + remark='Group B', + webURL='https://invalid.test/b', + ), + 'group-c': self._subscription( + remark='Group C', + webURL='https://invalid.test/c', + ), + } + + with mock.patch.object(Storage, 'UserSubs', return_value=subscriptions): + table = SubscriptionTableView() + changes = [] + + def changed(topLeft, bottomRight, _roles): + """Capture one exact model repaint range.""" + changes.append( + ( + (topLeft.row(), topLeft.column()), + (bottomRight.row(), bottomRight.column()), + ) + ) + + table.sourceModel.dataChanged.connect(changed) + + with mock.patch.object(table, 'flushAll') as flushAll: + table.refreshSubscriptionState(('group-b',)) + + self.assertEqual( + changes, + [ + ( + (1, table.ItemKey.index('lastSyncStatus')), + (1, table.ItemKey.index('profiles')), + ) + ], + ) + flushAll.assert_not_called() + table.deleteLater() + + def testUpdateAllPublishesOneImmediateSyncingSnapshot(self): + """Expose one metadata batch without publishing structural changes.""" + subscriptions = { + 'group-a': self._subscription(), + 'group-b': self._subscription( + remark='Group B', + webURL='https://invalid.test/b', + ), + } + manager = self._manager(subscriptions) + snapshots = [] + stateChanges = [] + structuralChanges = [] + + def group(unique): + """Return one isolated persisted group.""" + value = subscriptions.get(unique) + + return ( + SubscriptionGroup.fromMapping(unique, value) + if value is not None + else None + ) + + def upsert(value): + """Persist one status transition into the isolated repository.""" + subscriptions[value.id] = value.toMapping() + + def stateChanged(uniques): + """Capture the stable IDs and their state at notification time.""" + stateChanges.append(tuple(uniques)) + snapshots.append( + tuple( + subscriptions[unique].get('lastSyncStatus', '') + for unique in ('group-a', 'group-b') + ) + ) + + manager.subscriptionStateChanged.connect(stateChanged) + manager.subscriptionsChanged.connect(lambda: structuralChanges.append(True)) + + with ( + mock.patch.object(Storage, 'UserSubs', return_value=subscriptions), + mock.patch.object(Storage, 'SubscriptionGroup', side_effect=group), + mock.patch.object(Storage, 'upsertSubscriptionGroup', side_effect=upsert), + mock.patch.object(manager, 'updateSubsByWebGET') as update, + ): + manager.updateSubs() + + self.assertEqual(snapshots, [('syncing', 'syncing')]) + self.assertEqual(stateChanges, [('group-a', 'group-b')]) + self.assertEqual(structuralChanges, []) + self.assertEqual(update.call_count, 2) + self.assertEqual( + {call.kwargs['unique'] for call in update.call_args_list}, + {'group-a', 'group-b'}, + ) + + manager.deleteLater() + + def testTargetedUpdatePublishesImmediateSyncingState(self): + """Publish one narrow targeted state before network I/O.""" + subscriptions = {'group-a': self._subscription()} + manager = self._manager(subscriptions) + group = SubscriptionGroup.fromMapping('group-a', subscriptions['group-a']) + snapshots = [] + stateChanges = [] + structuralChanges = [] + + def stateChanged(uniques): + """Capture the target and persisted state before network I/O.""" + stateChanges.append(tuple(uniques)) + snapshots.append(group.lastSyncStatus) + + manager.subscriptionStateChanged.connect(stateChanged) + manager.subscriptionsChanged.connect(lambda: structuralChanges.append(True)) + + with ( + mock.patch.object(Storage, 'UserSubs', return_value=subscriptions), + mock.patch.object(Storage, 'SubscriptionGroup', return_value=group), + mock.patch.object(Storage, 'upsertSubscriptionGroup'), + mock.patch.object(manager, 'updateSubsByWebGET') as update, + ): + manager.updateSubsByUnique('group-a') + + self.assertEqual(snapshots, ['syncing']) + self.assertEqual(stateChanges, [('group-a',)]) + self.assertEqual(structuralChanges, []) + update.assert_called_once() + + manager.deleteLater() + def testTargetedPolicyChangeDoesNotDisturbOtherSubscriptions(self): """Reconcile only the edited subscription's schedule.""" subscriptions = { @@ -741,6 +914,95 @@ class SubscriptionManagerTest(TestCase): manager._replySubscriptions.clear() manager.deleteLater() + def testUpdateSelectedUsesOneOrderedCanonicalBatchCall(self): + """Keep a real selected-update click narrow and event-loop responsive.""" + subscriptions = { + 'group-a': self._subscription(), + 'group-b': self._subscription( + remark='Group B', + webURL='https://invalid.test/b', + ), + 'group-c': self._subscription( + remark='Group C', + webURL='https://invalid.test/c', + ), + } + + def group(unique): + """Return one isolated persisted group.""" + value = subscriptions.get(unique) + + return ( + SubscriptionGroup.fromMapping(unique, value) + if value is not None + else None + ) + + def upsert(value): + """Persist one status transition into the isolated repository.""" + subscriptions[value.id] = value.toMapping() + + with ( + mock.patch.object(Storage, 'UserSubs', return_value=subscriptions), + mock.patch.object(Storage, 'SubscriptionGroup', side_effect=group), + mock.patch.object(Storage, 'upsertSubscriptionGroup', side_effect=upsert), + ): + manager = SubscriptionManager() + stateChanges = [] + structuralChanges = [] + + def updateSubscriptions(uniques, _httpProxy, **kwargs): + """Cross the production wrapper boundary into the real manager.""" + kwargs.pop('parent', None) + manager.updateSubscriptions(uniques, **kwargs) + + serverTable = SimpleNamespace(subsManager=manager) + serverTable.updateSubscriptions = mock.Mock(side_effect=updateSubscriptions) + page = SubscriptionPage(serverTable) + selection = page.table.selectionModel() + flags = ( + QtCore.QItemSelectionModel.SelectionFlag.Select + | QtCore.QItemSelectionModel.SelectionFlag.Rows + ) + + selection.select(page.table.sourceModel.index(0, 0), flags) + selection.select(page.table.sourceModel.index(2, 0), flags) + + manager.subscriptionStateChanged.connect( + lambda uniques: stateChanges.append(tuple(uniques)) + ) + manager.subscriptionsChanged.connect(lambda: structuralChanges.append(True)) + + page.show() + processQtEvents(1) + + handled = [] + QtCore.QTimer.singleShot(0, lambda: handled.append(True)) + + with mock.patch.object(manager, 'updateSubsByWebGET') as update: + QtTest.QTest.mouseClick( + page.updateSelectedButton, + QtCore.Qt.MouseButton.LeftButton, + ) + + processQtEvents(1) + + serverTable.updateSubscriptions.assert_called_once_with( + ('group-a', 'group-c'), + None, + showMessageBox=True, + parent=page, + ) + self.assertEqual(stateChanges, [('group-a', 'group-c')]) + self.assertEqual(structuralChanges, []) + self.assertEqual(update.call_count, 2) + self.assertEqual(handled, [True]) + + page.deleteLater() + manager.deleteLater() + + processQtEvents() + def testPageNavigationIsPresentationOnlyForAutoUpdateScheduler(self): """Keep page show/hide cycles outside scheduler policy ownership.""" subscriptions = {'group-a': self._subscription()}