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.

Sign up for GitHub

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

Jump to bottom

Support redactions and relations of/with unsent events. #947

Merged
merged 21 commits into from
Jun 13, 2019
Merged

Support redactions and relations of/with unsent events. #947

merged 21 commits into from
Jun 13, 2019

Conversation

bwindels
Copy link
Contributor

@bwindels bwindels commented Jun 7, 2019 •
edited
Loading

PR does several things to make this work:

  • fix scheduler assigning queue for redactions and relations
  • ensure queued relations and redactions to an unsent target have their local id replaced with a remote id before being sent:
    • have one method to return the related id for redactions and relations (getRelatedId) on Event
    • emit event on MatrixEvent once remote id is known
    • make the scheduler start sending a new event asynchronously, so the above code gets a chance to run before the next event is sent. (not entirely happy with this, we're relying on async implementation details that make this work, suggestions welcome)
  • preserve the locally redacted state when receiving the remote echo
  • don't re-add a locally redacted reaction when receiving remote echo

Hammering the react button now works as expected with local echo:
hammertime2

Part of: element-hq/element-web#9860
Fixes: element-hq/element-web#10034

@bwindels bwindels changed the title Support redactions and relations of/with unsent events. #946 Support redactions and relations of/with unsent events. Jun 7, 2019
@bwindels bwindels force-pushed the bwindels/relations-unsent branch 6 times, most recently from 5210826 to 10308a9 Compare June 11, 2019 09:15
@bwindels bwindels mentioned this pull request Jun 11, 2019
Closed
@bwindels bwindels marked this pull request as ready for review June 12, 2019 08:14
@bwindels bwindels requested a review from a team June 12, 2019 08:14
@jryans jryans requested review from jryans and removed request for a team June 12, 2019 09:49
jryans
jryans requested changes Jun 12, 2019
View reviewed changes
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I am particularly curious about the changes to redactions on remote echo, so I'd like to have another review for that bit.

Please note I am on holiday Thursday - Friday, so you may need a different reviewer if I am away when the next round appears.

spec/unit/scheduler.spec.js Outdated Show resolved Hide resolved
spec/unit/scheduler.spec.js Outdated Show resolved Hide resolved
src/client.js Show resolved Hide resolved
src/client.js Show resolved Hide resolved
spec/unit/scheduler.spec.js Outdated Show resolved Hide resolved
src/models/event.js Outdated Show resolved Hide resolved
src/models/event.js Outdated Show resolved Hide resolved
src/models/event.js Outdated Show resolved Hide resolved
src/models/room.js Outdated Show resolved Hide resolved
src/scheduler.js Show resolved Hide resolved
@bwindels bwindels mentioned this pull request Jun 12, 2019
Merged
bwindels and others added 16 commits June 13, 2019 12:26
@bwindels
enqueue relations and redactions as well
e222fb1 
as they might need to wait until their target has been sent
@bwindels
give the client a chance to run room.updatePendingEvent after sending
c58db66 
before the next event is sent. This is needed to update the target id
if it was the local id of the event that was just sent.
@bwindels
first look in pending event list for event being redacted
6eb229a 
in case the redacted event hasn't been sent yet
@bwindels
emit remote id once received so enqueued relations have it when sent
831aec6
@bwindels
emit Relations.redaction synchronously, timeout should not be needed
7a10d50 
listeners shouldn't care about the original event, as it's removed
from the Relations collection already.
@bwindels
rename target id to related id and add jsdoc comments
f1336a5
@bwindels
fix lint
3f917b3
@bwindels
fix tests
7d2f7fa
@bwindels
get the txnId from the correct place to delete event after remote echo
624c6f0
@bwindels
preserve (locally) redacted state after applying remote echo
6d9fba8 
because the RedactionDimensions was trying to redact an event
that was already redacted after it's remote echo had come in
but it's redaction hadn't synced yet.
@bwindels
don't add events from /sync that have been locally redacted
930de64 
it'll cause the reactions counter to go up and down while reactions
and redactions come in.

In case the local redaction gets cancelled,
Room._revertRedactionLocalEcho will add the relation back to
the relations collection.
@bwindels
make sure where not re-adding cancelled events when undoing local red.
5602b94
@bwindels @jryans
comment typo
a9f9e2c 
Co-Authored-By: J. Ryan Stinnett <[email protected]>
@bwindels @jryans
comment typo
b005b75 
Co-Authored-By: J. Ryan Stinnett <[email protected]>
@bwindels
clarify why we need to listen for remote echo of related event
3ed9b00
@bwindels
rename related id to associated id
4143a79
@bwindels bwindels requested a review from a team June 13, 2019 10:39
@bwindels
Copy link
Contributor Author

Thanks for working on this! I am particularly curious about the changes to redactions on remote echo, so I'd like to have another review for that bit.

Please note I am on holiday Thursday - Friday, so you may need a different reviewer if I am away when the next round appears.

Could someone have a second look as Ryan suggested? Thanks!

@dbkr dbkr requested review from dbkr and removed request for a team June 13, 2019 10:43
dbkr
dbkr approved these changes Jun 13, 2019
View reviewed changes
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

I think this makes sense!

spec/unit/scheduler.spec.js Show resolved Hide resolved
@bwindels bwindels merged commit 26aab4f into develop Jun 13, 2019
@t3chguy t3chguy deleted the bwindels/relations-unsent branch May 10, 2022 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jryans jryans jryans requested changes

@dbkr dbkr dbkr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

local echo of redactions doesn't seem to honour rate limiting properly
3 participants
@bwindels @jryans @dbkr