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.

Sign up for GitHub

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

Jump to bottom

Refactor folder sync #326

Merged
merged 54 commits into from
Aug 22, 2017
Merged

Refactor folder sync #326

merged 54 commits into from
Aug 22, 2017

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Feb 28, 2017 •
edited
Loading

Partially mostly implements #318
Fixes #244, #318, #335, #290

  • Sync

    • Use horde sync to get new/changed/vanished messages
    • Implement message mapper (reuse old code if possible)
    • Implement sync mechanism in the client
    • Implement sync for the unified inbox
    • Update unified inbox if individual ones change
    • Implement sync for search mailboxes – should work now fingers crossed
    • Implement pagination for search mailboxes
    • test search

  • Pagination

    • Implement date-based cursor-based pagination on the server
    • Implement new pagination on the client
    • Implement pagination for the unified inbox

  • Necessary follow-ups

    • Kill background sync. It's extremely buggy in its current form. If the new horde sync approach is fast enough, I'm planning to just let the current folder view refresh its content once a minute. If the folder is a specific inbox, we refresh the unified one. This should cover everything.

@ChristophWurst
Copy link
Member Author

This is gonna be a big one. I'm already sorry for that, reviewers. I've tried to split it up but this I'm afraid that would be 5x more work.

@ChristophWurst
Copy link
Member Author

Wow, my IMAP server doesn't even provide the capabilities that make horde sync fast (MODSEQ for example) and still it is noticeably faster 😮 🚀

@ChristophWurst
Copy link
Member Author

Unified inbox synchronization and pagination seems to work. Still, I have to tweak it a bit to be as efficient as I desired it (remove some redundant requests). Looks promising so far 🚀

@ChristophWurst
Move imap related classes to OCA\Mail\IMAP namespace
5bd5eee
@ChristophWurst
Add request-response based synchronizer around horde sync
d52e2f2 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Implementd and test imap message mapper
95a7090 
This class shall help connect the IMAP world with the OOP
world.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Send client sync request to refresh messages; cursor-based pagination
55ff914 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Implement date- and cursor-based pagination
7a576bc 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Implement client-side horde sync mechanism
7b39eb7 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Sync single and unified mailboxes efficiently
4b7c9f8 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Remove debug statement
a51f0cb 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Minor code cleanup
c790efa 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Remove debug messages
70c2170 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Synchronize and paginate the unified inbox
332ac83 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Fix php docblock
1a11059 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Always log message loading errors and resolve promise correctly
8a1b3df 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Make messages loadable again
49e11dc 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Don't rely on global state variables when chaning clags/deleting mess…
bdcc9f1 
…ages

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Use message object directly instead of buggy lookup
77f161c 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Only limit search to cursor time if it's actually set
46be5f3 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Remove dead pagination code
a1ff649 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Use proper DateTime object as cursor
d8300e0 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Add a sync token to the special favs folder
9145dc9 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst changed the title [WIP] Refactor folder sync Refactor folder sync May 22, 2017
@ChristophWurst
Merge branch 'master' of github.com:/nextcloud/mail into refactor/fol…
86541ab 
…der-sync
@ChristophWurst
Fix imported class
07a90df 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Also test changed and vanished message sync
4666961 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst mentioned this pull request Aug 7, 2017
Merged
ChristophWurst added a commit that referenced this pull request Aug 21, 2017
@ChristophWurst
Use another IMAP docker image that allows more connections
b16a826 
Since CI builds of #326 failed and I could reproduce this locally,
it turned out that running single tests or tests of a single directory
was no problem. However, executing all tests at once led to failing
IMAP connections. Apparently an increased number of allowed IMAP
connections fixes the problem (locally).

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst mentioned this pull request Aug 21, 2017
Merged
@ChristophWurst
Copy link
Member Author

I think I was able to debug why tests fail here: #445

@ChristophWurst
Copy link
Member Author

I think I was able to debug why tests fail here: #445

Voilà: All checks have passed

@lcalaresu
Copy link
Member

Voilà: All checks have passed

Yay! that's great!!
Coverage also starts to looks interesting :))

It starts to look great. No big issues for the last few days except for known issues above...
Maybe once on my phone but wasn't able to reproduce it so... 🤔

Anyway~~ Should update to a more recent version.
Is there any packaged version of the latest version? Coz I can't generate the product right now :/

@ChristophWurst
Copy link
Member Author

Is there any packaged version of the latest version?

mail.tar.gz

@ChristophWurst
Copy link
Member Author

When using a gmail account, I cannot open the [Gmail] folder

Solved meanwhile (can't remember whether it happened in this PR or another one) and now those special folders cannot be opened anymore. However, it's still possible to expand it to open subfolders.

On a personal server, I have a JS error every time I change the folder (even base folders like sent, inbox, etc.):

Please check whether this is still the case, it could have been fixed meanwhile.

Are those the only two issues you have?

@ChristophWurst
Copy link
Member Author

No big issues for the last few days except for known issues above...

If it's just minor things that I'd say we can fix them later. IMO this PR is open for quite long time and it's about time to finish this ;-)

@ChristophWurst ChristophWurst self-assigned this Aug 22, 2017
@lcalaresu
Copy link
Member

If it's just minor things that I'd say we can fix them later. IMO this PR is open for quite long time and it's about time to finish this ;-)

I do agree with that one ^^
Since I installed the version of the mail app from yesterday, I haven't had any probleme yet ^^

@ChristophWurst
Copy link
Member Author

I do agree with that one ^^
Since I installed the version of the mail app from yesterday, I haven't had any probleme yet ^^

Yay, thanks a lot for testing. I'll merge this PR now. @jancborchardt please file issues for the issues you had if you can still reproduce them. Thanks everyone :)

@ChristophWurst ChristophWurst merged commit 632e9ed into master Aug 22, 2017
@ChristophWurst ChristophWurst deleted the refactor/folder-sync branch August 22, 2017 13:34
@ChristophWurst ChristophWurst mentioned this pull request Aug 22, 2017
Closed
This was referenced Aug 23, 2017
Closed
Closed
@lock
Copy link

lock bot commented Nov 20, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and questions.

@lock lock bot locked and limited conversation to collaborators Nov 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jancborchardt jancborchardt jancborchardt left review comments

@Gomez Gomez Awaiting requested review from Gomez

Assignees

@ChristophWurst ChristophWurst

Labels
3. to review enhancement
Projects
None yet
Milestone
0.7
Development

Successfully merging this pull request may close these issues.
4 participants
@ChristophWurst @Gomez @jancborchardt @lcalaresu