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

DEVX-5446 Implement Force Mute feature #233

Merged
merged 8 commits into from
Dec 7, 2021

Conversation

superchilled
Copy link
Contributor

@superchilled superchilled commented Oct 29, 2021

Description

Implements the force mute feature, either for an individual stream or for all streams in a session. Two methods have been added, one for each of these actions.

Motivation and Context

It implements a feature in the SDK which has already been added to the API.

How Has This Been Tested?

Example Output or Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@superchilled superchilled changed the title Adding force_mute_stream and force_mute_session methods DEVX-5446 Implement Force Mute feature Oct 29, 2021
@superchilled superchilled marked this pull request as ready for review October 31, 2021 14:15
Copy link
Collaborator

@jeffswartz jeffswartz left a comment

Choose a reason for hiding this comment

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

@superchilled These methods do not relate to OpenTok connections. They relate to sessions and streams. (You mute streams in a session, not connections.) I propose that we should have the following two methods:

  • opentok.sessions.forceMuteAll(session_id, opts)
  • opentok.streams.forceMute(session_id, stream_id,opts)

I'm not a Ruby expert, so please let me know if this makes sense.

@superchilled
Copy link
Contributor Author

@superchilled These methods do not relate to OpenTok connections. They relate to sessions and streams. (You mute streams in a session, not connections.) I propose that we should have the following two methods:

* `opentok.sessions.forceMuteAll(session_id, opts)`

* `opentok.streams.forceMute(session_id, stream_id,opts)`

I'm not a Ruby expert, so please let me know if this makes sense.

@jeffswartz ah, ok. I think I confused connections and streams. In that case, based on the current implementation, I think it would make more sense to define both methods in the Streams class. This would be in line with the fact that this class currently implements Streams#find (which hits the https://api.opentok.com/v2/project/<apiKey>/session/<sessionId>/stream/<streamId> endpoint) and Streams#all (which hits the https://api.opentok.com/v2/project/<apiKey>/session/<sessionId>/stream/ endpoint).

Implementing forceMuteAll in Session would be tricky because the Session object doesn't have direct access to the Client object (all calls to the API endpoints are called via methods in the Client object). The Session object basically just contains information about the current session and is used by other objects in the library when they need to invoke methods on the Clientwhich relate to that session.

Let me know if you're happy for both these methods to be in the Streams class and I'll make the necessary changes.

lib/opentok/opentok.rb Outdated Show resolved Hide resolved
lib/opentok/client.rb Outdated Show resolved Hide resolved
lib/opentok/client.rb Outdated Show resolved Hide resolved
Comment on lines +87 to +104
# Force all streams connected to an OpenTok session to mute themselves.
#
# @param [String] session_id The session ID of the OpenTok session.
# @param [Hash] opts An optional hash defining options for muting action. For example:
# @option opts [true, false] :active Whether streams published after this call, in
# addition to the current streams in the session, should be muted (true) or not (false).
# @option opts [Array] :excluded_streams The stream IDs for streams that should not be muted.
# This is an optional property. If you omit this property, all streams in the session will be muted.
# @example
# {
# "active": true,
# "excluded_streams": [
# "excludedStreamId1",
# "excludedStreamId2"
# ]
# }
#
def force_mute_all(session_id, opts = {})
Copy link
Collaborator

@jeffswartz jeffswartz Dec 6, 2021

Choose a reason for hiding this comment

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

At first, I didn't think the force_mute_all() method should be in the Streams class, since it applies to all streams in a session, not to a specific stream. But i don't know where else to put it. We don't have an OpenTok::Sessions object. So, I think including it in the Streams class is fine. (Sorry for rambling.)

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.

2 participants