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

Immediate update of snippets #752

Merged
merged 4 commits into from
Feb 4, 2020

Conversation

ntoll
Copy link
Contributor

@ntoll ntoll commented Jan 30, 2020

Description

Ready for review.

Fixes #707.

This is a preview. There are performance issues with the code as it stands (tested with 200 sources), but I wanted to give you a sense of the progress made so far.

It took a while to work around SqlAlchemy, but got there in the end.

  • On start, the snippets are updated as soon as the messages / replies etc are downloaded from the server.
  • If a new message comes in while the client is active, the snippet is appropriately updated.
  • When a new reply is sent and succeeds, the snippet is appropriately updated.
  • When a file is downloaded from the server, the snippet is appropriately updated.

Screenie of some of this here:

snippet_updates

Performance refactoring currently underway.

Test Plan

I've updated the unit tests. Please check with Eyeball mk.1 👀 for functionality described above.

Performance sanity check with 200 sources currently results in an unresponsive app while the messages / replies are downloading.

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

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:

  • I have submitted a separate PR to the packaging repo
  • No update to the packaging logic (e.g., AppArmor profile) is required for these changes
  • I don't know and would appreciate guidance

self.controller.message_ready.connect(self.set_snippet)
self.controller.reply_ready.connect(self.set_snippet)
self.controller.reply_succeeded.connect(self.set_snippet)
self.controller.file_ready.connect(self.set_snippet)

Copy link
Contributor

Choose a reason for hiding this comment

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

headsup: you may need to connect one more signal if this gets merged first

Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

wip pr looks good to me

@ntoll ntoll marked this pull request as ready for review February 3, 2020 13:47
@ntoll
Copy link
Contributor Author

ntoll commented Feb 3, 2020

I've managed to fix the unresponsive UI problem, fixed up the unit tests and tested with 200 sources to make sure it all works fine. Feedback most welcome..!

@rmol
Copy link
Contributor

rmol commented Feb 3, 2020

Looks good so far -- responsiveness is much improved, and the snippet is updating. I did notice a delay updating the message in the conversation pane: a box appeared for a new message but was empty, though the snippet under the source was updated with the message content ("This space intentionally left blank."):
image
This did self-correct after a few seconds, presumably at the next sync?

@rmol
Copy link
Contributor

rmol commented Feb 3, 2020

Replies can likewise stay blank for a bit after they're fetched and the snippet updated. Clicking on another source and back redraws them.

Downloading a file updated the snippet immediately, but the "downloading" spinner got stuck on, and this isn't cleared up by clicking another source then returning:

image

I should probably have mentioned that I'm testing on Qubes, under i3.

@rmol
Copy link
Contributor

rmol commented Feb 3, 2020

Same behavior observed under XFCE.

@ntoll
Copy link
Contributor Author

ntoll commented Feb 3, 2020

@rmol aha... thank you. I'll take a look. I've been able to see this too. I suspect the blanks are an interesting intermediate state where the message is downloaded, but not yet decrypted. I'll need to check.

@ntoll
Copy link
Contributor Author

ntoll commented Feb 3, 2020

I'll fix the downloading spinner in the morning.

@ntoll
Copy link
Contributor Author

ntoll commented Feb 3, 2020

@creviera please confirm the spinner behaviour is not related to this branch (but is the problem you saw when testing the decryption story on the client side). If this is "just a bug" and not anything to do with this branch, I'll just ignore it and let it be fixed in another ticket.

@sssoleileraaa
Copy link
Contributor

@ntoll #754 only occurs when the decryption fails, so this is unrelated to this PR

@ntoll
Copy link
Contributor Author

ntoll commented Feb 3, 2020

@creviera cool... so I won't fix in this PR. :-)

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

Works with different trys, for replies, or new messages. Approved from my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

snippets don't update when content of message/reply is updated
5 participants