refactor: Start to use imap-codec. #223

duesee · 2023-05-31T03:41:07+03:00

duesee commented

2023-05-31 03:41:07 +03:00

This PR introduces imap-codec to meli. I intended to replace all send_command instances, but there are a few remainig ones where I don't fully understand what should happen :-)

Can someone take a look and tell me what they think? There are a few TODOs that I put on my imap-codec list to make things more pleasant.

Also, there seems to be a bug because imap-codec complains that we try to send a UID == 0.

On a positive side: I think this may fix some instances where literals were not handled before. At least it appeared to me that it could happen that values are encoded as quoted strings, although we need to use literals.

I will work on my setup and try to test this better.

This PR introduces imap-codec to meli. I intended to replace all `send_command` instances, but there are a few remainig ones where I don't fully understand what should happen :-) Can someone take a look and tell me what they think? There are a few `TODO`s that I put on my imap-codec list to make things more pleasant. Also, there seems to be a bug because imap-codec complains that we try to send a `UID == 0`. On a positive side: I think this may fix some instances where literals were not handled before. At least it appeared to me that it could happen that values are encoded as quoted strings, although we need to use literals. I will work on my setup and try to test this better.

duesee force-pushed duesee/use_imap_codec from 65578b06bb to 82787ced43

2023-06-01 16:56:20 +03:00

Compare

duesee force-pushed duesee/use_imap_codec from 82787ced43 to 6dfb03aaa6

2023-06-01 18:01:52 +03:00

Compare

duesee changed title from ~~WIP: refactor: Start to use imap-codec.~~ to refactor: Start to use imap-codec.

2023-06-01 18:02:05 +03:00

duesee changed title from ~~refactor: Start to use imap-codec.~~ to WIP: refactor: Start to use imap-codec.

2023-06-01 18:10:23 +03:00

Manos Pitsidianakis reviewed 2023-06-01 19:36:15 +03:00

Manos Pitsidianakis left a comment

I love how tidy and clean this looks :) Didn't test it yet though, does it work/compile?

❤️ 1

melib/src/backends/imap/cache/sync.rs Outdated

													
				@ -237,1 +250,3 @@

				        }

				            format!("1:{}", max_uid)

				        };

				        self.send_command_imap_codec(CommandBody::fetch(

Manos Pitsidianakis commented

2023-06-01 19:23:04 +03:00

Is it okay if we use sequences here instead of UID?

epilys marked this conversation as resolved

melib/src/backends/imap/cache/sync.rs Outdated

													
				@ -687,1 +706,3 @@

				                .await?;

				            self.send_command_imap_codec(CommandBody::status(

				                mailbox_path,

				                vec![StatusAttribute::UidNext],

Manos Pitsidianakis commented

2023-06-01 19:24:11 +03:00

this could probably be a slice &[StatusAttribute] unless CommandBody::status needs the allocation

this could probably be a slice `&[StatusAttribute]` unless `CommandBody::status` needs the allocation

duesee commented

2023-06-03 20:07:39 +03:00

Poster

Next version :-) https://github.com/duesee/imap-codec/issues/240 Can I add a TODO and resolve this in the next release?

epilys marked this conversation as resolved

melib/src/backends/imap/untagged.rs Outdated

													
				@ -90,3 +96,3 @@

				                        self.uid_store.msn_index.lock().unwrap().get(&mailbox_hash)

				                    );

				                    self.send_command("UID SEARCH 1:*".as_bytes()).await?;

				                    self.send_command_imap_codec(CommandBody::search(

Manos Pitsidianakis commented

2023-06-01 19:28:03 +03:00

Again same question about uid<->sequence

epilys marked this conversation as resolved

melib/src/email.rs Outdated

													
				@ -216,16 +260,20 @@ crate::declare_u64_hash!(EnvelopeHash);

				/// bytes into an `Attachment` object.

				#[derive(Clone, Serialize, Deserialize)]

				pub struct Envelope {

				    pub hash: EnvelopeHash,

Manos Pitsidianakis commented

2023-06-01 19:34:54 +03:00

Hm yeah. What if we made an IMAPEnvelope type we can convert into Envelopes? Envelope -> ImapEnvelope would be lossy though, if the header map is not included.

duesee commented

2023-06-02 02:30:37 +03:00

Poster

Sounds good to me.

I played a bit with response handling but it just occured to me that we could do this in a later PR as it's not really required when replacing the "sending side". Shall we first try to get the sending side in, i.e, fully replace send_command and tackle responses later?

Sounds good to me. I played a bit with response handling but it just occured to me that we could do this in a later PR as it's not really required when replacing the "sending side". Shall we first try to get the sending side in, i.e, fully replace `send_command` and tackle responses later?

Manos Pitsidianakis commented

2023-06-02 10:39:30 +03:00

Ofc ofc

epilys marked this conversation as resolved

melib/src/email.rs Outdated

													
				@ -167,2 +175,4 @@

				}

				impl Flag {

				    // TODO: Remove this eventually.

Manos Pitsidianakis commented

2023-06-01 19:29:53 +03:00

Since imap-backend is a feature, we can make an imap specific trait that Flag implements if the feature is enabled

👍 1

duesee marked this conversation as resolved

melib/src/email.rs Outdated

													
				@ -169,0 +179,4 @@

				    pub(crate) fn derive_imap_codec_flags(&self) -> Vec<ImapCodecFlag> {

				        let mut flags = Vec::new();

				        if self.is_passed() {

Manos Pitsidianakis commented

2023-06-01 19:31:58 +03:00

Passed is from maildir http://cr.yp.to/proto/maildir.html

Flag "P" (passed): the user has resent/forwarded/bounced this message to someone else.

So not really meaningful in IMAP, it's there for legacy when parsing maildir and ignored in general.

Passed is from maildir http://cr.yp.to/proto/maildir.html > Flag "P" (passed): the user has resent/forwarded/bounced this message to someone else. So not really meaningful in IMAP, it's there for legacy when parsing maildir and ignored in general.

duesee marked this conversation as resolved

duesee commented

2023-06-02 02:36:17 +03:00

Poster

I love how tidy and clean this looks :) Didn't test it yet though, does it work/compile?

It compiles and seems to work a bit better with the fake-mail-server -- not sure if that's a good or bad sign :D -- because meli now shows some mails it didn't before. But I need to debug an X.509 issue first in order to test with dovecot/courier/... Will try on monday :-)

> I love how tidy and clean this looks :) Didn't test it yet though, does it work/compile? It compiles and seems to work a bit better with the fake-mail-server -- not sure if that's a good or bad sign :D -- because meli now shows some mails it didn't before. But I need to debug an X.509 issue first in order to test with dovecot/courier/... Will try on monday :-)

duesee commented

2023-06-02 14:35:59 +03:00

Poster

Had a bit time today and can use meli with a local Courier now :-)

Can't say for sure if everything is working as expected because it appears to me that there could be a bug. When I create a draft, and edit it, it seems that the body of the new draft is "merged" with the content of the old one.

But this happens on master, too. So I don't see any difference between master and duesee/use_imap_codec in this regard.

Had a bit time today and can use meli with a local Courier now :-) Can't say for sure if everything is working as expected because it appears to me that there could be a bug. When I create a draft, and edit it, it seems that the body of the new draft is "merged" with the content of the old one. But this happens on `master`, too. So I don't see any difference between `master` and `duesee/use_imap_codec` in this regard.

duesee force-pushed duesee/use_imap_codec from 6dfb03aaa6 to d22c00913f

2023-06-05 19:41:55 +03:00

Compare

duesee changed title from ~~WIP: refactor: Start to use imap-codec.~~ to refactor: Start to use imap-codec.

2023-06-05 19:42:55 +03:00

duesee force-pushed duesee/use_imap_codec from d22c00913f to a907f18667

2023-06-05 19:46:36 +03:00

Compare

duesee commented

2023-06-05 19:51:07 +03:00

Poster

I think this is ready to go :-) I don't really know what is supposed to work and what not but can't see a difference between master and this branch. So, while I don't feel too confident on this PR, I think it should be fine :-)

duesee force-pushed duesee/use_imap_codec from 8763ee23ee to f72056e6f1

2023-06-14 13:22:05 +03:00

Compare

duesee force-pushed duesee/use_imap_codec from f72056e6f1 to d58676292a

2023-06-14 13:26:47 +03:00

Compare

duesee reviewed 2023-06-14 13:28:33 +03:00

melib/src/backends/imap/connection.rs Outdated

													
				@ -39,6 +39,20 @@ use std::{

				};

				use futures::io::{AsyncReadExt, AsyncWriteExt};

				#[cfg(feature = "deflate_compression")]

duesee commented

2023-06-14 13:28:33 +03:00

Poster

Previously, CompressionAlgorithm wasn't feature-gated. This is change 1/2.

Previously, `CompressionAlgorithm` wasn't feature-gated. This is change 1/2.

epilys marked this conversation as resolved

duesee reviewed 2023-06-14 13:29:13 +03:00

melib/src/backends/imap/connection.rs Outdated

													
				@ -501,3 +505,3 @@

				    }

				    pub async fn send_command(&mut self, command: &[u8]) -> Result<()> {

				    pub async fn send_command(&mut self, body: CommandBody<'_>) -> Result<()> {