Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Fix calls incorrectly showing as "Connecting" #10204

Closed
wants to merge 4 commits into from

Conversation

artcodespace
Copy link
Contributor

@artcodespace artcodespace commented Feb 21, 2023

Fixes PSC-204

The issue as I saw it lay in the setState inside LegacyCallEventGrouper. The setState method is used in two places, it's called directly in setCall and also as a listener, attached to this.call.

The issue was that normally, when closing down a call, the this.call listener would fire last, (after all setCall calls). When this happens, we get the incorrect state displayed. Sometimes, the listener would fire in amongst all of the (about 20) calls to setState from setCall when trying to close a call. This would result in the expected behaviour.

I've tried to make the fix as small as possible by allowing the this.call listener to directly set the state of the LegacyCallEventGrouper given that this seemed like the source of truth to me. Applying this change made the text behave as expected in testing (video attached).

Nb left hand side of the screen is nightly (ie demonstrates the problem), right hand side is element web, with the changes applied
Video 1 - comparing missed calls
https://user-images.githubusercontent.com/56027671/220369272-1e5d2d26-a383-4b9e-9a76-ab0d91830d72.mov

Video 2 - comparing successful calls
https://user-images.githubusercontent.com/56027671/220369671-3fc5c336-abdb-42c5-8cb2-6256b1da3952.mov

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Fix calls incorrectly showing as "Connecting" (#10204). Contributed by @alunturner.

@artcodespace artcodespace added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Feb 21, 2023
@artcodespace
Copy link
Contributor Author

Have opened this up whilst I try to figure out how to add a sensible test for this, I'm not sure how easy this will be though

Copy link
Contributor

@justjanne justjanne left a comment

Choose a reason for hiding this comment

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

The code generally looks okay, but I'd really like to see a test. Especially for ad-hoc state machines like these, tests would be appreciated.

@artcodespace
Copy link
Contributor Author

artcodespace commented Feb 23, 2023

Good news @justjanne - this has been overtaken by #10223 so I'm closing this PR as it's not required

@artcodespace artcodespace deleted the alunturner/psc-204 branch February 23, 2023 10:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants