elizagamedev / mujmap

Bridge for synchronizing email and tags between JMAP and notmuch
GNU General Public License v3.0
63 stars 11 forks source link

Refactoring: apply mailboxes & tags, not emails; don't hold notmuch messsage objects #21

Closed robn closed 2 years ago

robn commented 2 years ago

This could be two PRs, one with the first three commits and one with the other two, but they're all moving towards the same goal so here they are all together.

This started with #10, where we learned that we need to be getting the Message object from notmuch to make sure we don't get a stale filename list.

I initially thought to keep a HashMap<String,notmuch::Message> inside struct Local, but the problem is that all of the sync and update code dealt only with Email objects, and the Local object wasn't available. As I teased it apart, it became clear to me that local and remote had too much knowledge of each other's Email objects, and that I would be passing a Local into Remote::update, very odd.

So I peeled all that back and made local only deal with messages in the notmuch index and the maildir, and remote only deals with the server, with mailboxes and tags as the "intermediate" data ferried between the two.

Its not perfect; I think maybe remote could benefit from a similar lift (removing impl remote::Email), and as I said in a comment, I think the add-mailboxes-to-tags logic might belong somewhere other than sync, and if you look at #10, it does still reach out for thenotmuch::Message to get the filename list, which suggests sync still knows too much. But I think I'm content with this for now because it certainly does remove a lot of coupling.

In the end I didn't actually add that cache of messages because we rarely reuse them and its easy to just look them up. But local::get_message could easily be made to keep hold of them internally and loan them out, if that turned out to be a performance bottleneck in the future.

elizagamedev commented 2 years ago

Excellent work, I agree with all these changes. Thank you for cleaning this up.