no longer delete all source widgets each update

securedrop_client/gui/widgets.py

@@ -854,43 +854,36 @@ def setup(self, controller):
 
     def update(self, sources: List[Source]) -> List[str]:
         """
-        Reset and update the list with the passed in list of sources.
-        """
-        # A flag to show if a source is currently selected (the current source
-        # doesn't have to be selected in a QListWidget -- it appears to default
-        # to whatever is in row 0, whether it's selected or not).
-        has_source_selected = False
-        if self.currentRow() > -1:
-            has_source_selected = self.item(self.currentRow()).isSelected()
-        current_source = self.get_current_source()
-        current_source_id = current_source and current_source.id
-
-        # Make a copy of the source_widgets that are currently displayed
-        # so that we can compare which widgets were removed. This means that
-        # the source was deleted, and we'll return this so we can also
-        # delete the corresponding SourceConversationWrapper.
-        existing_source_widgets = self.source_widgets.copy()
-
-        # When we call clear() to delete all SourceWidgets, we should
-        # also clear the source_widgets dict.
-        self.clear()
-        self.source_widgets = {}
-
+        Update the list with the passed in list of sources.
+        """
+        # Delete widgets that no longer exist in source list
+        source_uuids = [source.uuid for source in sources]
+        deleted_uuids = []
+        for i in range(self.count()):
+            list_item = self.item(i)
+            list_widget = self.itemWidget(list_item)
+
+            if list_widget and list_widget.source_uuid not in source_uuids:
+                if list_item.isSelected():
+                    self.setCurrentItem(None)
+                del self.source_widgets[list_widget.source_uuid]
+                deleted_uuids.append(list_widget.source_uuid)
+                self.takeItem(i)
+                list_widget.deleteLater()
+
+        # Create new widgets for new sources
+        widget_uuids = [self.itemWidget(self.item(i)).source_uuid for i in range(self.count())]
         for source in sources:
-            new_source = SourceWidget(source)
-            new_source.setup(self.controller)
-            self.source_widgets[source.uuid] = new_source
-
-            list_item = QListWidgetItem(self)
-            list_item.setSizeHint(new_source.sizeHint())
-
-            self.addItem(list_item)
-            self.setItemWidget(list_item, new_source)
+            if source.uuid not in widget_uuids:
+                new_source = SourceWidget(source)
+                new_source.setup(self.controller)
+                self.source_widgets[source.uuid] = new_source
 
-            if source.id == current_source_id and has_source_selected:
-                self.setCurrentItem(list_item)
+                list_item = QListWidgetItem()
+                self.insertItem(0, list_item)
+                list_item.setSizeHint(new_source.sizeHint())
+                self.setItemWidget(list_item, new_source)
 
-        deleted_uuids = list(set(existing_source_widgets.keys()) - set(self.source_widgets.keys()))
         return deleted_uuids
 
     def get_current_source(self):
@@ -977,6 +970,7 @@ def __init__(self, source: Source):
 
         # Store source
         self.source = source
+        self.source_uuid = source.uuid
 
         # Set styles
         self.setStyleSheet(self.CSS)

securedrop_client/logic.py

@@ -485,7 +485,7 @@ def update_sources(self):
         """
         sources = list(storage.get_local_sources(self.session))
         if sources:
-            sources.sort(key=lambda x: x.last_updated, reverse=True)
+            sources.sort(key=lambda x: x.last_updated)
         self.gui.show_sources(sources)
 
     def on_update_star_success(self, result) -> None:

tests/gui/test_widgets.py

@@ -16,13 +16,13 @@
 
 from securedrop_client import db, logic
 from securedrop_client.export import ExportError, ExportStatus
-from securedrop_client.gui.widgets import MainView, SourceList, SourceWidget, LoginDialog, \
-    SpeechBubble, MessageWidget, ReplyWidget, FileWidget, ConversationView, \
+from securedrop_client.gui.widgets import MainView, SourceList, SourceWidget, SecureQLabel, \
+    LoginDialog, SpeechBubble, MessageWidget, ReplyWidget, FileWidget, ConversationView, \
     DeleteSourceMessageBox, DeleteSourceAction, SourceMenu, TopPane, LeftPane, SyncIcon, \
     ErrorStatusBar, ActivityStatusBar, UserProfile, UserButton, UserMenu, LoginButton, \
     ReplyBoxWidget, ReplyTextEdit, SourceConversationWrapper, StarToggleButton, LoginOfflineLink, \
     LoginErrorBar, EmptyConversationView, FramelessDialog, ExportDialog, PrintDialog, \
-    PasswordEdit, SecureQLabel, SourceProfileShortWidget
+    PasswordEdit, SourceProfileShortWidget
 from tests import factory
 
 
@@ -734,15 +734,31 @@ def test_EmptyConversationView_show_no_source_selected_message(mocker):
         '• Send a response\n')
 
 
-def test_SourceList_update(mocker):
+def test_SourceList_get_current_source(mocker):
     """
-    Check the items in the source list are cleared and a new SourceWidget for
-    each passed-in source is created along with an associated QListWidgetItem.
+    Maintains the selected item if present in new list
+    """
+    sl = SourceList()
+    sl.controller = mocker.MagicMock()
+    sources = [factory.Source(), factory.Source()]
+    sl.update(sources)
+    sl.setCurrentItem(sl.itemAt(1, 0))  # select source2
+
+    current_source = sl.get_current_source()
+
+    assert current_source.id == sources[1].id
+
+
+def test_SourceList_update_adds_new_sources(mocker):
+    """
+    Check a new SourceWidget for each passed-in source is created and no widgets are cleared or
+    removed.
     """
     sl = SourceList()
 
     sl.clear = mocker.MagicMock()
-    sl.addItem = mocker.MagicMock()
+    sl.insertItem = mocker.MagicMock()
+    sl.takeItem = mocker.MagicMock()
     sl.setItemWidget = mocker.MagicMock()
     sl.controller = mocker.MagicMock()
     sl.setCurrentItem = mocker.MagicMock()
@@ -755,91 +771,115 @@ def test_SourceList_update(mocker):
     sources = [mocker.MagicMock(), mocker.MagicMock(), mocker.MagicMock(), ]
     sl.update(sources)
 
-    sl.clear.assert_called_once_with()
     assert mock_sw.call_count == len(sources)
     assert mock_lwi.call_count == len(sources)
-    assert sl.addItem.call_count == len(sources)
+    assert sl.insertItem.call_count == len(sources)
     assert sl.setItemWidget.call_count == len(sources)
     assert len(sl.source_widgets) == len(sources)
     assert sl.setCurrentItem.call_count == 0
+    sl.clear.assert_not_called()
+    sl.takeItem.assert_not_called()
+    mock_sw.deleteLater.assert_not_called()
 
 
-def test_SourceList_update_deleted_source(mocker):
+def test_SourceList_update_maintains_selection(mocker):
     """
-    When the SourceList is updated after deleting a source, check that we
-    do not keep references to deleted source widgets on the source_widgets
-    dict.
+    Maintains the selected item if present in new list
     """
     sl = SourceList()
-
-    sl.clear = mocker.MagicMock()
-    sl.addItem = mocker.MagicMock()
-    sl.setItemWidget = mocker.MagicMock()
     sl.controller = mocker.MagicMock()
-    sl.setCurrentItem = mocker.MagicMock()
-
-    mock_sw = mocker.MagicMock()
-    mock_lwi = mocker.MagicMock()
-    mocker.patch('securedrop_client.gui.widgets.SourceWidget', mock_sw)
-    mocker.patch('securedrop_client.gui.widgets.QListWidgetItem', mock_lwi)
+    sources = [factory.Source(), factory.Source()]
+    sl.update(sources)
 
-    sources = [mocker.MagicMock(), mocker.MagicMock(), mocker.MagicMock(), ]
+    sl.setCurrentItem(sl.itemAt(0, 0))  # select the first source
     sl.update(sources)
 
-    sources.pop()
+    assert sl.currentItem()
+    assert sl.itemWidget(sl.currentItem()).source.id == sources[0].id
+
+    sl.setCurrentItem(sl.itemAt(1, 0))  # select the second source
     sl.update(sources)
 
-    assert len(sl.source_widgets) == len(sources)
+    assert sl.currentItem()
+    assert sl.itemWidget(sl.currentItem()).source.id == sources[1].id
 
 
-def test_SourceList_update_with_pre_selected_source(mocker):
+def test_SourceList_update_with_pre_selected_source_maintains_selection(mocker):
     """
-    If there's a source already selected. It remains selected after update.
+    Check that an existing source widget that is selected remains selected.
     """
     sl = SourceList()
+    sl.controller = mocker.MagicMock()
+    sl.update([factory.Source(), factory.Source()])
+    second_item = sl.itemAt(1, 0)
+    sl.setCurrentItem(second_item)  # select the second source
+    mocker.patch.object(second_item, 'isSelected', return_value=True)
 
-    sl.clear = mocker.MagicMock()
-    sl.addItem = mocker.MagicMock()
-    sl.setItemWidget = mocker.MagicMock()
+    sl.update([factory.Source(), factory.Source()])
+
+    assert second_item.isSelected() is True
+
+
+def test_SourceList_update_removes_selected_item_results_in_no_current_selection(mocker):
+    """
+    Check that no items are currently selected if the currently selected item is deleted.
+    """
+    sl = SourceList()
     sl.controller = mocker.MagicMock()
-    sl.setCurrentItem = mocker.MagicMock()
-    sl.currentRow = mocker.MagicMock(return_value=1)
-    sl.item = mocker.MagicMock()
-    sl.item().isSelected.return_value = True
-    mock_source = mocker.MagicMock()
-    sl.get_current_source = mocker.MagicMock(return_value=mock_source)
+    sl.update([factory.Source(uuid='new'), factory.Source(uuid='newer')])
 
-    mock_sw = mocker.MagicMock()
-    mock_lwi = mocker.MagicMock()
-    mocker.patch('securedrop_client.gui.widgets.SourceWidget', mock_sw)
-    mocker.patch('securedrop_client.gui.widgets.QListWidgetItem', mock_lwi)
+    sl.setCurrentItem(sl.itemAt(0, 0))  # select source with uuid='newer'
+    sl.update([factory.Source(uuid='new')])  # delete source with uuid='newer'
 
-    sources = [mocker.MagicMock(), mock_source, mocker.MagicMock(), ]
-    sl.update(sources)
+    assert not sl.currentItem()
 
-    sl.clear.assert_called_once_with()
-    assert mock_sw.call_count == len(sources)
-    assert mock_lwi.call_count == len(sources)
-    assert sl.addItem.call_count == len(sources)
-    assert sl.setItemWidget.call_count == len(sources)
-    assert len(sl.source_widgets) == len(sources)
-    assert sl.setCurrentItem.call_count == 1
 
+def test_SourceList_update_removes_item_from_end_of_list(mocker):
+    """
+    Check that the item is removed from the source list and dict if the source no longer exists.
+    """
+    sl = SourceList()
+    sl.controller = mocker.MagicMock()
+    sl.update([
+        factory.Source(uuid='new'), factory.Source(uuid='newer'), factory.Source(uuid='newest')])
+    assert sl.count() == 3
+    sl.update([factory.Source(uuid='newer'), factory.Source(uuid='newest')])
+    assert sl.count() == 2
+    assert sl.itemWidget(sl.item(0)).source.uuid == 'newest'
+    assert sl.itemWidget(sl.item(1)).source.uuid == 'newer'
+    assert len(sl.source_widgets) == 2
 
-def test_SourceList_maintains_selection(mocker):
+
+def test_SourceList_update_removes_item_from_middle_of_list(mocker):
     """
-    Maintains the selected item if present in new list
+    Check that the item is removed from the source list and dict if the source no longer exists.
     """
     sl = SourceList()
-    sources = [factory.Source(), factory.Source()]
-    sl.setup(mocker.MagicMock())
-    sl.update(sources)
+    sl.controller = mocker.MagicMock()
+    sl.update([
+        factory.Source(uuid='new'), factory.Source(uuid='newer'), factory.Source(uuid='newest')])
+    assert sl.count() == 3
+    sl.update([factory.Source(uuid='new'), factory.Source(uuid='newest')])
+    assert sl.count() == 2
+    assert sl.itemWidget(sl.item(0)).source.uuid == 'newest'
+    assert sl.itemWidget(sl.item(1)).source.uuid == 'new'
+    assert len(sl.source_widgets) == 2
 
-    sl.setCurrentItem(sl.itemAt(0, 0))
-    sl.update(sources)
 
-    assert sl.currentItem()
-    assert sl.itemWidget(sl.currentItem()).source.id == sources[0].id
+def test_SourceList_update_removes_item_from_beginning_of_list(mocker):
+    """
+    Check that the item is removed from the source list and dict if the source no longer exists.
+    """
+    sl = SourceList()
+    sl.controller = mocker.MagicMock()
+    sl.update([
+        factory.Source(uuid='new'), factory.Source(uuid='newer'), factory.Source(uuid='newest')])
+    assert sl.count() == 3
+    sl.update([factory.Source(uuid='new'), factory.Source(uuid='newer')])
+    assert sl.count() == 2
+    assert sl.itemWidget(sl.item(0)).source.uuid == 'newer'
+    assert sl.itemWidget(sl.item(1)).source.uuid == 'new'
+    assert len(sl.source_widgets) == 2
 
 
 def test_SourceList_set_snippet(mocker):

tests/test_logic.py

@@ -537,7 +537,7 @@ def test_Controller_update_sources(homedir, config, mocker):
     co = Controller('http://localhost', mock_gui, mock_session_maker, homedir)
 
     mock_storage = mocker.patch('securedrop_client.logic.storage')
-    source_list = [factory.Source(last_updated=2), factory.Source(last_updated=1)]
+    source_list = [factory.Source(last_updated=1), factory.Source(last_updated=2)]
     mock_storage.get_local_sources.return_value = source_list
 
     co.update_sources()