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

feat: Sync chats deletion across devices #5007

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Nov 15, 2023

feat: Sync chats deletion across devices

Motivation: Now broadcast lists creation is synced across devices. Groups creation, in some means, too -- on a group promotion by a first message. So, to make this looking more feature-complete, sync chats deletion too.

An approach is to replay a user action on synchronised devices because the chat deletion is a complex procedure affecting several tables, self-chat, emitting the corresponding event etc.

Also this commit makes all deletions to happen in a transaction, to be on a safe side.

.await
.log_err(context)
.ok();
}

if chat.is_self_talk() {
Copy link
Collaborator Author

@iequidoo iequidoo Nov 15, 2023

Choose a reason for hiding this comment

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

I'd move this even before the actual deletion so that this message can't be missing if we fail halfway. But then the stock string should be changed to You are deleting the \"Saved messages\" chat... or smth like this

@iequidoo iequidoo marked this pull request as ready for review November 15, 2023 06:52
@hpk42
Copy link
Contributor

hpk42 commented Nov 15, 2023

general comment: please describe a description for your PRs, summarizing the motivation/reason and the approach/solution.

@iequidoo iequidoo marked this pull request as draft November 16, 2023 01:05
@iequidoo iequidoo marked this pull request as ready for review November 16, 2023 01:26
@iequidoo
Copy link
Collaborator Author

general comment: please describe a description for your PRs, summarizing the motivation/reason and the approach/solution.

Added a commit message. Yes, i hurried to share this work. Actually it should be a draft w/o good commit messages

@Simon-Laux
Copy link
Member

I have mixed feelings about chat deletion, users might expect it to delete all imap emails / messages from that chat too.

I wouldn't sync chat deletion before we sync message deletion.

@iequidoo
Copy link
Collaborator Author

I have mixed feelings about chat deletion, users might expect it to delete all imap emails / messages from that chat too.

I wouldn't sync chat deletion before we sync message deletion.

But these are independent things actually, one doesn't make another worse.

@link2xt
Copy link
Collaborator

link2xt commented Nov 16, 2023

For deletion current plan is not to release it into 1.42 series and stabilize everything else first. Deletion is destructive and therefore dangerous, let's test how well pinning/renaming works first on a large scale.

@iequidoo
Copy link
Collaborator Author

There may be a problem with synchronising chats deletion -- if another device has DeleteServerAfter set, messages wouldn't be deleted from the server as a user might think. But this is the problem also with the current chat deletion functionality probably -- chat messages remain on the server.

Motivation: Now broadcast lists creation is synced across devices. Groups creation, in some means, too -- on a group promotion by a first message. So, to make this looking more feature-complete, sync chats deletion too.

An approach is to replay a user action on synchronised devices because the chat deletion is a complex procedure affecting several tables, self-chat, emitting the corresponding event etc.

Also this commit makes all deletions to happen in a transaction, to be on a safe side.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants