update gui instead of sync when file missing #724
Conversation
sssoleileraaa
requested review from
kushaldas and
redshiftzero
as code owners
securedrop_client/logic.py
Outdated
| logger.debug('Cannot find {} in the data directory. File does not exist.'.format( | ||
| file.original_filename)) | ||
| storage.update_missing_files(self.data_dir, self.session) | ||
| self.session.refresh(file) |
There was a problem hiding this comment.
I don't think this updates the UI until the conversation view is redrawn for other reasons, i.e. if we click off this source and back onto the source corresponding to the missing file or if a sync happens to finish (btw to quickly see what parts of the UI are updating naturally even without network I've been commenting out this adding of the metadata sync job).
I think we want to make sure that the state of the UI is representing the state of the locally stored data even without frequent network syncs, since it's expected that even in the best conditions there will be multi-second delay, let me know what you think
There was a problem hiding this comment.
so i removed the refresh line since it is unnecessary.
also, just an explanation of what i'm trying to do in this pr: instead of relying on sync_api, we now call update_missing_files directly when we learn that the file does not exist. this made it so we could remove 3 unnecessary sync_api calls and you can see that it still works:
There was a problem hiding this comment.
oops that gif was with the self.session.refresh(file) line of code, which i removed. here's a gif without self.session.refresh(file):
There was a problem hiding this comment.
ah, there was a call to sync_api from the GUI... that explains why this was working. since we don't want calls to sync_api, i updated the GUI to receive a file_missing signal from the Controller instead
|
looks like this needs a rebase now that we have the ci fix from earlier this week |
sssoleileraaa
force-pushed
the
no-sync-on-missing-files
branch
from
fa7e4e0
to
71d7cf3
Compare
sssoleileraaa
force-pushed
the
no-sync-on-missing-files
branch
from
71d7cf3
to
3bc7042
Compare
|
@rmol thanks for the offline feedback, this is ready for review |
There was a problem hiding this comment.
looks great! my comments are addressed
@rmol gonna let you check this one out too
Description
Closes #670
Test Plan
Repeat steps 1-3 for export and print
Known issue
Currently refreshing the file object in the session clears the error status message so I removed the error message for now, see: #723