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.

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

Supporting fixes For making UnknownDeviceDialog not pop up automatically #575

Merged
merged 7 commits into from
Dec 8, 2017

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Nov 16, 2017

This is mostly a lot of VoIP fixes to make the flow of trying to place or answer a call when there are unknown devices in the room. We don't want to stack up VoIP events in the pending events and then fire them all out later: we should just abort the call and place a new one once the user has reviewed the new devices.

Reviewing this commit-by-commit may make more sense: the commits are fairly well contained, functionality-wise, but all in the same commit because they'd otherwise leave things a bit broken.

Reverts #378

This swallowed all errors from sendEvent, breaking the ICE candidate
retrying. react-sdk no longer listens for send_event_error so I
think it's best to just remove this.
 * If we can't send an invite due to unknown devices, abort the
   call.
 * Don't transition to the `invite_sent` state until the invite
   has actually sent.
 * Add a specific error code for failure due to unknown devices.
 * Don't send ICE candidate messages if the call has ended.
 * Add an `event` property to errors from `sendEvent` so that the
   caller can resend or cancel the event.
We won't have sent the invite anyway. Also termainate before we
fire the error event so the call is 'ended' when the event handlers
fire (which means if they try to hang up it's also ignored)
 * Store the answer we generate so if we fail to send it, we can
   try to send it again (doing the same again doesn't work as
   webrtc is in the wrong state).
 * Don't send ICE candidates if the call is ringing: queue them up
   so we can send them later if we manage to actually send the
   answer.
// We've failed to answer: back to the ringing state
setState(this, 'ringing');
this.client.cancelPendingEvent(error.event);
this.emit(
Copy link
Member

Choose a reason for hiding this comment

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

surely we should only emit this if the error is UnkDevices?

Copy link
Member

Choose a reason for hiding this comment

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

(it's a bit unintuitive also that the fact there are unknown devices in the room gets in the way of call signalling, which the user might reasonably expect to be 1:1. it's almost an argument that we should be using toDevice messaging for the 1:1 bits of VoIP Signalling...)

Copy link
Member Author

Choose a reason for hiding this comment

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

oops, thanks!

Copy link
Member Author

@dbkr dbkr Nov 17, 2017

Choose a reason for hiding this comment

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

and yeah, although in the case of an invite you'll still want to send it to all devices. For answers it is a bit silly since it only needs to go to the device the invite came from (although the other devices need to know the call has been picked up so they stop ringing).

}).catch((error) => {
self.client.cancelPendingEvent(error.event);
terminate(self, "local", "unknown_devices", false);
self.emit(
Copy link
Member

Choose a reason for hiding this comment

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

again, surely we should check the type of error before assuming it's an Unknown Devices?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed here too

@ara4n
Copy link
Member

ara4n commented Nov 17, 2017

looks plausible mod comments

@lukebarnard1 lukebarnard1 removed their assignment Nov 17, 2017
Send a sensible error message for other errors.
@dbkr dbkr merged commit d26b443 into develop Dec 8, 2017
krombel added a commit to krombel/matrix-js-sdk that referenced this pull request Mar 21, 2018
[Full Changelog](matrix-org/matrix-js-sdk@v0.9.2...v0.10.0-rc.1)
* Fix duplicated state events in timeline from peek
[\matrix-org#630](matrix-org#630)
* Create indexeddb worker when starting the store
[\matrix-org#627](matrix-org#627)
* Fix indexeddb logging
[\matrix-org#626](matrix-org#626)
* Don't do /keys/changes on incremental sync
[\matrix-org#625](matrix-org#625)
* Don't mark devicelist dirty unnecessarily
[\matrix-org#623](matrix-org#623)
* Cache the joined member count for a room state
[\matrix-org#619](matrix-org#619)
* Fix JS doc
[\matrix-org#618](matrix-org#618)
* Precompute push actions for state events
[\matrix-org#617](matrix-org#617)
* Fix bug where global "Never send to unverified..." is ignored
[\matrix-org#616](matrix-org#616)
* Intern legacy top-level 'membership' field
[\matrix-org#615](matrix-org#615)
* Don't synthesize RR for m.room.redaction as causes the RR to go missing.
[\matrix-org#598](matrix-org#598)
* Make Events create Dates on demand
[\matrix-org#613](matrix-org#613)
* Stop cloning events when adding to state
[\matrix-org#612](matrix-org#612)
* De-dup code: use the initialiseState function
[\matrix-org#611](matrix-org#611)
* Create sentinel members on-demand
[\matrix-org#610](matrix-org#610)
* Some more doc on how sentinels work
[\matrix-org#609](matrix-org#609)
* Migrate room encryption store to crypto store
[\matrix-org#597](matrix-org#597)
* add parameter to getIdentityServerUrl to strip the protocol for invites
[\matrix-org#600](matrix-org#600)
* Move Device Tracking Data to Crypto Store
[\matrix-org#594](matrix-org#594)
* Optimise pushprocessor
[\matrix-org#591](matrix-org#591)
* Set event error before emitting
[\matrix-org#592](matrix-org#592)
* Add event type for stickers [WIP]
[\matrix-org#590](matrix-org#590)
* Migrate inbound sessions to cryptostore
[\matrix-org#587](matrix-org#587)
* Disambiguate names if they contain an mxid
[\matrix-org#588](matrix-org#588)
* Check for sessions in indexeddb before migrating
[\matrix-org#585](matrix-org#585)
* Emit an event for crypto store migration
[\matrix-org#586](matrix-org#586)
* Supporting fixes For making UnknownDeviceDialog not pop up automatically
[\matrix-org#575](matrix-org#575)
* Move sessions to the crypto store
[\matrix-org#584](matrix-org#584)
* Change crypto store transaction API
[\matrix-org#582](matrix-org#582)
* Add some missed copyright notices
[\matrix-org#581](matrix-org#581)
* Move Olm account to IndexedDB
[\matrix-org#579](matrix-org#579)
* Fix logging of DecryptionErrors to be more useful
[\matrix-org#580](matrix-org#580)
* [BREAKING] Change the behaviour of the unverfied devices blacklist flag
[\matrix-org#568](matrix-org#568)
* Support set_presence=offline for syncing
[\matrix-org#557](matrix-org#557)
* Consider cases where the sender may not redact their own event
[\matrix-org#556](matrix-org#556)
krombel added a commit to krombel/matrix-js-sdk that referenced this pull request Apr 18, 2018
[Full Changelog](matrix-org/matrix-js-sdk@v0.9.2...v0.10.0-rc.1)
* Fix duplicated state events in timeline from peek
[\matrix-org#630](matrix-org#630)
* Create indexeddb worker when starting the store
[\matrix-org#627](matrix-org#627)
* Fix indexeddb logging
[\matrix-org#626](matrix-org#626)
* Don't do /keys/changes on incremental sync
[\matrix-org#625](matrix-org#625)
* Don't mark devicelist dirty unnecessarily
[\matrix-org#623](matrix-org#623)
* Cache the joined member count for a room state
[\matrix-org#619](matrix-org#619)
* Fix JS doc
[\matrix-org#618](matrix-org#618)
* Precompute push actions for state events
[\matrix-org#617](matrix-org#617)
* Fix bug where global "Never send to unverified..." is ignored
[\matrix-org#616](matrix-org#616)
* Intern legacy top-level 'membership' field
[\matrix-org#615](matrix-org#615)
* Don't synthesize RR for m.room.redaction as causes the RR to go missing.
[\matrix-org#598](matrix-org#598)
* Make Events create Dates on demand
[\matrix-org#613](matrix-org#613)
* Stop cloning events when adding to state
[\matrix-org#612](matrix-org#612)
* De-dup code: use the initialiseState function
[\matrix-org#611](matrix-org#611)
* Create sentinel members on-demand
[\matrix-org#610](matrix-org#610)
* Some more doc on how sentinels work
[\matrix-org#609](matrix-org#609)
* Migrate room encryption store to crypto store
[\matrix-org#597](matrix-org#597)
* add parameter to getIdentityServerUrl to strip the protocol for invites
[\matrix-org#600](matrix-org#600)
* Move Device Tracking Data to Crypto Store
[\matrix-org#594](matrix-org#594)
* Optimise pushprocessor
[\matrix-org#591](matrix-org#591)
* Set event error before emitting
[\matrix-org#592](matrix-org#592)
* Add event type for stickers [WIP]
[\matrix-org#590](matrix-org#590)
* Migrate inbound sessions to cryptostore
[\matrix-org#587](matrix-org#587)
* Disambiguate names if they contain an mxid
[\matrix-org#588](matrix-org#588)
* Check for sessions in indexeddb before migrating
[\matrix-org#585](matrix-org#585)
* Emit an event for crypto store migration
[\matrix-org#586](matrix-org#586)
* Supporting fixes For making UnknownDeviceDialog not pop up automatically
[\matrix-org#575](matrix-org#575)
* Move sessions to the crypto store
[\matrix-org#584](matrix-org#584)
* Change crypto store transaction API
[\matrix-org#582](matrix-org#582)
* Add some missed copyright notices
[\matrix-org#581](matrix-org#581)
* Move Olm account to IndexedDB
[\matrix-org#579](matrix-org#579)
* Fix logging of DecryptionErrors to be more useful
[\matrix-org#580](matrix-org#580)
* [BREAKING] Change the behaviour of the unverfied devices blacklist flag
[\matrix-org#568](matrix-org#568)
* Support set_presence=offline for syncing
[\matrix-org#557](matrix-org#557)
* Consider cases where the sender may not redact their own event
[\matrix-org#556](matrix-org#556)
@t3chguy t3chguy deleted the dbkr/udd_no_auto_show branch May 10, 2022 14:17
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.

3 participants