From 69efc043063d75f66bc78b01a632c2df8ec8ed6a Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Fri, 6 Mar 2020 14:51:33 -0800 Subject: [PATCH 1/2] no longer delete all source widgets each update --- securedrop_client/gui/widgets.py | 60 +++++------- securedrop_client/logic.py | 2 +- tests/gui/test_widgets.py | 158 +++++++++++++++++++------------ tests/test_logic.py | 2 +- 4 files changed, 127 insertions(+), 95 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 4cf972a6b..7c2de930f 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -854,41 +854,33 @@ 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] + 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] + 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.id == current_source_id and has_source_selected: - self.setCurrentItem(list_item) + if source.uuid not in widget_uuids: + new_source = SourceWidget(source) + new_source.setup(self.controller) + self.source_widgets[source.uuid] = new_source + + 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 diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index 996fc645d..44057f8e6 100644 --- a/securedrop_client/logic.py +++ b/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: diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index 07ab8f777..332fc657f 100644 --- a/tests/gui/test_widgets.py +++ b/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): diff --git a/tests/test_logic.py b/tests/test_logic.py index 00acc5530..b933aa3a4 100644 --- a/tests/test_logic.py +++ b/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() From cd7a585f890b65ecf0c0e139b8027cb217a187e1 Mon Sep 17 00:00:00 2001 From: Allie Crevier Date: Tue, 10 Mar 2020 16:46:59 -0700 Subject: [PATCH 2/2] if source was deleted do not access source --- securedrop_client/gui/widgets.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 7c2de930f..4df5aa947 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -858,19 +858,21 @@ def update(self, sources: List[Source]) -> List[str]: """ # 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_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] + 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())] + widget_uuids = [self.itemWidget(self.item(i)).source_uuid for i in range(self.count())] for source in sources: if source.uuid not in widget_uuids: new_source = SourceWidget(source) @@ -882,7 +884,6 @@ def update(self, sources: List[Source]) -> List[str]: 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): @@ -969,6 +970,7 @@ def __init__(self, source: Source): # Store source self.source = source + self.source_uuid = source.uuid # Set styles self.setStyleSheet(self.CSS)