Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

no longer delete all source widgets each update #893

Merged
merged 2 commits into from
Mar 11, 2020

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Mar 7, 2020

Description

Fixes #905
Fixes #352

We currently delete all source widgets and recreated them every time a user logs in, a user deletes a source, and whenever the client syncs with the server.

This PR makes it so only add source widgets when there are new sources and delete source widgets when the source has been deleted.

Performance testing on a large source list will need to be done over the next day or so. Also we should check if this helps with the flicker issue.

Test Plan

Make sure there are no regressions. This is a refactor so the source list should behave the same.

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:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

@sssoleileraaa sssoleileraaa force-pushed the no-longer-recreate-existing-source-widgets branch from 438da27 to cb4d8dc Compare March 9, 2020 19:04
@sssoleileraaa
Copy link
Contributor Author

rebased to keep this still ready for review

@sssoleileraaa
Copy link
Contributor Author

sssoleileraaa commented Mar 9, 2020

i gathered some perf data and it looks like update_sources which updates the source list is now dramatically faster, and you can see how the GUI doesn't freeze up anymore when you select between sources with a source list of 400.

n=100 sources

before

0.46784329414367676
0.2571258544921875
0.2532978057861328
0.2886519432067871
0.2683417797088623
0.2562129497528076
0.2976980209350586
0.2644331455230713
0.2614705562591553
0.261214017868042
0.26711487770080566
0.2992699146270752
0.2701082229614258

after

0.4843146800994873
0.0020008087158203125
0.0019850730895996094
0.002037525177001953
0.002022981643676758
0.0020155906677246094
0.0021219253540039062
0.0020911693572998047
0.002016782760620117
0.0019176006317138672
0.002031087875366211
0.0019462108612060547
0.002020120620727539

n=400 sources

before

0.5068690776824951
1.8598730564117432
3.211728811264038
3.6987013816833496
3.1780104637145996
4.074876308441162
3.9540183544158936
(i had to stop because each sync was taking so long)

after

1.827559471130371
0.008049488067626953
0.01360464096069336
0.01093912124633789
0.01216745376586914
0.00998687744140625
0.010653972625732422
0.011968851089477539
0.010234355926513672
0.011265039443969727
0.016140460968017578
0.011561155319213867
0.010588884353637695
0.012189388275146484
0.011542081832885742
0.011640310287475586
0.012189626693725586
0.010349273681640625

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great, a few thoughts inline

securedrop_client/gui/widgets.py Outdated Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confirming: since we no longer need to redraw the whole view, we don't need this logic to preserve the current source selection

self.source_widgets[source.uuid] = new_source

list_item = QListWidgetItem()
self.insertItem(0, list_item)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensuring that new sources are placed at the top of the list ✅

securedrop_client/logic.py Show resolved Hide resolved
@redshiftzero redshiftzero mentioned this pull request Mar 10, 2020
6 tasks
@sssoleileraaa
Copy link
Contributor Author

just need to update tests and rebase but i addressed @redshiftzero's PR comment https://github.com/freedomofpress/securedrop-client/pull/893/files#r390031326 about this introducing an ObjectionDeletionError by storing the source uuid on the widget so that we can delete the widget without access the db source object. as follow-up we can completely remove the source object from the source widget, but this will require updating set_snippet and a few other areas of the code.

@sssoleileraaa sssoleileraaa force-pushed the no-longer-recreate-existing-source-widgets branch from 3ed5c88 to 12d5905 Compare March 10, 2020 23:50
@sssoleileraaa sssoleileraaa force-pushed the no-longer-recreate-existing-source-widgets branch 2 times, most recently from d3c5d0a to 571b6a0 Compare March 10, 2020 23:58
@sssoleileraaa sssoleileraaa force-pushed the no-longer-recreate-existing-source-widgets branch from 571b6a0 to cd7a585 Compare March 10, 2020 23:59
@sssoleileraaa
Copy link
Contributor Author

okay this is ready now

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my comments are addressed, thanks

@redshiftzero redshiftzero merged commit efe68bd into master Mar 11, 2020
@redshiftzero redshiftzero deleted the no-longer-recreate-existing-source-widgets branch March 11, 2020 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants