Add support for togetherjs in Popcorn Maker

VERIFIED FIXED

Status

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: reyre, Assigned: reyre)

Tracking

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Assignee: nobody → rick.eyre
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 927063
Attachment mime type: text/plain → text/x-github-pull-request
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.
Attachment #817350 - Flags: review?(scott) → review-
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #817350 - Flags: review?(scott)
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.
Attachment #817350 - Flags: review?(kieran.sedgwick)
(Assignee)

Updated

5 years ago
Attachment #817350 - Flags: review-
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.
Attachment #817350 - Flags: review?(kieran.sedgwick) → review+
(Assignee)

Comment 7

5 years ago
Wonderful, thanks!
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.
Attachment #817350 - Flags: review?(schranz.m) → review-
Comment on attachment 817350 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/263

Feature flag to hide this was added.
Attachment #817350 - Flags: review- → review+
(Assignee)

Comment 10

5 years ago
Follow up bug for real GUIDs in Popcorn Maker is filed as bug 929020.
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.
Attachment #817350 - Flags: review?(scott) → review-
(Assignee)

Updated

5 years ago
Attachment #817350 - Flags: review- → review?
(Assignee)

Updated

5 years ago
Attachment #817350 - Flags: review? → review?(scott)
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.
Attachment #817350 - Flags: review?(scott) → review+
(Assignee)

Comment 13

5 years ago
Awesome! Thanks for reviewing everyone.

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Needs verification.
Flags: needinfo?(schranz.m)
Status: RESOLVED → VERIFIED
Flags: needinfo?(schranz.m)
You need to log in before you can comment on or make changes to this bug.