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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop recording screencast when user clicks "stop" using browser button #4180

Merged
merged 1 commit into from Dec 20, 2021

Conversation

tvst
Copy link
Contributor

@tvst tvst commented Dec 15, 2021

馃摎 Context

Today there's the "Record a screencast" feature has a bug where it becomes impossible to stop recording when you do the following:

  1. Start recording a screencast for the current tab
  2. Click "stop sharing" in the banner that Chrome shows at the top of the tab's contents
  3. Try to stop recording.

This PR fixes that.

  • What kind of change does this PR introduce?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe:

馃 Description of Changes

  • Add onstop and onerror handlers to the MediaRecorder object.

  • Upon an error/stop even, tell the UI's state machine that we're no longer recording.

    • This is a breaking API change
    • This is a visible (user-facing) change

馃И Testing Done

  • Screenshots included n/a
  • Added/Updated unit tests This code is not currently tested or easily testable. Just landing a quick fix. Hopefully this is OK?
  • Added/Updated e2e tests n/a

馃寪 References

Does this depend on other work, documents, or tickets?


Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@tvst
Copy link
Contributor Author

tvst commented Dec 15, 2021

Controversial part: no tests. But the code had no tests before, is hard to test without testing the exact implementation, and is not in the app's critical path. I leave it up to you whether this is mergeable 馃槅

@tvst tvst requested a review from kmcgrady December 15, 2021 03:55
@kajarenc
Copy link
Collaborator

LGTM 馃憤

@kajarenc
Copy link
Collaborator

@tvst We reviewed and tested this PR in a mob programming session with the almost whole team, and the general opinion is that it is quite difficult to test the behavior of screencast recording, which is also a good argument in favor of the decision to remove this feature. (it seems that Cypress has no available primitives for this).

So this is not only my approval but also the consolidated opinion of the team, feel free to merge!

@tvst tvst merged commit 497e3fc into streamlit:develop Dec 20, 2021
@tvst tvst deleted the fix-screencast branch December 20, 2021 20:06
tconkling added a commit to tconkling/streamlit that referenced this pull request Jan 3, 2022
* develop:
  Fix hello demo type annotation (streamlit#4228)
  Release 1.3.1 (streamlit#4220)
  Improve beta_ deprecation message (streamlit#4219)
  Changing Image Algorithm to Bilinear (streamlit#4159)
  Allow columns to be rendered to create spacing (streamlit#4217)
  Fix issue with hidden balloons (approach 2) (streamlit#4204)
  Fix mypy errors from 0.930 release (streamlit#4218)
  Fix/selectbox typings (streamlit#4194)
  Support running tests locally on Apple Silicon (streamlit#4185)
  Stop screencast recorder when user removes permission / stops using browser button. (streamlit#4180)
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.

Recording does not Stop
2 participants