Re-order widgets in the SourceList on update. #1013
Conversation
ntoll
requested review from
sssoleileraaa,
kushaldas and
redshiftzero
as code owners
| my_ts = arrow.get(me.source.last_updated) | ||
| other_ts = arrow.get(them.source.last_updated) | ||
| return my_ts < other_ts | ||
| return True |
There was a problem hiding this comment.
that sounds good to me @redshiftzero
sssoleileraaa
force-pushed
the
reorder-sources-on-update-by-timestamp
branch
from
5c3bb65 to
807783f
Compare
There was a problem hiding this comment.
I like this minimal approach. It looks like sources are moved to the top for new files and messages but not for replies. I didn't see any mention of this, so maybe my assumption is wrong that if you reply to a source it should bring that source up to the top. I think after a reply successfully makes it to the server and we move it from draft to the replies table, we should update the source list (actually surprised this isn't happening already so I'll need to investigate). Also, I'll test this on qubes with 400 and 1000 sources.
Update
After discussing with @ninavizz and @eloquence and recalling a similar previous conversation I see why successful replies are not moved to the top, but once they are considered "last activity" of a source, then they will be. This also means that the timestamp we display in the preview will be correct except for replies, since the last_updated timestamp will not take into account replies. We will need to open an issue to make this really clear since it's easy to forget this detail.
| self.insertItem(0, list_item) | ||
| list_item.setSizeHint(new_source.sizeHint()) | ||
| self.setItemWidget(list_item, new_source) | ||
|
|
||
| # Sort..! | ||
| self.sortItems(Qt.DescendingOrder) |
There was a problem hiding this comment.
since we're resorting the entire source list for every sync, we'll need to test this against 400 and 1000 sources to make sure things don't freeze up
There was a problem hiding this comment.
lgtm, i kept running into this issue so I could not test with 1000 sources in qubes. I was able to test with 20 sources and things worked as expected. Since we have separate issues for improving scalability, we can address performance bottlenecks there.
Description
Fixes #914.
Ensure the widgets are ordered by timestamp on update in the source list. Screenie of the behaviour:
Implementation of this was interesting. I was quite a way through doing something in the
updatemethod since thesourceslist passed into the method is in the correct order. However, I took a detour and wondered if Qt had some way to order widgets in a QListWidget. Turns out that it does and so I went for this far simpler code. Also, this will be a faster ordering since the ordering is being done in the Qt (C++) level rather than in pure Python.Test Plan
Updated the unit tests to patch the new
SourceListWidgetItemclass. Eyeball Mk.1 to ensure the order is correct (see screenie). 👀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: