Delete button message #189
Conversation
|
hey @rockhouse , thanks for the PR.. But, your code seems outdated.. Your PR is removing some functionality already on master. Can you take a look on this? About your question, this behavior is how our message reactivity works.. Look at this code: https://github.com/RocketChat/Rocket.Chat/blob/master/client%2Flib%2Fcollections.coffee#L14-L16 And, I think you shoud not delete the message record. In discussion with other members of the dev team, we think you should do the follow:
What you think? I'm sorry for not thinking on this before.. I think we should discussed this on an issue before you start coding =( |
|
hi @sampaiodiego, thanks for the quick feedback. PR should be up to date now! Let me know if not. Regarding the behaviour, thanks for the pointer I think I understand the problem now. The msg is actually added back to ChatMessageHistory and that is then displayed at the top of the chat again as that collection is reactive as well and triggers the addition back to the top. I guess the oplog listener to remove should be deleted as deleted messages should not be showing up anyhow, right? Or am I missing something, is the remove used somewhere else? Then I guess it is time to have this discussion now :) It looks a little over engineered to just delete a message don't you think? If we set it to blank than I don't understand what the added value is to keep the message in the DB. But maybe I am missing something. Let's discuss! :D |
|
Hi, I have a mixed feeling about this too. I generally don't like the idea of things disappearing completely, I may be very confusing for users... in so many situations. If you KNOW you read somethings, or received a sound alert for a new message... and it gets deleted.. you will think that there was a bug or something if you can't find ANY indication of what happened. |
|
Maybe the message can be marked as deleted to show something to users and then deleted after a certain time like 24h. |
|
I'd first try just to mark as deleted... and see what people think of it. |
|
+1 for full delete Slack's default behaviour is a full delete without message and I like this behavior because it delivers a cleaner experience. And the history is already "untrusted" - I mean everyone can change all this messages even years ago to whatever he wants. Slack even goes further and if you edit a message to black it suggests a full remove without message. Note: But Slack allows to change the default behaviour. The admin can disable edit's and deletes in the settings for a secure history. |
|
I guess the KEY here is configuration. We must support both behaviour and make it configurable. |
|
we must support two behaviors, but we must choose the default one. how the code are at moment, it may be difficult to simply remove the message, because of how message reactivity and history were implemented. so, maybe updating the message should be easiest way to accomplish the feature until the "blaze refactoring" be done. |
|
Agreed with @sampaiodiego |
|
just for my understanding @sampaiodiego why are those lines needed: As far as I could tell it does not make sense to add a deleted item to the ChatHistory, but maybe I am missing something? If it is not needed there is no code reason why the implementation of the delete functionality could not move forward as proposed and adjusted later? What do you think? |
|
so @rockhouse .. the message subscription (here) only sends 50 messages to client. this means that only the last 50 messages will be "reactive". so when a new message is sent, the last message of this "merge box" will be discarted, but the client still wants to show it.. this is what the code you asked for do. it "catches" the messages discarted by the server and put it in another local collection (ChatMessageHistory), so the view can still show the message, even if it isn't on the merge box anymore.. this means this message isn't reactive too. you get it? this implementation was a try to solve some load issues.. have too many messages on server merge box is really pain, for example, it uses too many memory and cpu.. but this could not be best way to achieve this result.. |
confirmation The trigger for remove is not needed in building the ChatHistory and would otherwise interfer with the delete message function
|
I completely get it :) thanks for the explanation! I adjusted my changes so it can delete messages and does not destroy the reactivity :) I made a little change and update the entry before and remove the rid then we delete. In the reactive code we check for the remove trigger but can filter on rid and leave it out. What do you think? |
|
Due to modifications on message rendering I will reimplement the delete functionality on the new code base and make a new pull request as discussed with @sampaiodiego. |
use electron-builder's install-app-deps, drop native-module stuff
…_width Fix Sidebar Width not proper when last msg is very Long
This implements a delete button next to the edit button. When it's pressed a sweetAlarm (as discussed with @sampaiodiego) modal is presented to acknowledge or cancel. The entry is deleted from both server and client mini mongo but somehow jumps to the top of the chat and a full refresh is necessary to get rid of the message, any ideas why that is?