Improvements to event listeners by pmogollons · Pull Request #157 · mieszko4/react-native-zoom-us

pmogollons · 2021-10-08T20:27:07Z

Moved listeners to a separate file
Changed listener event names
Send what the event receives to RN
Implemented new listeners

This PR is just a draft to start working on the new events, there are still some questions and things to be discussed like:

Should events just send userId and we get the actual user in RN or should this events send the userId, user, and some other params like isSelf and other user info?
I have seen that in some cases the custom UI doesn't update when a user is unmuted, it keeps showing the muted icon beside the name, should we update the view on other listeners?
Is there really a need for multiple event "namespaces" AuthEvent and MeetingEvent, should it be better to just have one event that is less prone to collisions with other packages like ZoomUsEvent?
Should we send VideoStatus, AudioStatus, RecordingStatus, SharingStatus as int's or should we send it as Strings?
Should we send sourceType as int or as string?

* Moved listeners to a separate file * Changed listener event names * Send what the event receives to RN * Implemented new listeners Removed comment

mieszko4

I got few notes inline
Also some general problems:

it is very hard to track the changes since the entire pr is in one commit - use multiple commit, especially a single commit should be just for moving out events into the new file.
all of these changes are breaking changes and I still think we should first agree on the event names and then do the new implementation, especially:

what event name pattern we will apply
if we will always use minimum event data with an idea of fetching the details by the user with helper functions (e.g. just send id of user and use getUserInfo in js callback)
what names we will use for the even data (userId is not clear what it means)

In the next comment I will write about my proposal of how to push this forward.

mieszko4 · 2021-10-12T16:16:18Z

android/src/main/java/ch/milosz/reactnative/RNZoomUsModule.java

+          // TODO: We should return the promise resolve
        } else {
-          sendEvent("MeetingEvent", "screenShareError", result);
+          // TODO: We should return the promise rejection


mieszko4 · 2021-10-12T16:26:38Z

android/src/main/java/ch/milosz/reactnative/RNZoomUsInMeetingServiceListener.java

+   */
+  @Override
+  public void onHostAskUnMute(long userId) {
+    sendEvent("audio.host.unmute", userId);


Some suggestions since we do it from scratch:

Suggested change

sendEvent("audio.host.unmute", userId);

// 1. always put it in object

WritableMap data = Arguments.createMap();

// 2. this allows to name the params intuitively

data.putDouble("requestorId", userId);

// or

data.putDouble("requesteeId", userId);

// (userId does not tell us if it is a requestor or requestee)

// 3. name event more specefically (unmute could mean that host unmuted itself

sendEvent("audio.host.unmuteRequested", data);

mieszko4 · 2021-10-12T16:28:58Z

android/src/main/java/ch/milosz/reactnative/RNZoomUsInMeetingServiceListener.java

+    WritableMap data = Arguments.createMap();
+    data.putInt("type", type);
+
+    sendEvent("audio.self.type.changed", data);


I like how you use host and self as the origin of the event triggering

Also I like the grouping, i.e. the first part of the event name.
I see the following event name pattern here:

[eventGroup].[originOfEventTrigger].[eventName]

Where:

eventGroup = audio, video, chat, ...

originOfEventTrigger = self, host, any, ...

eventName = noun with verb, e.g. typeChanged, unmuteRequested (if we agree that "unmute" is a noun)

We should always stick to the same agreed event name pattern (not necessarily this one)

🤔 I am just still not sure if it makes sense to name the events with such pattern...

mieszko4 · 2021-10-12T16:32:37Z

android/src/main/java/ch/milosz/reactnative/RNZoomUsInMeetingServiceListener.java

+
+  @Override
+  public void onUserAudioStatusChanged(long userId, AudioStatus audioStatus) {
+    sendEvent("audio.status.changed", userId, audioStatus);


here status in the name is not the origin - so it is confusing.
I would do audio.self.statusChanged if we follow [eventGroup].[originOfEventTrigger].[eventName]

audio.status.changed follows another event name pattern of [eventGroup].[subject].[pastVerb].
The question is is it feasable to apply such pattern to all the events?

mieszko4 · 2021-10-12T16:45:50Z

android/src/main/java/ch/milosz/reactnative/RNZoomUsInMeetingServiceListener.java

+
+  @Override
+  public void onMicrophoneStatusError(InMeetingAudioController.MobileRTCMicrophoneError error) {
+    sendEvent("audio.status.error", error);


🤔 not sure how to fit "error" into the event name pattern.
There is a verb "to err" but that would look weird, i.e. "audio.status.erred" or "audio.self.statusErred"

mieszko4 · 2021-10-12T16:50:41Z

android/src/main/java/ch/milosz/reactnative/RNZoomUsInMeetingServiceListener.java

+  @Override
+  public void onMyAudioSourceTypeChanged(int type) {
+    WritableMap data = Arguments.createMap();
+    data.putInt("type", type);


I agree to remove final InMeetingUserInfo userInfo, i.e. to keep the minimum number of params and then to expose helper methods (here we should expose getMyUserInfo)

I think int that are used and are enum-like should be converted to enums, so it is easy to write code with typescript. type is an example here.

mieszko4 · 2021-10-12T16:57:31Z

android/src/main/java/ch/milosz/reactnative/RNZoomUsModule.java

-  public void onShareActiveUser(long userId) {
-    final InMeetingService inMeetingService = ZoomSDK.getInstance().getInMeetingService();
-
-    updateVideoView();
-
-    if (inMeetingService.isMyself(userId)) {
-      sendEvent("MeetingEvent", "screenShareStarted");
-
-      final InMeetingShareController shareController = inMeetingService.getInMeetingShareController();
-
-      if (shareController.isSharingOut()) {
-        if (shareController.isSharingScreen()) {
-            shareController.startShareScreenContent();
-        }
-      }
-    } else if (userId == 0) {
-      sendEvent("MeetingEvent", "screenShareStopped");
-    } else {
-      sendEvent("MeetingEvent", "screenShareStartedByUser", userId);
-    }
-  }


I see that onShareActiveUser was removed but not copied. Why?

mieszko4 · 2021-10-12T17:09:14Z

android/src/main/java/ch/milosz/reactnative/RNZoomUsInMeetingServiceListener.java

+  private void sendEvent(String event, long userId, AudioStatus audioStatus) {
+    WritableMap data = Arguments.createMap();
+    data.putDouble("userId", userId);
+    data.putInt("audioStatus", AudioStatus.valueOf(audioStatus));
+
+    sendEvent(event, data);
+  }
+
+  private void sendEvent(String event, long userId, VideoStatus videoStatus) {
+    WritableMap data = Arguments.createMap();
+    data.putDouble("userId", userId);
+    data.putInt("videoStatus", VideoStatus.valueOf(videoStatus));
+
+    sendEvent(event, data);
+  }
+
+  private void sendEvent(String event, long userId, SharingStatus sharingStatus) {
+    WritableMap data = Arguments.createMap();
+    // https://marketplacefront.zoom.us/sdk/meeting/android/us/zoom/sdk/SharingStatus.html
+    data.putInt("sharingStatus", SharingStatus.valueOf(sharingStatus));
+
+    sendEvent(event, data);
+  }


🤔 not sure if we need this. If we can have helper functions to access this info from code I think that userId should be enough... That would simplify the events

mieszko4 · 2021-10-12T17:46:21Z

Moved listeners to a separate file

Changed listener event names

Send what the event receives to RN

Implemented new listeners

This PR is just a draft to start working on the new events, there are still some questions and things to be discussed like:

Should events just send userId and we get the actual user in RN or should this events send the userId, user, and some other params like isSelf and other user info?

I am for sending just userId and getting the actual user info in RN. I am just wondering if that will be possible for non-self user.

I have seen that in some cases the custom UI doesn't update when a user is unmuted, it keeps showing the muted icon beside the name, should we update the view on other listeners?

Not sure, but it should probably not be the focus of this refactoring

Is there really a need for multiple event "namespaces" AuthEvent and MeetingEvent, should it be better to just have one event that is less prone to collisions with other packages like ZoomUsEvent?

🤔 I agree - one event name should be enough. But there cannot be event name collisions with other libs, no? Each lib has its own app id. So event like video.self.statusChange is fine, i.e. no need for any prefixing if that's what you meant

Should we send VideoStatus, AudioStatus, RecordingStatus, SharingStatus as int's or should we send it as Strings?

I think it does not matter as long as from typescript we can access them with enums.
If there is inconsistency between ios and android then we could have a mapping, e.g. in index.ts (i.e. no need to have the mapping in native java/objective-c) so that when using the lib a programmer can use the same Enum.

Should we send sourceType as int or as string?

The same as above - it should be an enum in the end.

Since there are a lot of moving parts here, let's try keep it minimalistic/simple as much as possible - my proposal how to push things forwards:

Make a new pr where the only thing would be moving the events into a separate listener file (non-breaking, easy to track changes).
Create another listener file where we would add the new events that will not be documented so they can be easily changed until we are happy with the naming
Document the new listener file and deprecate the old file
After some time remove the deprecated events from the doc and the deprecated old file, too - release with major version

As for choosing the naming I would first start with finding a consistent event name pattern that would fit to all the events. I like the idea of grouping (video, audio, chat...) and I strive for the consistency

Improvements to event listeners

efa53e4

* Moved listeners to a separate file * Changed listener event names * Send what the event receives to RN * Implemented new listeners Removed comment

mieszko4 force-pushed the master branch from 8a1596f to 25d0125 Compare October 11, 2021 23:25

Merge branch 'mieszko4:master' into new-events

bb7d3c6

mieszko4 force-pushed the master branch from 25d0125 to e3cc872 Compare October 12, 2021 12:50

mieszko4 reviewed Oct 12, 2021

View reviewed changes

Merge branch 'mieszko4:master' into new-events

e8c39d7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to event listeners#157

Improvements to event listeners#157
pmogollons wants to merge 3 commits intomieszko4:masterfrom
propiedata:new-events

pmogollons commented Oct 8, 2021

Uh oh!

mieszko4 left a comment

Uh oh!

mieszko4 Oct 12, 2021

Uh oh!

mieszko4 Oct 12, 2021

Uh oh!

mieszko4 Oct 12, 2021

Uh oh!

mieszko4 Oct 12, 2021

Uh oh!

mieszko4 Oct 12, 2021

Uh oh!

mieszko4 Oct 12, 2021

Uh oh!

mieszko4 Oct 12, 2021

Uh oh!

mieszko4 Oct 12, 2021

Uh oh!

mieszko4 commented Oct 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-    sendEvent("audio.host.unmute", userId);
+    // 1. always put it in object
+    WritableMap data = Arguments.createMap();
+    // 2. this allows to name the params intuitively
+    data.putDouble("requestorId", userId);
+    // or
+    data.putDouble("requesteeId", userId);
+    // (userId does not tell us if it is a requestor or requestee)
+    // 3. name event more specefically (unmute could mean that host unmuted itself
+    sendEvent("audio.host.unmuteRequested", data);

Conversation

pmogollons commented Oct 8, 2021

Uh oh!

mieszko4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mieszko4 commented Oct 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants