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

Acknowledge DeviceMute widget actions #12790

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Jul 18, 2024

This prevents sending the default error response to the widget.
required for: element-hq/element-call#2482

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@toger5 toger5 requested a review from a team as a code owner July 18, 2024 12:13
@toger5 toger5 requested review from dbkr and robintown July 18, 2024 12:13
@toger5 toger5 changed the title acknowledge DeviceMute widget actions Acknowledge DeviceMute widget actions Jul 18, 2024
@@ -21,8 +21,6 @@ export enum ElementWidgetActions {
JoinCall = "io.element.join",
HangupCall = "im.vector.hangup",
CallParticipants = "io.element.participants",
MuteAudio = "io.element.mute_audio",
UnmuteAudio = "io.element.unmute_audio",
Copy link
Member

Choose a reason for hiding this comment

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

These were just unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I could not find where they were used in EC, the react sdk and the widget api.

Copy link
Member

Choose a reason for hiding this comment

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

They were used by Jitsi.

https://github.com/element-hq/element-web/blob/develop/src/vector/jitsi/index.ts#L197-L206

If they were stale then they should have been removed on that side first

Copy link
Member

Choose a reason for hiding this comment

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

Looks like 1f64835 stopped the use of those enum values

Copy link
Member

Choose a reason for hiding this comment

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

@robintown as the author of that commit do you have any additional context?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it powered a set of quick controls in the left panel which were removed

image

Copy link
Member

Choose a reason for hiding this comment

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

src/stores/widgets/ElementWidgetActions.ts Outdated Show resolved Hide resolved
src/stores/widgets/ElementWidgetActions.ts Show resolved Hide resolved
@toger5 toger5 added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Jul 19, 2024
@toger5
Copy link
Contributor Author

toger5 commented Jul 19, 2024

Can we allow this change without test coverage? I don't want to write a test that emits a signal and checks if reply is called? There might be a tiny bit of value in this test in terms of regression prevention but if someone removes this code it most likely is for a reason?

Am I overseeing sth. What are other takes on this?

@t3chguy
Copy link
Member

t3chguy commented Jul 19, 2024

The policy is code either needs Jest coverage or a playwright test up to an 80% mark to match customer contracts. Pick your poison unfortunately

@toger5
Copy link
Contributor Author

toger5 commented Jul 19, 2024

The policy is code either needs Jest coverage or a playwright test up to an 80% mark to match customer contracts. Pick your poison unfortunately

Okay that obviously rules out the "being less strict" approach.
I will write a unit test for it.

@toger5 toger5 force-pushed the toger5/ack-device-mute-widget-action branch from d5256f7 to 14de45f Compare July 25, 2024 09:11
@toger5 toger5 added this pull request to the merge queue Jul 25, 2024
Merged via the queue into develop with commit 72e7df0 Jul 25, 2024
45 of 49 checks passed
@toger5 toger5 deleted the toger5/ack-device-mute-widget-action branch July 25, 2024 09:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants