refactor: Remove FolderMeaning and target_folder - #8456
Conversation
Hocuri
changed the title
|
|
||
| impl Session { | ||
| /// Synchronizes UIDs for all folders. | ||
| pub(crate) async fn resync_folders(&mut self, context: &Context) -> Result<()> { |
There was a problem hiding this comment.
We only look at one folder, so, resync_folder_uids() (which I renamed to resync_uids_with_server()) can be called directly, rather than needing this intermediate function that iterates over all the folders.
| "INSERT INTO imap (transport_id, rfc724_mid, folder, uid, uidvalidity, target) | ||
| VALUES (?, ?, ?, ?, ?, ?) | ||
| ON CONFLICT(transport_id, folder, uid, uidvalidity) | ||
| DO UPDATE SET rfc724_mid=excluded.rfc724_mid, | ||
| target=excluded.target", | ||
| (transport_id, rfc724_mid, folder, uid, uid_validity, target), | ||
| (transport_id, rfc724_mid, folder, uid, uid_validity, folder), |
There was a problem hiding this comment.
This SQL statement might be the only thing that needs attentive review in this PR, the rest is just removing unused functions, enums, and constants
There was a problem hiding this comment.
your PR doesn't change it, but the "DELETE FROM" could have empty target (indicating deletion) and then these deleted messages become un-deleted, right? However, re-sync is rare, and it's a pre-existing issue. Would probably need msgs to also carry a 'target' computed from whether the message was marked deleted around line 821.
There was a problem hiding this comment.
I'm just trying to understand the whole deletion-on-server vs deletion-locally logic.
msgs already has a deleted column, but it's only checked if the message later arrives via IMAP (in imap.rs with message::rfc724_mid_exists_ex(context, message_id, "deleted=1")). And it is only set by ChatId::delete_ex() and MsgId::trash(). It is:
- not set by
delete_expired_messages()(which deletes ephemeral messages locally) - not checked by
delete_expired_imap_messages()(which marks ephemeral messages for deletion on IMAP, and if bcc_self is off it marks all fully downloaded messages for deletion on IMAP [non non-chatmail accounts, only encrypted ones])
There actually is another bug here that prevents ephemeral messages from being deleted on the server: delete_expired_messages() deletes the message locally, then delete_expired_imap_messages() doesn't find the message anymore, and can't mark it for deletion. I could reproduce this bug.
If we make delete_expired_messages() set the deleted flag and make delete_expired_imap_messages() check it, then this should fix both the bug @hpk42 just saw as well as the one I just found.
Note that insert_tombstone() also creates a trashed message without the deleted flag set; this is probably fine.
Hocuri
changed the title
hpk42
left a comment
hpk42
left a comment
There was a problem hiding this comment.
great, more removals :) PR looks good to me including the SQL statement. Just a note about a pre-existing issue that should be easy to fix.
| "INSERT INTO imap (transport_id, rfc724_mid, folder, uid, uidvalidity, target) | ||
| VALUES (?, ?, ?, ?, ?, ?) | ||
| ON CONFLICT(transport_id, folder, uid, uidvalidity) | ||
| DO UPDATE SET rfc724_mid=excluded.rfc724_mid, | ||
| target=excluded.target", | ||
| (transport_id, rfc724_mid, folder, uid, uid_validity, target), | ||
| (transport_id, rfc724_mid, folder, uid, uid_validity, folder), |
There was a problem hiding this comment.
your PR doesn't change it, but the "DELETE FROM" could have empty target (indicating deletion) and then these deleted messages become un-deleted, right? However, re-sync is rare, and it's a pre-existing issue. Would probably need msgs to also carry a 'target' computed from whether the message was marked deleted around line 821.
2 participants
The
FolderMeaningand thetarget_folder()function and related code were needed for the message moving logic. The message moving logic isn't needed anymore, so, this PR is a first step to remove it.We only ever look at one folder by now (usually the Inbox folder), so, any message moving logic isn't needed anymore because we will never notice a message that is supposed to be moved.
The
targetcolumn of theimaptable is mostly the same as thefoldercolumn now. Except when the message is supposed to be deleted, then it's an empty string.