Skip to content

Conversation

@AshutoshKhadse23
Copy link

Replace inefficient edit_history scanning with direct last_moved_timestamp lookup to improve performance when determining when items were last moved.

Fixes #1397

Replace inefficient edit_history scanning with direct last_moved_timestamp
lookup to improve performance when determining when items were last moved.

Fixes zulip#1397
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

One comment below, from a quick skim. Also, this PR needs tests.

Comment on lines 1184 to 1194
// Old server: need to scan edit_history to determine if content was edited.
final editHistory = json['edit_history'] as List<dynamic>?;
if (editHistory == null) return lastEditTimestamp;

// Find the timestamp of the first content edit.
for (final entry in editHistory) {
if (entry['prev_content'] != null) {
return entry['timestamp'] as int?;
}
}
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is meant to replace the history-scanning code in MessageEditState._readFromMessage, right? But that code is still present, untouched.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but in the issue I observed other contributor plan was to use editHistory too.
But after you point out this I checked is it neccessary to keep that, but I conclude to that it is not necessary and everything is working correctly on the new server.
@chrisbobbe now could you check ?

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.

Use last_moved_timestamp instead of scanning edit_history

2 participants