#50 - Update the view when <video> elements are added or removed from the DOM (or when it's style or any parent style changes)
#59
Conversation
…ntok into issue-50 # Conflicts: # src/js/OTSession.coffee # www/opentok.js
…entok into issue-50 # Conflicts: # src/js/OTPublisher.coffee # src/js/OTSession.coffee # src/js/OTSubscriber.coffee # www/opentok.js
|
@msach22 Can we get this one out? Maybe in a |
|
@wolfenrain @mark-veenstra If possible, could you please update the PR so we can get this merged in! This would be great for |
…entok into issue-50 # Conflicts: # src/js/OTSubscriber.coffee # www/opentok.js
| @@ -18,7 +18,7 @@ class TBPublisher | |||
| constructor: (one, two) -> | |||
| @sanitizeInputs(one, two) | |||
| pdebug "creating publisher", {} | |||
| position = getPosition(@pubElement) | |||
| @position = getPosition(@pubElement) | |||
There was a problem hiding this comment.
Since this line changes position to this.position lines 36 and 37 should also change to position to this.position.
There was a problem hiding this comment.
Lines 37 and 37 have been changed to use the @position
src/js/OTSession.coffee
Outdated
| @dispatchEvent(new TBEvent("sessionConnected")) | ||
| @connection = new TBConnection( event.connection ) | ||
| @connections[event.connection.connectionId] = @connection | ||
| event = null | ||
| return @ | ||
| sessionDisconnected: (event) => | ||
| pdebug "sessionDisconnected event", event |
There was a problem hiding this comment.
Please remove this, we do not need to log this event.
src/js/OTSubscriber.coffee
Outdated
| position = getPosition(@element) | ||
| ratios = TBGetScreenRatios() | ||
| OT.getHelper().eventing(@) | ||
| Cordova.exec(TBSuccess, TBError, OTPlugin, "subscribe", [stream.streamId, position.top, position.left, width, height, zIndex, subscribeToAudio, subscribeToVideo, ratios.widthRatio, ratios.heightRatio] ) | ||
| Cordova.exec(@eventReceived, TBSuccess, OTPlugin, "addEvent", ["subscriberEvents"] ) | ||
|
|
||
| eventReceived: (response) => | ||
| @[response.eventType](response.data) | ||
| pdebug "subscriber event received", response |
There was a problem hiding this comment.
Please remove this because this causes issues with stats events which are fired constantly. We don't need to log to the console each time we receive this event.
src/js/OTSession.coffee
Outdated
| @@ -166,7 +164,11 @@ class TBSession | |||
| # event listeners | |||
| # todo - other events: connectionCreated, connectionDestroyed, signal?, streamPropertyChanged, signal:type? | |||
| eventReceived: (response) => | |||
| @[response.eventType](response.data) | |||
| pdebug "session event received", response | |||
There was a problem hiding this comment.
Please remove this line, we don't need to log these events
|
@wolfenrain @mark-veenstra It looks like this PR would break any Android device using API Level 16 or many other newer versions for that matter. It looks like https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver#Browser_compatibility |
|
@msach22 Regarding the But on the compatibility table they also state that when you want support from Android API level 18 to 26 you can use the vendor prefix Also note that Cordova is only supporting API level 19 ( Android 4.4) - 27 (Android 8) from the most recent version. See this link: https://cordova.apache.org/docs/en/latest/guide/platforms/android/#requirements-and-support. They also will dropped support when Android version dip below 5% on Google's distribution dashboard. So Android level 16 is even not supported by the most recent Cordova version. Also the market share of API level 16 and 17 is too small to keep it supported, that is why Cordova dropped it also. This PR helps you to update the views automatically. For everyone that has an unsupported WebView or browser they still can use the old API by calling |
|
@msach22 @mark-veenstra Changes have been made, also implemented the check for MutationObserver, if we want to have more coverage we could implement a bigger shim, but as @mark-veenstra mentioned Cordova itself doesn't support lower then API level 19, so I dont see a direct use for it. |
|
@wolfenrain @mark-veenstra Thanks for your comments. The OpenTok Android SDK supports Android API level 16 and above and this plugin currently supports Cordova 3.5.0 and above. This means that we would have to do a breaking change to add this functionality. I would really like to see this feature get merged in because I think it would make using the plugin easier, but we cannot drop support for Android 16. |
|
@msach22 If you still want to support API level 16 (Android 4.1.x) and 17 (Android 4.2.x), then this PR is blocking it. I understand you will not merge this in if this is the business decision. But we can keep it open for users who want a seperate solution and merge it in themselves. Or when you drop support for those old versions, then it can be merged as well. |
|
@wolfenrain @mark-veenstra Yes, unfortunately I cannot merge this in, but I will leave this open in case developers want to add it to their forks. Thanks again for the great PR! |
|
@mark-veenstra @wolfenrain Do you think we can add conditional observers where if the platform is the supported version we can use this observer, if not they will not have this functionality? |
|
It should be technically possible. It just dawned to me that a polyfill might be possible aswell? Could that be a more all round solution?
…On Dec 17, 2018, 7:58 PM, at 7:58 PM, Manik Sachdeva ***@***.***> wrote:
@mark-veenstra @wolfenrain Do you think we can add conditional
observers where if the platform is the supported version we can use
this observer, if not they will not have this functionality?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#59 (comment)
|
|
@wolfenrain Yea having a polyfill might be the best approach. Let me know what you think |
|
As I said here we could use that polyfill. Haven't done an indepth look. But it does provide us with the functionality needed. |
Solves #50