Page MenuHomePhabricator
Differential D6965

[lib][native] Displaying edited messages from database
ClosedPublic
Actions

Authored by • kuba on Mar 7 2023, 1:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 28, 6:12 AM
Unknown Object (File)
Mon, Oct 28, 6:12 AM
Unknown Object (File)
Mon, Oct 28, 6:12 AM
Unknown Object (File)
Mon, Oct 28, 6:12 AM
Unknown Object (File)
Mon, Oct 28, 6:12 AM
Unknown Object (File)
Mon, Oct 28, 6:12 AM
Unknown Object (File)
Mon, Oct 28, 6:04 AM
Unknown Object (File)
Oct 21 2024, 3:57 PM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
michal
inka
ginsu
Commits
rCOMM0234a2396b70: [lib][native] Displaying edited messages from database
Summary

Now edited messages are visible in the chats. Currently, it's only possible to edit the message (send a new message with edit_message type) from the database. Editing message
which is source of sidebar currently isn't visible in the sidebar view.

Test Plan
  • added edited message to the database, checked if it shows correctly,
  • edited several times the same message, checked if the latest one is shown,
  • edited a few messages in one thread, checked if the messages are updated correctly,
  • edited a few messages in a few threads, checked if the messages are correctly updated

Screenshot 2023-03-07 at 11.01.26.png (1×2 px, 782 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• kuba created this revision.Mar 7 2023, 1:44 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMar 7 2023, 1:44 AM
• kuba added a parent revision: D6964: [native] Added new editing messages type to MessageSpecs in C++.Mar 7 2023, 1:44 AM
• kuba added a child revision: D6966: [lib] Add 'hasBeenEdited' status to messages.Mar 7 2023, 1:48 AM
Harbormaster completed remote builds in B17138: Diff 23478.Mar 7 2023, 1:59 AM
• kuba requested review of this revision.Mar 7 2023, 1:59 AM
• kuba edited the test plan for this revision. (Show Details)Mar 7 2023, 2:01 AM
ashoat added inline comments.Mar 7 2023, 6:07 AM
lib/selectors/chat-selectors.js
403 ↗(On Diff #23478)

See comment in D6966 about read-only types

lib/types/messages/text.js
31 ↗(On Diff #23478)

Make this read-only

• kuba added a reviewer: ginsu.Mar 7 2023, 8:11 AM
• kuba updated this revision to Diff 23528.Mar 8 2023, 2:59 AM
• kuba marked 2 inline comments as done.

Responded to comments, made read-only 'text' property

Harbormaster completed remote builds in B17180: Diff 23528.Mar 8 2023, 3:15 AM
michal accepted this revision.Mar 13 2023, 5:59 AM

Looks good to me, but I haven't worked with this code so it would be nice if someone else could also take a look.

This revision is now accepted and ready to land.Mar 13 2023, 5:59 AM
ashoat added 1 blocking reviewer(s): ginsu.Mar 13 2023, 9:47 AM
This revision now requires review to proceed.Mar 13 2023, 9:47 AM
ginsu accepted this revision.Mar 16 2023, 5:33 PM

accepting with one question inline

lib/selectors/chat-selectors.js
405 ↗(On Diff #23528)

Could we simply this condition to this? Or are we concerned about newText possibly being an empty string?

This revision is now accepted and ready to land.Mar 16 2023, 5:33 PM
• kuba updated this revision to Diff 23981.Mar 22 2023, 5:13 AM
• kuba marked an inline comment as done.

Simplified condition

• kuba retitled this revision from [lib][native] Displaying edited messages from database to [lib] Displaying edited messages from database.Mar 22 2023, 5:23 AM
• kuba updated this revision to Diff 23982.Mar 22 2023, 5:23 AM
• kuba marked an inline comment as not done.
• kuba retitled this revision from [lib] Displaying edited messages from database to [lib][native] Displaying edited messages from database.

Revert the changes

lib/selectors/chat-selectors.js
405 ↗(On Diff #23528)

I changed it, but then after more thought, I believe, that it should stay like this. If the newText is empty we would prefer to display an even empty message, than the original content (that is what the user would probably want). I revert the changes.

We can consider preventing users from sending empty edit messages.

But in the future, it would be easy to add deleting messages by sending edit messages with empty text. We would just not display them at all.

Harbormaster completed remote builds in B17513: Diff 23981.Mar 22 2023, 5:29 AM
Harbormaster completed remote builds in B17514: Diff 23982.Mar 22 2023, 5:38 AM
ashoat added a comment.Mar 22 2023, 2:46 PM

We can consider preventing users from sending empty edit messages.

We should do this. We don't allow empty messages for messageTypes.TEXT. Can you create a follow-up Linear task for this before landing?

• kuba added a comment.Mar 24 2023, 2:57 AM
In D6965#212104, @ashoat wrote:

We can consider preventing users from sending empty edit messages.

We should do this. We don't allow empty messages for messageTypes.TEXT. Can you create a follow-up Linear task for this before landing?

Done: https://linear.app/comm/issue/ENG-3395/prevent-empty-messages-in-message-editing

Closed by commit rCOMM0234a2396b70: [lib][native] Displaying edited messages from database (authored by kosmydel <kukubaczek@gmail.com>). · Explain WhyMar 30 2023, 7:36 AM
This revision was automatically updated to reflect the committed changes.
kosmydel <kukubaczek@gmail.com> added a commit: rCOMM0234a2396b70: [lib][native] Displaying edited messages from database.

Revision Contents
Changeset List

PathSize
lib/
selectors/
21 lines

Diff 24404

View Options

lib/selectors/chat-selectors.js

Loading...