Closed Bug 1074686 Opened 5 years ago Closed 5 years ago

As a user, I should see a room preview when I am the only person in a room

Categories

(Hello (Loop) :: Client, enhancement, P1)

enhancement
Points:
13

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
Blocking Flags:
backlog Fx35+

People

(Reporter: standard8, Assigned: standard8)

References

()

Details

(Whiteboard: [rooms])

User Story

* As a user, I should see a room preview, so that I can prepare for a conversation

UX:
* Display the room name in the title bar

(other bugs will implement the buttons, camera preview and rename).

Implementation:
* Create an "empty room" view
* Construct a roomStore (similar idea to the conversationStore)
* When gathering data, get the data from the backend service via the mozLoop API
* Display the room name in the title bar for the empty room view; using the roomStore state

Patch 2:
* Back out various removeMe stubs

Attachments

(6 files, 11 obsolete files)

239.95 KB, image/png
Details
44.71 KB, patch
dmose
: review+
Details | Diff | Splinter Review
34.62 KB, patch
standard8
: review+
Details | Diff | Splinter Review
27.41 KB, patch
standard8
: review+
Details | Diff | Splinter Review
27.32 KB, patch
NiKo
: review+
Details | Diff | Splinter Review
49.75 KB, patch
NiKo
: review+
Details | Diff | Splinter Review
Attached image Expected UX
No description provided.
User Story: (updated)
Blocks: 1074688
Blocks: 1074693
Blocks: 1074694
Blocks: 1074696
Assignee: nobody → dmose
Comment on attachment 8499242 [details] [diff] [review]
First cut with view, action, and store

Review of attachment 8499242 [details] [diff] [review]:
-----------------------------------------------------------------

Mark, does this look basically right?  Ignore the if(1)'s of course, and changes to the tests are mostly broken, so ignore those too.  But other than that, it does cause the name to be set on the view by dispatching an action to the store...
Attachment #8499242 - Flags: feedback?(standard8)
Comment on attachment 8499242 [details] [diff] [review]
First cut with view, action, and store

Review of attachment 8499242 [details] [diff] [review]:
-----------------------------------------------------------------

The general ideas seem reasonable; comments inline.

::: browser/components/loop/content/js/conversationViews.jsx
@@ +305,5 @@
>        }
>      },
>    });
>  
> +  var EmptyRoomView = React.createClass({

Unless there's a strong reason, I'd be tempted to put this in a roomViews file.

@@ +323,5 @@
> +        this.setState(this.props.store.attributes);
> +        console.log("in change listener: this.state.roomName", this.state.roomName);
> +      }, this);
> +
> +      // XXX should this be managed by the store?

No, the store should have nothing to do with the display/dom

@@ +333,5 @@
> +    },
> +
> +    render: function() {
> +      console.log("in render: this.state.roomName", this.state.roomName);
> +      this.state.document.title = this.state.roomName;

Note: you're actually mutating the state here, without using setState, which seems wrong. I'm also not sure if there's some expected side effects for storing the document in the state.

I realise you're trying to make it testable with stubs, but I think this may be a bit more than necessary.

Maybe NiKo has some ideas here.
Attachment #8499242 - Flags: feedback?(standard8) → feedback?(nperriault)
Comment on attachment 8499242 [details] [diff] [review]
First cut with view, action, and store

Review of attachment 8499242 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/content/js/conversationViews.jsx
@@ +333,5 @@
> +    },
> +
> +    render: function() {
> +      console.log("in render: this.state.roomName", this.state.roomName);
> +      this.state.document.title = this.state.roomName;

If you want to keep the idea of passing the DOM document object as a prop to ease faking it in tests, pass it as a prop. It should definitely not be attached to component state.

Note that we could also create a mixin making mutating document properties like current title trivial, and easily unit-testable. Have a look at the mixins module and its setRootObject() method.

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +343,5 @@
>        this.dispatcher.dispatch(action);
>      }
>    });
>  
> +  var RoomStore = Backbone.Model.extend({

I second :Standard8 here, please move this to its own module file as this shouldn't be conceptually coupled to 1:1 calls.
Attachment #8499242 - Flags: feedback?(nperriault) → feedback-
After doing more pow-wowing today on how to make the interfaces, and talking with abr, pkerr and I agreed tweaked the APIs for the conversation window a bit to make them easier to implement on the gecko side.

<https://etherpad.mozilla.org/loop-rooms-rest-stuff> lines 28-41 are pretty much up-to-date.

Patch attached, in the midst of refactoring to make things testable and work with these new APIs.
Comment on attachment 8503882 [details] [diff] [review]
Test/impl EmptyRoomView, store, and actions

EmptyRoomView, LocalRoomStore, and all the various tests, actions, and bits to drive them.

Rebased-against current fx-team, unit tests should all pass, there is bunch of pref-controlled code for testing the setup which will go away in part two of the bug, which will land after the necessary support for gecko rooms does.
Attachment #8503882 - Flags: review?(standard8)
Attachment #8499242 - Attachment is obsolete: true
Attachment #8500828 - Attachment is obsolete: true
Attachment #8502963 - Attachment is obsolete: true
Comment on attachment 8503882 [details] [diff] [review]
Test/impl EmptyRoomView, store, and actions

Review of attachment 8503882 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly pair reviewed over Vidyo.

::: browser/components/loop/content/js/conversation.jsx
@@ +623,5 @@
>  
> +   if (localRoomStore) {
> +      dispatcher.dispatch(
> +        new sharedActions.SetupEmptyRoom({localRoomId: hash[1]}));
> +      return;

You should move this just after you've created the store, and consider extracting the whole resulting chunk into a separate function.

@@ +624,5 @@
> +   if (localRoomStore) {
> +      dispatcher.dispatch(
> +        new sharedActions.SetupEmptyRoom({localRoomId: hash[1]}));
> +      return;
> +    }

Push the location hash parsing within the conversationControllerView

From there instantiate a new RoomControllerView if we want a room
in the RoomControllerView

ConversationControllerView should be renamed to smthg more accurate like {App|Root}ControllerView

Stores should be created into dedicated controller views:
- conversationStore -> ConversationControllerView
- roomStore -> RoomControllerView

::: browser/components/loop/content/js/roomViews.jsx
@@ +26,5 @@
> +    console.info("loop.roomViews: rootObject set to ", obj);
> +    rootObject = obj;
> +  }
> +
> +  var EmptyRoomView = React.createClass({displayName: 'EmptyRoomView',

Unneeded when you're in a jsx file.

@@ +47,5 @@
> +    componentDidMount: function() {
> +      // YYY removeMe
> +      if (rootObject.navigator.mozLoop.getLoopBoolPref("test.alwaysUseRooms")) {
> +        return;
> +      }

As discussed over vidyo, please consider faking the mozLoop.rooms API locally in the store, as I did for the roomListStore.

@@ +53,5 @@
> +        "RoomCreationError", this.onCreationError);
> +    },
> +
> +    onCreationError: function(err) {
> +      rootObject.console.error("EmptyRoomView creation error: ", err);

Add a XXX comment to ensure we'll get some en-user friendly error message displayed into the UI.

@@ +58,5 @@
> +    },
> +
> +    _onLocalRoomStoreChanged: function() {
> +      this.setState(this.props.localRoomStore.getStoreState());
> +      this.setState(this.props.localRoomStore.getStoreState());

Unnecessary duplication.

@@ +76,5 @@
> +
> +    render: function() {
> +
> +      if (this.state.serverData && this.state.serverData.roomName) {
> +        rootObject.document.title = this.state.serverData.roomName;

Please have a look at andrei's ongoing work to bring a DocumentTitleMixin XXX bug number.

::: browser/components/loop/content/shared/js/localRoomStore.js
@@ +41,5 @@
> +  }
> +
> +  LocalRoomStore.prototype = _.extend({
> +    _storeState: {
> +      gatheringData: false,

Unused, should be removed.

@@ +60,5 @@
> +      var serverData;
> +      if (this.mozLoop.getLoopBoolPref("test.alwaysUseRooms")) {
> +        serverData = {roomName: "Donkeys"};
> +      } else {
> +        serverData = this.mozLoop.rooms.getRoomData(actionData.localRoomId);

Please talk to pkker about this part of the API? I think we should stick with a single async-style API for everything, to keep things consistent. See pad here https://etherpad.mozilla.org/loop-rooms-rest-stuff

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +109,5 @@
>        }));
>      });
>  
> +    describe("when locationHash begins with #room", function () {
> +      var fakeRoomID = String(33);

"33" is probably good enough :)

@@ +139,5 @@
> +          sinon.assert.calledWithExactly(loop.Dispatcher.prototype.dispatch,
> +            new loop.shared.actions.SetupEmptyRoom({localRoomId: fakeRoomID}));
> +        });
> +
> +      it("should create the ConversationControllerView with no localStore");

Shouldn't be required anymore if you refactor the init() function as we discussed previously.

@@ +234,5 @@
> +        removeCallback: function() {} } };
> +      // YYY removeMe
> +      fakeMozLoop.getLoopBoolPref = function() { return false };
> +      var fakeWindow = { document: {}, navigator: { mozLoop: fakeMozLoop } };
> +      loop.roomViews.setRootObject(fakeWindow);

We should really rather pass the mozLoop object as a dependency, so it'd be easier to mock.

::: browser/components/loop/test/desktop-local/roomViews_test.js
@@ +31,5 @@
> +  });
> +
> +  afterEach(function() {
> +    sinon.sandbox.restore();
> +    // XXX restore the original window with setRootObject?

Yeah reverting the root object for the module is needed; this problems also exists for the mixins module, please file a bug about it.

::: browser/components/loop/test/shared/localRoomStore_test.js
@@ +73,5 @@
> +  });
> +
> +  afterEach(function() {
> +    sandbox.restore();
> +  });

Nit: please move this next to beforeEach.
Attachment #8503882 - Flags: feedback+
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #10)
> @@ +624,5 @@
> > +   if (localRoomStore) {
> > +      dispatcher.dispatch(
> > +        new sharedActions.SetupEmptyRoom({localRoomId: hash[1]}));
> > +      return;
> > +    }
> 
> Push the location hash parsing within the conversationControllerView
> 
> From there instantiate a new RoomControllerView if we want a room
> in the RoomControllerView
> 
> ConversationControllerView should be renamed to smthg more accurate like
> {App|Root}ControllerView
> 
> Stores should be created into dedicated controller views:
> - conversationStore -> ConversationControllerView
> - roomStore -> RoomControllerView

After a bunch of discussion with Standard8, he suggested a different way of cleaning up the init function: by giving the AppControllerView its own store, and adding an action to set that store to create the sub-stores and communicate the hashes.  The more detailed rationale is all in the IRC logs.

In the interest of not pulling too much more stuff into this bug, we agreed that for now, I'll do the following:

* rename ConversationControllerView to AppControllerView
* I'll create and insert a RoomControllerView into the hierarchy above the EmptyRoomView
* I'll file a bug to spin the remaining cleanup out of this one.

Feel free to chat with Standard8 about this in more detail during your day tomorrow, of course.  :-)
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #10)
> ::: browser/components/loop/content/js/conversation.jsx
> @@ +623,5 @@
> >  
> > +   if (localRoomStore) {
> > +      dispatcher.dispatch(
> > +        new sharedActions.SetupEmptyRoom({localRoomId: hash[1]}));
> > +      return;
> 
> You should move this just after you've created the store, and consider
> extracting the whole resulting chunk into a separate function.

I can't actually move the whole thing, because the return has to happen after the AppControllerView has been rendered, not before.  So for now, I've left it alone, rather than split it into two parts, which seems even worse.  As discussed in IRC, this wants to be dealt with in the follow-in bug which I'll file.

> @@ +624,5 @@
> > +   if (localRoomStore) {
> > +      dispatcher.dispatch(
> > +        new sharedActions.SetupEmptyRoom({localRoomId: hash[1]}));
> > +      return;
> > +    }
> 
> Push the location hash parsing within the conversationControllerView
> 
> From there instantiate a new RoomControllerView if we want a room
> in the RoomControllerView
> 
> ConversationControllerView should be renamed to smthg more accurate like
> {App|Root}ControllerView
> 
> Stores should be created into dedicated controller views:
> - conversationStore -> ConversationControllerView
> - roomStore -> RoomControllerView

Based on further IRC discussion, for this patch, I'm renaming ConversationControllerView to AppControllerView for clarity.  

> ::: browser/components/loop/content/js/roomViews.jsx
> @@ +26,5 @@
> > +    console.info("loop.roomViews: rootObject set to ", obj);
> > +    rootObject = obj;
> > +  }
> > +
> > +  var EmptyRoomView = React.createClass({displayName: 'EmptyRoomView',
> 
> Unneeded when you're in a jsx file.

Fixed.
 
> @@ +47,5 @@
> > +    componentDidMount: function() {
> > +      // YYY removeMe
> > +      if (rootObject.navigator.mozLoop.getLoopBoolPref("test.alwaysUseRooms")) {
> > +        return;
> > +      }
> 
> As discussed over vidyo, please consider faking the mozLoop.rooms API
> locally in the store, as I did for the roomListStore.

Done.  Not _quite_ as cleanly, but cleanly enough, I think.

> @@ +53,5 @@
> > +        "RoomCreationError", this.onCreationError);
> > +    },
> > +
> > +    onCreationError: function(err) {
> > +      rootObject.console.error("EmptyRoomView creation error: ", err);
> 
> Add a XXX comment to ensure we'll get some en-user friendly error message
> displayed into the UI.

Done.

> @@ +58,5 @@
> > +    },
> > +
> > +    _onLocalRoomStoreChanged: function() {
> > +      this.setState(this.props.localRoomStore.getStoreState());
> > +      this.setState(this.props.localRoomStore.getStoreState());
> 
> Unnecessary duplication.

Fixed.

> @@ +76,5 @@
> > +
> > +    render: function() {
> > +
> > +      if (this.state.serverData && this.state.serverData.roomName) {
> > +        rootObject.document.title = this.state.serverData.roomName;
> 
> Please have a look at andrei's ongoing work to bring a DocumentTitleMixin
> XXX bug number.

I've added a reference to bug 1081079, though Andrei does not appear to be involved.
 
> ::: browser/components/loop/content/shared/js/localRoomStore.js
> @@ +41,5 @@
> > +  }
> > +
> > +  LocalRoomStore.prototype = _.extend({
> > +    _storeState: {
> > +      gatheringData: false,
> 
> Unused, should be removed.

Done.

> @@ +60,5 @@
> > +      var serverData;
> > +      if (this.mozLoop.getLoopBoolPref("test.alwaysUseRooms")) {
> > +        serverData = {roomName: "Donkeys"};
> > +      } else {
> > +        serverData = this.mozLoop.rooms.getRoomData(actionData.localRoomId);
> 
> Please talk to pkker about this part of the API? I think we should stick
> with a single async-style API for everything, to keep things consistent. See
> pad here https://etherpad.mozilla.org/loop-rooms-rest-stuff

API has been made async on the Etherpad after discussion with pkerr, and the content-side tests and code have been updated to match.

> ::: browser/components/loop/test/desktop-local/conversation_test.js
> @@ +109,5 @@
> >        }));
> >      });
> >  
> > +    describe("when locationHash begins with #room", function () {
> > +      var fakeRoomID = String(33);
> 
> "33" is probably good enough :)

Fixed.

> @@ +139,5 @@
> > +          sinon.assert.calledWithExactly(loop.Dispatcher.prototype.dispatch,
> > +            new loop.shared.actions.SetupEmptyRoom({localRoomId: fakeRoomID}));
> > +        });
> > +
> > +      it("should create the ConversationControllerView with no localStore");
> 
> Shouldn't be required anymore if you refactor the init() function as we
> discussed previously.

Removed, since the fix is not to test for this, but instead to refactor as we'll do in the spinoff bug.

> @@ +234,5 @@
> > +        removeCallback: function() {} } };
> > +      // YYY removeMe
> > +      fakeMozLoop.getLoopBoolPref = function() { return false };
> > +      var fakeWindow = { document: {}, navigator: { mozLoop: fakeMozLoop } };
> > +      loop.roomViews.setRootObject(fakeWindow);
> 
> We should really rather pass the mozLoop object as a dependency, so it'd be
> easier to mock.

Fixed, in both conversation_test.js and roomViews_test.js

> ::: browser/components/loop/test/desktop-local/roomViews_test.js
> @@ +31,5 @@
> > +  });
> > +
> > +  afterEach(function() {
> > +    sinon.sandbox.restore();
> > +    // XXX restore the original window with setRootObject?
> 
> Yeah reverting the root object for the module is needed; this problems also
> exists for the mixins module, please file a bug about it.

Fixed for both this code and the mixins tests, it was a one-liner, which was way less work than filing another bug.  :-)

> ::: browser/components/loop/test/shared/localRoomStore_test.js
> @@ +73,5 @@
> > +  });
> > +
> > +  afterEach(function() {
> > +    sandbox.restore();
> > +  });
> 
> Nit: please move this next to beforeEach.

Fixed.
Attachment #8503882 - Attachment is obsolete: true
Attachment #8503882 - Flags: review?(standard8)
Attachment #8504400 - Flags: review?(standard8)
Attachment #8504393 - Attachment is obsolete: true
Before I land, I still need to file a bug on createRoom and the related addCallback stuff for pkerr, as well as a cleanup bug for the app's init function.
Points: --- → 13
backlog: --- → Fx35+
Some related bugs that I've updated or spun-off:

CreateRoom: bug 1074699
JoinRoom: bug 1074688
clean up init() and friends: bug 1082729
User Story: (updated)
Comment on attachment 8504400 [details] [diff] [review]
Test/impl EmptyRoomView, store, and actions

Review of attachment 8504400 [details] [diff] [review]:
-----------------------------------------------------------------

This really isn't far off now, but I'd like one more hopefully brief look at it once you've addressed the comments.

::: browser/components/loop/content/js/conversation.jsx
@@ +509,5 @@
>        }, this);
>      },
>  
>      render: function() {
> +

nit: unnecessary blank line.

@@ +514,5 @@
> +      if (this.props.localRoomStore) {
> +        return (
> +          <EmptyRoomView
> +            mozLoop={navigator.mozLoop}
> +            localRoomStore={this.props.localRoomStore}

So normally I'd say that the empty room view should only be getting state, not the props. However, that's a side-effect of not having a room controller component, so I'm going to let it go for now, but we should fix it when we implement the room controller component.

@@ +629,5 @@
>      />, document.querySelector('#main'));
>  
> +   if (localRoomStore) {
> +      dispatcher.dispatch(
> +        new sharedActions.SetupEmptyRoom({localRoomId: hash[1]}));

I'm tempted to say this should be one action (shared with GatherCallData), but I think we should wait on that refactor until we sort out the ids in the location hashes.

::: browser/components/loop/content/js/roomViews.jsx
@@ +22,5 @@
> +   *
> +   * @param {Object}
> +   */
> +  function setRootObject(obj) {
> +    console.info("loop.roomViews: rootObject set to ", obj);

Do we really need this logging?

@@ +55,5 @@
> +          "RoomCreationError", this.onCreationError);
> +      }
> +    },
> +
> +    onCreationError: function(err) {

These functions all need jsdocs

@@ +77,5 @@
> +      }
> +    },
> +
> +    render: function() {
> +

nit: unnecessary blank line.

::: browser/components/loop/content/shared/js/localRoomStore.js
@@ +40,5 @@
> +    this.dispatcher.register(this, ["setupEmptyRoom"]);
> +  }
> +
> +  LocalRoomStore.prototype = _.extend({
> +    _storeState: {

Please make sure the functions and attributes in this file all have jsdocs.

I'm also tempted to say that we should just use setState/getState (and internally 'state'), as that reflects React. Its slightly less confusing what it is then.

Also, should we have documentation of what gets stored in the store's state? and defaults? This is something that Backbone models gives us the facility (with "defaults") for - and I feel like we're missing it here.

I guess I'm not too worried about defaults, but an easily accessible outline of what is stored would be useful.

@@ +66,5 @@
> +        cb(null, {roomName: "Donkeys"});
> +      }
> +    },
> +
> +

nit: only one blank line required.

@@ +68,5 @@
> +    },
> +
> +
> +    setupEmptyRoom: function(actionData) {
> +

nit: unnecessary blank lines throughout this function

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +232,5 @@
>          loop.conversation.IncomingConversationView);
>      });
> +
> +    it("should display the EmptyRoomView for rooms", function() {
> +      navigator.mozLoop.rooms = {

Why does this need to be navigator.mozLoop, when you're passing it into the room store?

If you keep it as navigator.mozLoop, then it needs deleting in an afterEach.

@@ +241,5 @@
> +        mozLoop: navigator.mozLoop,
> +        dispatcher: dispatcher
> +      });
> +
> +      ccView = mountTestComponent(localRoomStore);

It seems a bit strange to only be passing in the localRoomStore here as the parameter, when we don't pass anything else. We should either create it in the beforeEach, or reconsider what we pass in.

::: browser/components/loop/test/desktop-local/roomViews_test.js
@@ +83,5 @@
> +          var fakeRoomName = "Monkey";
> +          store.setStoreState({serverData: {roomName: fakeRoomName},
> +                               localRoomId: fakeRoomId});
> +
> +          mountTestComponent();

I'd put the mount before the setting of the state, because then you're also testing the change of state.

Although, when we pass in state to this view and not the store, then you'd be setting the state in mountTestComponent anyway, so it probably doesn't matter too much atm.

::: browser/components/loop/test/shared/localRoomStore_test.js
@@ +10,5 @@
> +  beforeEach(function() {
> +    sandbox = sinon.sandbox.create();
> +    dispatcher = new loop.Dispatcher();
> +  });
> +  afterEach(function() {

nit: prefer a blank line in-between the var, then in-between the before & afterEach

@@ +32,5 @@
> +  describe("#setupEmptyRoom", function() {
> +    var store, fakeMozLoop, fakeRoomId, fakeRoomName;
> +
> +    beforeEach(function() {
> +

nit: unnecessary blank line.

@@ +42,5 @@
> +
> +      store = new loop.store.LocalRoomStore(
> +        {mozLoop: fakeMozLoop, dispatcher: dispatcher});
> +      fakeMozLoop.rooms.getRoomData
> +        .withArgs(fakeRoomId).

I think we should keep the dots at the beginning of lines for consistency purposes.

@@ +90,5 @@
> +      function(done) {
> +
> +        var fakeError = new Error("fake error");
> +        fakeMozLoop.rooms.getRoomData
> +          .withArgs(fakeRoomId).

dot at beginning of the next line please.
Attachment #8504400 - Flags: review?(standard8) → review-
(In reply to Mark Banner (:standard8) from comment #17)
> ::: browser/components/loop/content/js/conversation.jsx
> @@ +509,5 @@
> >        }, this);
> >      },
> >  
> >      render: function() {
> > +
> 
> nit: unnecessary blank line.

Fixed.

> @@ +629,5 @@
> >      />, document.querySelector('#main'));
> >  
> > +   if (localRoomStore) {
> > +      dispatcher.dispatch(
> > +        new sharedActions.SetupEmptyRoom({localRoomId: hash[1]}));
> 
> I'm tempted to say this should be one action (shared with GatherCallData),
> but I think we should wait on that refactor until we sort out the ids in the
> location hashes.

Waiting sounds like the right call to me.
 
> ::: browser/components/loop/content/js/roomViews.jsx
> @@ +22,5 @@
> > +   *
> > +   * @param {Object}
> > +   */
> > +  function setRootObject(obj) {
> > +    console.info("loop.roomViews: rootObject set to ", obj);
> 
> Do we really need this logging?

No, removed.


> @@ +55,5 @@
> > +          "RoomCreationError", this.onCreationError);
> > +      }
> > +    },
> > +
> > +    onCreationError: function(err) {
> 
> These functions all need jsdocs

Added.

> @@ +77,5 @@
> > +      }
> > +    },
> > +
> > +    render: function() {
> > +
 
Removed.
 
> ::: browser/components/loop/content/shared/js/localRoomStore.js
> @@ +40,5 @@
> > +    this.dispatcher.register(this, ["setupEmptyRoom"]);
> > +  }
> > +
> > +  LocalRoomStore.prototype = _.extend({
> > +    _storeState: {
> 
> Please make sure the functions and attributes in this file all have jsdocs.

Done.

> I'm also tempted to say that we should just use setState/getState (and
> internally 'state'), as that reflects React. Its slightly less confusing
> what it is then.

Arguably, it's less confusing the way it is, because it's easy to disambiguate between the store stored in the object
and the state stored in the view.  FWIW, this is borrowed from RoomListStore, which follows the same convention.

> Also, should we have documentation of what gets stored in the store's state?
> and defaults? This is something that Backbone models gives us the facility
> (with "defaults") for - and I feel like we're missing it here.
>
> I guess I'm not too worried about defaults, but an easily accessible outline
> of what is stored would be useful.
 
Agreed.  Here's what I've put above the _storeState:

  LocalRoomStore.prototype = _.extend({

    /**
     * Stored data reflecting the local state of a given room, used to drive
     * the room's views.
     *
     * @property {Object} serverData - local cache of the data returned by
     *                                 MozLoop.getRoomData for this room.
     * @see https://wiki.mozilla.org/Loop/Architecture/Rooms#GET_.2Frooms.2F.7Btoken.7D
     *
     * @property {Error=} error - if the room is an error state, this will be
     *                            set to an Error object reflecting the problem;
     *                            otherwise it will be unset.
     *
     * @property {String} localRoomId - profile-local identifier used with
     *                                  the MozLoop API.
     */

Which isn't perfect, but is likely to be good enough.  That said, a better convention wouldn't be terrible.

> @@ +66,5 @@
> > +        cb(null, {roomName: "Donkeys"});
> > +      }
> > +    },
> > +
> > +
> 
> nit: only one blank line required.

Fixed.

> @@ +68,5 @@
> > +    },
> > +
> > +
> > +    setupEmptyRoom: function(actionData) {
> > +
> 
> nit: unnecessary blank lines throughout this function

I find that they add materially to the readability of the code; unless you real feel strongly, I'd suggest leaving them.
 
> ::: browser/components/loop/test/desktop-local/conversation_test.js
> @@ +232,5 @@
> >          loop.conversation.IncomingConversationView);
> >      });
> > +
> > +    it("should display the EmptyRoomView for rooms", function() {
> > +      navigator.mozLoop.rooms = {
> 
> Why does this need to be navigator.mozLoop, when you're passing it into the
> room store?

Because I'm extending the navigator.mozLoop created in a high-level before each.

> If you keep it as navigator.mozLoop, then it needs deleting in an afterEach.

That already happens in the higher-level afterEach.

> @@ +241,5 @@
> > +        mozLoop: navigator.mozLoop,
> > +        dispatcher: dispatcher
> > +      });
> > +
> > +      ccView = mountTestComponent(localRoomStore);
> 
> It seems a bit strange to only be passing in the localRoomStore here as the
> parameter, when we don't pass anything else. We should either create it in
> the beforeEach, or reconsider what we pass in. 

To be fixed in the init() cleanup, as per IRC discussion.

> ::: browser/components/loop/test/desktop-local/roomViews_test.js
> @@ +83,5 @@
> > +          var fakeRoomName = "Monkey";
> > +          store.setStoreState({serverData: {roomName: fakeRoomName},
> > +                               localRoomId: fakeRoomId});
> > +
> > +          mountTestComponent();
> 
> I'd put the mount before the setting of the state, because then you're also
> testing the change of state.
> 
> Although, when we pass in state to this view and not the store, then you'd
> be setting the state in mountTestComponent anyway, so it probably doesn't
> matter too much atm.

Leaving this alone, as per IRC conversation.

> ::: browser/components/loop/test/shared/localRoomStore_test.js
> @@ +10,5 @@
> > +  beforeEach(function() {
> > +    sandbox = sinon.sandbox.create();
> > +    dispatcher = new loop.Dispatcher();
> > +  });
> > +  afterEach(function() {
> 
> nit: prefer a blank line in-between the var, then in-between the before &
> afterEach

OK, fixed.

> @@ +32,5 @@
> > +  describe("#setupEmptyRoom", function() {
> > +    var store, fakeMozLoop, fakeRoomId, fakeRoomName;
> > +
> > +    beforeEach(function() {
> > +
> 
> nit: unnecessary blank line.

Fixed.

> @@ +42,5 @@
> > +
> > +      store = new loop.store.LocalRoomStore(
> > +        {mozLoop: fakeMozLoop, dispatcher: dispatcher});
> > +      fakeMozLoop.rooms.getRoomData
> > +        .withArgs(fakeRoomId).
> 
> I think we should keep the dots at the beginning of lines for consistency
> purposes.

Sold, and fixed.

> @@ +90,5 @@
> > +      function(done) {
> > +
> > +        var fakeError = new Error("fake error");
> > +        fakeMozLoop.rooms.getRoomData
> > +          .withArgs(fakeRoomId).
> 
> dot at beginning of the next line please.

Sold, and fixed.
Attachment #8504400 - Attachment is obsolete: true
Attachment #8505042 - Flags: review?(standard8)
Comment on attachment 8505042 [details] [diff] [review]
Test/impl EmptyRoomView, store, and actions

Review of attachment 8505042 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, that's better with the changes. Just one comment about the = below you might want to address.

> > > +    setupEmptyRoom: function(actionData) {
> > > +
> > 
> > nit: unnecessary blank lines throughout this function
> 
> I find that they add materially to the readability of the code; unless you
> real feel strongly, I'd suggest leaving them.

I think it leads to inconsistency of style. I personally find it harder to scan as it isn't so obvious where the function separations are. I think you can leave them in this patch, but we should consider if this is something we really want to define a consistent style on.

::: browser/components/loop/content/shared/js/localRoomStore.js
@@ +48,5 @@
> +     * @property {Object} serverData - local cache of the data returned by
> +     *                                 MozLoop.getRoomData for this room.
> +     * @see https://wiki.mozilla.org/Loop/Architecture/Rooms#GET_.2Frooms.2F.7Btoken.7D
> +     *
> +     * @property {Error=} error - if the room is an error state, this will be

nit: not sure you need the = here.
Attachment #8505042 - Flags: review?(standard8) → review+
(In reply to Mark Banner (:standard8) from comment #20)
> Comment on attachment 8505042 [details] [diff] [review]
> Test/impl EmptyRoomView, store, and actions
> 
> Review of attachment 8505042 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, that's better with the changes. Just one comment about the = below you
> might want to address.

That's jsdoc3 for "optional".

> > > > +    setupEmptyRoom: function(actionData) {
> > > > +
> > > 
> > > nit: unnecessary blank lines throughout this function
> > 
> > I find that they add materially to the readability of the code; unless you
> > real feel strongly, I'd suggest leaving them.
> 
> I think it leads to inconsistency of style. I personally find it harder to
> scan as it isn't so obvious where the function separations are. I think you
> can leave them in this patch, but we should consider if this is something we
> really want to define a consistent style on.

Fair enough.  I tried removing the lines as you suggest, and it's not as bad as I remembered.  Someday we'll set up a .editorconfig file for all of our style stuff.  :-)
No longer blocks: 1082729
Keywords: leave-open
Attachment #8505060 - Attachment description: Test/impl EmptyRoomView, store, and actions, r=Standard8 → [part 1, landed on fx-team] Test/impl EmptyRoomView, store, and actions, r=Standard8
Updated dependencies to reflect what part 2 is blocked on.
No longer blocks: 1074699
Depends on: 1074664
Part 2 is also blocked on 1074699, but Bugzilla gets mad about circular dependencies when I try to express that.
The error-handling piece has moved to bug 1083340.
User Story: (updated)
Priority: -- → P1
I'm assuming it makes the most sense for one of (Standard8, mikedeboer, Niko) to pick this up...
Assignee: dmose → nobody
This patch adds basic views & styling for rooms in Loop Desktop conversation window.
Attachment #8519900 - Flags: review?(standard8)
Renamed patch & updated commit message.
Attachment #8519900 - Attachment is obsolete: true
Attachment #8519900 - Flags: review?(standard8)
Attachment #8519904 - Flags: review?(standard8)
Comment on attachment 8519904 [details] [diff] [review]
Part 2: Room views for Loop Desktop.

Review of attachment 8519904 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. Although this isn't functional yet - and we know there's some layout issues in the invitation view - I'm working on adding those parts in for part 3. So this is r=me with the one comment fixed.

::: browser/components/loop/content/js/roomViews.jsx
@@ +112,5 @@
> +      };
> +    },
> +
> +    getInitialState: function() {
> +      return this.props.roomStore.getStoreState("activeRoom");

Either this needs to be in the mixin, or it needs to be in DesktopRoomInvitationView as well.
Attachment #8519904 - Flags: review?(standard8) → review+
Addressed comment, also added unit test for the ActiveRoomStoreMixin.
Attachment #8519904 - Attachment is obsolete: true
Attachment #8519927 - Flags: review?(standard8)
Attachment #8519927 - Flags: review?(standard8) → review+
This is a revised view architecture for Loop desktop rooms, as per discussed on IRC.
Attachment #8520073 - Flags: review?(standard8)
Comment on attachment 8520073 [details] [diff] [review]
[checked in] Part 3 - Revamped view architecture for Desktop Loop rooms.

Review of attachment 8520073 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, and it works with the new room store code.
Attachment #8520073 - Flags: review?(standard8) → review+
This is a simple patch to separate out some changes that we need from the next part - to make it simpler for review.

The PeerHungupCall -> RemotePeerDisconnected change is because I want to add a RemotePeerConnected action in the next part as that makes more sense with rooms.
Attachment #8520214 - Flags: review?(nperriault)
This is the WIP for part 5. Its largely there, but I want to do a bit more manual testing of the flows, and obviously bring the patch up to review standard (comments + tests).
Attachment #8505060 - Attachment description: [part 1, landed on fx-team] Test/impl EmptyRoomView, store, and actions, r=Standard8 → [checked in] Part 1 Test/impl EmptyRoomView, store, and actions, r=Standard8
Comment on attachment 8519927 [details] [diff] [review]
[checked in] Part 2: Room views for Loop Desktop.

https://hg.mozilla.org/integration/fx-team/rev/f1eeacead124
Attachment #8519927 - Attachment description: Part 2: Room views for Loop Desktop. → [on fx-team] Part 2: Room views for Loop Desktop.
Comment on attachment 8520214 [details] [diff] [review]
[checked in] Part 4. Improve Loop conversation store registration to only register for actions when they need it, and change PeerHungupCall into RemotePeerDisconnected to fit better with what it is for.

Review of attachment 8520214 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8520214 - Flags: review?(nperriault) → review+
Attachment #8519927 - Attachment description: [on fx-team] Part 2: Room views for Loop Desktop. → [checked in] Part 2: Room views for Loop Desktop.
Comment on attachment 8520073 [details] [diff] [review]
[checked in] Part 3 - Revamped view architecture for Desktop Loop rooms.

https://hg.mozilla.org/integration/fx-team/rev/4b4b6b20ae2d
Attachment #8520073 - Attachment description: Part 3 - Revamped view architecture for Desktop Loop rooms. → [on fx-team] Part 3 - Revamped view architecture for Desktop Loop rooms.
Comment on attachment 8520214 [details] [diff] [review]
[checked in] Part 4. Improve Loop conversation store registration to only register for actions when they need it, and change PeerHungupCall into RemotePeerDisconnected to fit better with what it is for.

https://hg.mozilla.org/integration/fx-team/rev/f503d2369b2e
Attachment #8520214 - Attachment description: Part 4. Improve Loop conversation store registration to only register for actions when they need it, and change PeerHungupCall into RemotePeerDisconnected to fit better with what it is for. → [on fx-team] Part 4. Improve Loop conversation store registration to only register for actions when they need it, and change PeerHungupCall into RemotePeerDisconnected to fit better with what it is for.
This does the rest of the work to get the desktop rooms hooked up to the sdk and working properly. The main idea here is once the window is opened, we connect to the server and "publish" the stream. I checked that publishing the stream doesn't send data (this is webrtc, so without a remote endpoint, it shouldn't).

Then, when the remote party joins, they connect and we show the full room view.
Attachment #8520215 - Attachment is obsolete: true
Attachment #8520591 - Flags: review?(nperriault)
Attached patch Temp standalone hookup (obsolete) — Splinter Review
This is a standalone hookup I did, not intended for landing, just so that I could more easily test the desktop side of things. We'll do the standalone views etc in bug 1074702 and others.
Comment on attachment 8520591 [details] [diff] [review]
[checked in] Part 5 Hook up the active room store to the sdk for Loop rooms on desktop to enable audio and video in rooms.

Review of attachment 8520591 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with comments addressed.

::: browser/components/loop/content/js/roomViews.jsx
@@ -111,5 @@
> -
> -    getDefaultProps: function() {
> -      return {
> -        video: {enabled: true, visible: true},
> -        audio: {enabled: true, visible: true}

Please check that this doesn't break the UI showcase :)

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +307,5 @@
> +    remotePeerDisconnected: function() {
> +      // As we only support two users at the moment, we just set this
> +      // back to joined.
> +      this.setStoreState({
> +        roomState: ROOM_STATES.JOINED

should be ROOM_STATES.CONNECTED (could be renamed to SESSION_CONNECTED maybe)

@@ +357,5 @@
>      /**
>       * Handles leaving a room. Clears any membership timeouts, then
>       * signals to the server the leave of the room.
>       */
> +    _leaveRoom: function(nextState) {

Nit: missing jsdocs

::: browser/components/loop/test/shared/activeRoomStore_test.js
@@ +402,5 @@
> +  });
> +
> +  describe("#setMute", function() {
> +    it("should save the mute state for the audio stream", function() {
> +      store.setStoreState({"audioMuted": false});

Nit: unneeded double quotes.

@@ +436,5 @@
> +  describe("#remotePeerDisconnected", function() {
> +    it("should set the state to `JOINED`", function() {
> +      store.remotePeerDisconnected();
> +
> +      expect(store.getStoreState().roomState).eql(ROOM_STATES.JOINED);

This should be testing for CONNECTED.

@@ +450,5 @@
>        });
>      });
>  
> +    it("should disconnect from the servers via the sdk", function() {
> +      store.windowUnload();

Slightly unrelated: please ensure a bug is filed for the issue making the standalone application not completing all pending operations when the window "unload" event is caught.
Attachment #8520591 - Flags: review?(nperriault) → review+
Comment on attachment 8520591 [details] [diff] [review]
[checked in] Part 5 Hook up the active room store to the sdk for Loop rooms on desktop to enable audio and video in rooms.

https://hg.mozilla.org/integration/fx-team/rev/1c1e25f36e74
Attachment #8520591 - Attachment description: Part 5 Hook up the active room store to the sdk for Loop rooms on desktop to enable audio and video in rooms. → [on fx-team] Part 5 Hook up the active room store to the sdk for Loop rooms on desktop to enable audio and video in rooms.
Blocks: 1074690
Blocks: 1083340
Assignee: nobody → standard8
Iteration: --- → 36.2
Target Milestone: --- → mozilla36
Iteration: 36.2 → 36.3
Comment on attachment 8505060 [details] [diff] [review]
[checked in] Part 1 Test/impl EmptyRoomView, store, and actions, r=Standard8

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8505060 - Flags: approval-mozilla-aurora?
Whiteboard: [rooms] → [rooms][fixed on fx-team, need all 5 patches to land and then will mark resolved]
Attachment #8505060 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/4b4b6b20ae2d
https://hg.mozilla.org/mozilla-central/rev/f503d2369b2e
https://hg.mozilla.org/mozilla-central/rev/1c1e25f36e74
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [rooms][fixed on fx-team, need all 5 patches to land and then will mark resolved] → [rooms]
Attachment #8520073 - Attachment description: [on fx-team] Part 3 - Revamped view architecture for Desktop Loop rooms. → [checked in] Part 3 - Revamped view architecture for Desktop Loop rooms.
Attachment #8520214 - Attachment description: [on fx-team] Part 4. Improve Loop conversation store registration to only register for actions when they need it, and change PeerHungupCall into RemotePeerDisconnected to fit better with what it is for. → [checked in] Part 4. Improve Loop conversation store registration to only register for actions when they need it, and change PeerHungupCall into RemotePeerDisconnected to fit better with what it is for.
Attachment #8520591 - Attachment description: [on fx-team] Part 5 Hook up the active room store to the sdk for Loop rooms on desktop to enable audio and video in rooms. → [checked in] Part 5 Hook up the active room store to the sdk for Loop rooms on desktop to enable audio and video in rooms.
Attachment #8520592 - Attachment is obsolete: true
Flags: qe-verify+
Flags: in-moztrap?
Flags: qe-verify-
Flags: qe-verify+
Flags: in-moztrap?
You need to log in before you can comment on or make changes to this bug.