-
Notifications
You must be signed in to change notification settings - Fork 743
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
replyTo are not updated if the original message is edited #6404
Conversation
@@ -65,7 +65,7 @@ data class TimelineEvent( | |||
|
|||
init { | |||
if (BuildConfig.DEBUG) { | |||
assert(eventId == root.eventId) | |||
// assert(eventId == root.eventId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add this line @bmarty ? If i keep it, it's not working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.eventId
should always have the same value than this.root.eventId
. In which case this was not true on your side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to use some code from LocalEventFactory to avoid the code duplication. In createEvent
, a new eventId is created for the event.
In my case i don't know if i should use the past id even if i updated the relatedTo part ? or should i create a new id ? 🤔
Knowing i never update the DB, i juste update the object before print it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ganfra told me that i should keep the id.
.../src/main/java/org/matrix/android/sdk/internal/session/room/timeline/LoadTimelineStrategy.kt
Outdated
Show resolved
Hide resolved
.../src/main/java/org/matrix/android/sdk/internal/session/room/timeline/LoadTimelineStrategy.kt
Outdated
Show resolved
Hide resolved
(uiEchoManager?.decorateEventWithReactionUiEcho(it) ?: it) | ||
uiEchoManager?.decorateEventWithReactionUiEcho(it) | ||
|
||
if (it.isReply()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a name to the let scope param instead of using it
} | ||
|
||
fun createEvent(roomId: String, type: String, content: Content?): Event { | ||
fun createEvent(roomId: String, type: String, content: Content?, currentEventId: String? = null): Event { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit hacky to use an eventId on the LocalEchoEventFactory, it should only gives Event with local.id.
I don't think we need a complete new Event but just a new Content. Create a method which returns the Content and uses this in both the Timeline and this Factory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to do that and I failed to copy the Content of the root.
i did in timelinechunk:
val newEvent = it.root.copy(content = newContent)
it.copy(root = newEvent)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how you fail?
Also we have to be careful to copy all fields of Event, so we avoid loosing transient values:
Check here how we do that:
fun updateSentStateWithUiEcho(timelineEvent: TimelineEvent): TimelineEvent {
if (timelineEvent.root.sendState.isSent()) return timelineEvent
val inMemoryState = inMemorySendingStates[timelineEvent.eventId] ?: return timelineEvent
// Timber.v("## ${clock.epochMillis()} Send event refresh echo with live state $inMemoryState from state ${element.root.sendState}")
return timelineEvent.copy(
root = timelineEvent.root.copyAll()
.also { it.sendState = inMemoryState }
)
}
@@ -573,6 +573,51 @@ internal class LocalEchoEventFactory @Inject constructor( | |||
return clock.epochMillis() | |||
} | |||
|
|||
fun createReplyTextContent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use this in createReplyTextEvent
showInThread = false | ||
) | ||
val event = currentTimelineEvent.root | ||
Event( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are missing the transient fields here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If i keep the transient fileds here, my message is not updated: the old version is keep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you are also copying the decrypted response :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes
SonarCloud Quality Gate failed. |
)" This reverts commit 99a906f.
Type of change
Content
Before printing a replied event, update it with the event from the DB.
I will do another PR for the rédaction.
Same solution implemented in iOs.
Motivation and context
Closes #5546
Screenshots / GIFs
repliedto_cXCxsYR6.mp4
Tests
Send a message, replied to the event and edit the first message.
Tested devices
Checklist