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

Allow deleting a message locally #4192

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

Conversation

marmistrz
Copy link

@marmistrz marmistrz commented Oct 9, 2021

Summary

This is a PoC of a heavily desired feature, e.g. in #4149, #1748, #2267, currently created as a draft to enable early feedback.

Progress

  • Update the message status in the database
  • Display a placeholder instead of a deleted message
  • Show a confirmation dialog before deleting a message (including an information that other participants retain the message)
  • Only show the context menu item if the message has not yet been deleted

@marmistrz
Copy link
Author

marmistrz commented Oct 9, 2021

Currently, I'm using the deleted field of the Message class to mark the message as deleted for the PoC. This is not the best option because the field is also used to mark deleted files (which can possibly be redownloaded). if a message contains a file sent via HTTP Upload, both options should be available: to delete the file downloaded and to delete the message with the link.

I see the following options here:

  • rename the current deleted field to something like fileDeleted / use a new field messageDeleted
  • extend the Status "enum" in Message.java to include STATUS_DELETED. I don't think it's a good idea since it might clash with functionality like XEP-0184 support.

@iNPUTmice what do you think? How would you prefer the deletion marker to be stored?

@mightymop
Copy link

mightymop commented Oct 21, 2021

@iNPUTmice what do you think? How would you prefer the deletion marker to be stored?

Save the retraction message with its information too but hide it in the chatarea. This message has the origin-id of the deleted message.
So if it will be processed then you are able to delete / change the bodytext or whatever of the target message because you have its ID. Advantage is: if you reload from mam it would be deleted again ;)

btw. same for XEP - 308 (Last Message Correction)

@marmistrz
Copy link
Author

@mightymop I'm not implementing XEP-0424 so there is no retraction message.

@mightymop
Copy link

mightymop commented Oct 21, 2021

@mightymop I'm not implementing XEP-0424 so there is no retraction message.

ah ok hm but why not implementing this xep?
i just implemented this xep for jsxc too in the last days :)

@marmistrz
Copy link
Author

ah ok hm but why not implementing this xep?

The usecase is completely different. With local deletion like this I can delete messages sent by remote parties, without affecting their storage. XEP-0424 is for retracting the sender's own messages. XEP-0424 has UX challenges, we probably should still allow the receiver to see the retracted message content; local deletion is much simpler.

@mightymop
Copy link

mightymop commented Oct 21, 2021

I understand...
Then i would keep the "deleted" attribute and add another one and save those messages in a new table. So you could look into that table while processing a message if it was "deleted" in the past if you reload it with mam
and if it will be found then you could do your actions..

@marmistrz
Copy link
Author

marmistrz commented Oct 31, 2021

@iNPUTmice ping, can you share some feedback regarding things I mentioned in this comment: #4192 (comment)

@iNPUTmice
Copy link
Owner

@marmistrz If all goes well (if nothing super urgent security wise comes up) there won’t be any more changes made to Conversations 2.x

@marmistrz
Copy link
Author

So I should wait for the release of Conversations 3.x before working on any new features?

@marmistrz marmistrz mentioned this pull request Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants