922108 - Add support for togetherjs in Popcorn Maker

Status: VERIFIED FIXED

(Webmaker Graveyard :: Popcorn Maker, defect)

Comment 1

[Rick Eyre (:reyre)]

The way TogetherJS keep pages synced is by allowing clients to send messages to one another. We can utilize this in Popcorn to package information about what has changed on the page and notifying TogetherJS to send that information to other clients. So we need to be able to hook into Popcorn's event mechanism and package enough information about what has changed that other clients can rebuild the change on their page as well.

I'm unsure what the best solution would be. I see that their are a ton of event mechanisms in Popcorn, but I'm unclear as to whether they would be able to plug into this easily.

Comment 2

[Rick Eyre (:reyre)]

Attachment: https://github.com/mozilla/popcorn.webmaker.org/pull/263

Comment 3

[Scott [:thecount] Downe]

Comment on attachment 817350 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/263

So far the things I have found are:

1. crash when updating sequencer, this is your fix:

diff --git a/public/templates/assets/plugins/sequencer/popcorn.sequencer.js b/public/templates/assets/plu
index d2eadd6..9e00011 100644
--- a/public/templates/assets/plugins/sequencer/popcorn.sequencer.js
+++ b/public/templates/assets/plugins/sequencer/popcorn.sequencer.js
@@ -494,10 +494,6 @@
           loadingHandler.add( options, options.addSource );
         }
       }
-      if ( updates.hasOwnProperty( "mute" ) ) {
-        options.mute = updates.mute;
-        options._volumeEvent();
-      }
       if ( updates.hasOwnProperty( "top" ) ) {
         options.top = updates.top;
         options._container.style.top = ( options.top || "0" ) + "%";
@@ -515,6 +511,10 @@
         options._container.style.width = ( options.width || "100" ) + "%";
       }
       if ( options.ready ) {
+        if ( updates.hasOwnProperty( "mute" ) ) {
+          options.mute = updates.mute;
+          options._volumeEvent();
+        }
         if ( updates.hasOwnProperty( "volume" ) ) {
           options.volume = updates.volume;
           options._volumeEvent();

2. Change the toggle off button to not say collaborate.
3. Has a lot of funtionality that's not just data sync, but ui and state sync still. We need to consider where we care about this, but I would say if we just sync track events, tracks and duration, we're safe.

Comment 4

[Rick Eyre (:reyre)]

(In reply to Scott [:thecount] Downe from comment #3)

> So far the things I have found are:
> 
> 1. crash when updating sequencer, this is your fix:

Thanks for the fix Scott. We can mark bug 927063 as a duplicate of this one then.

> 2. Change the toggle off button to not say collaborate.

I've fixed it so that people who join will see the correct text on the collaborate button.

> 3. Has a lot of funtionality that's not just data sync, but ui and state
> sync still. We need to consider where we care about this, but I would say if
> we just sync track events, tracks and duration, we're safe.

I've removed "mediatimeupdate" and "trackorderchanged" along these lines and as per our conversations.

Matt, I talked with Scott and we agreed that we can land a lot of the smaller fixes in other bugs like the automatic selection upon trackeventadded if that is alright with you.

Comment 5

[Scott [:thecount] Downe]

Comment on attachment 817350 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/263

Adding another reviewer to catch things sooner.

From a functionality point of view, I'm liking this. I just want to take a closer look at the code.

About the dupe, I think we'll keep it open and when this lands, I can mark the other as verified.

Comment 6

[Kieran Sedgwick (:sedge)]

Comment on attachment 817350 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/263

This is some whack stuff boys, love it.

Minor nits in the code.  Mostly concerning commenting - question you should ask yourself is, "does someone have to read the code to understand the context?". If yes, it needs more comments.

Comment 7

[Rick Eyre (:reyre)]

Wonderful, thanks!

Comment 8

[Matthew Schranz [:mjschranz]]

Comment on attachment 817350 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/263

We are going to need a feature flag to keep this hidden on production, but this is in a ready enough state to land and show off to people.

Comment 9

[Matthew Schranz [:mjschranz]]

Comment on attachment 817350 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/263

Feature flag to hide this was added.

Comment 10

[Rick Eyre (:reyre)]

Follow up bug for real GUIDs in Popcorn Maker is filed as bug 929020.

Comment 11

[Scott [:thecount] Downe]

Comment on attachment 817350 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/263

Some nits.

It works well.

I don't want this on prod, even if it is behind a feature flag.

Let's get this on staging after we do a prod push today.

Comment 12

[Scott [:thecount] Downe]

Comment on attachment 817350 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/263

R+.

We should land this on top of everything we currently have after prod is updated.

Comment 13

[Rick Eyre (:reyre)]

Awesome! Thanks for reviewing everyone.

Comment 14

[[github robot]]

Commit pushed to master at https://github.com/mozilla/popcorn.webmaker.org

https://github.com/mozilla/popcorn.webmaker.org/commit/21c3e50846fdcf388b560350d7671c1ca20efd2d
Fix Bug 922108 - Add support for TogetherJS in Popcorn Maker.

Comment 15

[Matthew Schranz [:mjschranz]]

Needs verification.