fix remaining app crasher bugs #890
Conversation
redshiftzero
force-pushed
the
857-conversation-view-ref
branch
from
c40988c
to
c9170b8
Compare
redshiftzero
force-pushed
the
857-conversation-view-ref
branch
from
c9170b8
to
cc120bc
Compare
redshiftzero
changed the title
redshiftzero
force-pushed
the
857-conversation-view-ref
branch
from
9553546
to
22ce630
Compare
We have a loop at the end of ConversationView.update() that removes widgets from the conversation view that are associated with an item that is no longer in the source collection. We should also ensure that we remove python references to these widgets from the current_messages dict.
And remove its reference in the dict we store on the MainView
Since we call update_sources() in both the source-deleted-due-to-sync and source-deleted-due-to-source-deletion-job-completion scenarios, we can put the logic to remove the corresponding SourceConversationWrapper in one place.
redshiftzero
force-pushed
the
857-conversation-view-ref
branch
from
22ce630
to
8ce3258
Compare
securedrop_client/gui/widgets.py
Outdated
| logger.debug('Deleting SourceConversationWrapper for {}'.format(source_uuid)) | ||
| conversation_wrapper = self.source_conversations[source_uuid] | ||
| conversation_wrapper.deleteLater() | ||
| self.source_conversations.pop(source_uuid) |
There was a problem hiding this comment.
nit: you can just use del if you don't need the return value
| @@ -692,7 +692,10 @@ def show_sources(self, sources: List[Source]): | |||
| self.empty_conversation_view.show_no_source_selected_message() | |||
| self.empty_conversation_view.show() | |||
|
|
|||
| self.source_list.update(sources) | |||
| deleted_sources = self.source_list.update(sources) | |||
| for source_uuid in deleted_sources: | |||
There was a problem hiding this comment.
this change makes a lot of sense to me, before we would delete the source items without deleting the conversation wrappers
| @@ -1896,6 +1921,7 @@ def __init__( | |||
|
|
|||
| self.controller = controller | |||
| self.file = self.controller.get_file(file_uuid) | |||
| self.uuid = file_uuid | |||
There was a problem hiding this comment.
can we remove self.file since we keep its uuid now?
There was a problem hiding this comment.
unfortunately we still need this as the initializer calls _set_file_state which uses self.file
| for item_widget in current_conversation.values(): | ||
| self.current_messages.pop(item_widget.uuid) |
There was a problem hiding this comment.
since we already have a source dict and a conversation wrapper dict, why do we need a current_messages dict?
There was a problem hiding this comment.
they each serve different purposes on different widgets: one is on the source list mapping individual sources to their SourceWidget in the source list, one is on the main view mapping individual sources to SourceConversationWrapper parent widgets, and this one on the conversation view which maps message/file/reply items to their corresponding widget.
This dict serves the purpose of on a per-source basis tracking which widgets are displayed: when we draw the view, we only draw items that aren't already in the dict.
Description
Similar fix to #887 but for the remaining crasher bugs
Fixes #859
Fixes #857
Test Plan
LOGLEVEL=DEBUG ./run.shSourceConversationWrapper).SourceConversationWrapperwas deleted:Checklist
If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:
If these changes add or remove files other than client code, packaging logic (e.g., the AppArmor profile) may need to be updated. Please check as applicable: