1162991 - Prototype in-conversation text chat

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Mark Banner (:standard8)]

To start the text-chat work, we want to build a basic working "prototype" that is capable of doing simple text chat.

The basic idea here is simple text in text boxes, running over data channels.

We might not land it - depending on what state the prototype is in.

Comment 1

[Mark Banner (:standard8)]

Attachment: WIP Implement an initial layer of text chat for Loop conversations.

I think this is almost ready for review now. I'll take another look through it tomorrow, but the fundamentals are here.

The layout is wacky, but it is intended that way until we do the layout properly in other bugs.

Comment 2

[Mark Banner (:standard8)]

Attachment: Implement an initial layer of text chat for Loop conversations.

Now ready for review. This is the initial version, which is preffed off by default, but intended as something we can build on.

The data channels are set up, and there's a basic text chat view to allow chat to be tested.

We'll need to do more things, like providing a bit of a protocol for the data channels, as well as all the UX work, but that can build on top of this starter patch.

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8608057 [details] [diff] [review]
Implement an initial layer of text chat for Loop conversations.

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

Almost there! :-)

So we can argue about the pref name on IRC, but the PropTypes vs. propTypes does need to be fixed, I think.
The rest is all nits...

::: browser/app/profile/firefox.js
@@ +1698,5 @@
>  pref("image.mem.max_decoded_image_kb", 256000);
>  
>  pref("loop.enabled", true);
> +pref("loop.contextInConversations.enabled", true);
> +pref("loop.textChatInConversations.enabled", false);

Why not make it 'loop.textChat.enabled'? I don't see any other place for it than conversations, so it's kinda implicit...

::: browser/components/loop/content/js/conversation.jsx
@@ +97,5 @@
>      var dispatcher = new loop.Dispatcher();
>      var client = new loop.Client();
>      var sdkDriver = new loop.OTSdkDriver({
>        isDesktop: true,
> +      dataChannelsWanted: dataChannelsWanted,

nit: `useDataChannels`?

@@ +165,5 @@
>  
>      React.render(<AppControllerView
>        roomStore={roomStore}
>        dispatcher={dispatcher}
> +      mozLoop={navigator.mozLoop} />, document.querySelector('#main'));

While you're here, can you change these single quotes to doubles?

::: browser/components/loop/content/shared/css/conversation.css
@@ +689,5 @@
>  .conversation {
>    height: 100%;
>  }
>  
> +.fx-embedded .text-chat-view {

Please add a comment that all these text-chat styles are _very_ temporary.

::: browser/components/loop/content/shared/js/actions.js
@@ +176,5 @@
> +    /**
> +     * Used to send a message to the other peer.
> +     */
> +    SendTextChatMessage: Action.define("sendTextChatMessage", {
> +      message: String

Can we make this an Object right away, or do you think that's too premature?

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +409,5 @@
>        }
>        this.connections[connection.id] = connection;
>        this._notifyMetricsEvent("Session.connectionCreated", "peer");
>        this.dispatcher.dispatch(new sharedActions.RemotePeerConnected());
> +

whoops, newline.

@@ +513,5 @@
>  
>        var remoteElement = this.getScreenShareElementFunc();
>  
>        this.session.subscribe(stream,
> +        remoteElement, this._getCopyPublisherConfig);

AH! This was a bug?

::: browser/components/loop/content/shared/js/textChatStore.js
@@ +93,5 @@
> +        type: CHAT_MESSAGE_TYPES.SENT,
> +        message: actionData.message
> +      });
> +      this._sdkDriver.sendTextChatMessage(actionData.message);
> +      this.setStoreState({messageList: newList});

nit: spaces inside curlies.

::: browser/components/loop/content/shared/js/textChatView.jsx
@@ +14,5 @@
> +   */
> +  var TextChatEntry = React.createClass({
> +    mixins: [React.addons.PureRenderMixin],
> +
> +    PropTypes: {

Isn't this supposed to be `propTypes`?

@@ +123,5 @@
> +        message: this.state.messageDetail
> +      }));
> +
> +      // Reset the form to empty, ready for the next message.
> +      this.setState({messageDetail: ""});

nit: spaces inside the curlies.

::: browser/components/loop/standalone/content/css/webapp.css
@@ +291,5 @@
>    /* Hide  local media stream when feedback form is shown. */
>    display: none;
>  }
>  
> +.text-chat-view {

Same comment here about the temp-ness.

::: browser/components/loop/test/shared/textChatStore_test.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +

nit: one newline too many.

@@ +66,5 @@
> +      sinon.assert.calledWithExactly(fakeSdkDriver.sendTextChatMessage, message);
> +    });
> +
> +    it("should add the message to the list", function() {
> +      var message = "Its awesome!";

nit: s/Its/It's/

::: browser/components/loop/test/shared/textChatView_test.js
@@ +44,5 @@
> +        }));
> +    }
> +
> +    beforeEach(function() {
> +      store.setStoreState({textChatEnabled: true});

nit: spaces inside curlies throughout this file.

@@ +55,5 @@
> +
> +      expect(view.getDOMNode()).eql(null);
> +    });
> +
> +    it("should dispatch `sendTextChatMessage` when enter is pressed", function() {

s/`sendTextChatMessage`/SendTextChatMessage action/

Comment 4

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> ::: browser/components/loop/content/shared/js/actions.js
> @@ +176,5 @@
> > +    /**
> > +     * Used to send a message to the other peer.
> > +     */
> > +    SendTextChatMessage: Action.define("sendTextChatMessage", {
> > +      message: String
> 
> Can we make this an Object right away, or do you think that's too premature?

I was expecting it to be a bit more complicated to work out, but I think its probably reasonable to do now. I've not quite made the action an object, but what we send and receive is now an object.

> @@ +513,5 @@
> >  
> >        var remoteElement = this.getScreenShareElementFunc();
> >  
> >        this.session.subscribe(stream,
> > +        remoteElement, this._getCopyPublisherConfig);
> 
> AH! This was a bug?

Not really, but I thought it was cleaner switching to a getter as it matches the data channel one then.

> ::: browser/components/loop/content/shared/js/textChatStore.js
> @@ +93,5 @@
> > +        type: CHAT_MESSAGE_TYPES.SENT,
> > +        message: actionData.message
> > +      });
> > +      this._sdkDriver.sendTextChatMessage(actionData.message);
> > +      this.setStoreState({messageList: newList});
> 
> nit: spaces inside curlies.

I don't think we've historically been consistent about that. I think I marginally prefer without, but I've added them anyway.

Comment 5

[Mark Banner (:standard8)]

Attachment: Implement an initial layer of text chat for Loop conversations.

Updated for review comments.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8608655 [details] [diff] [review]
Implement an initial layer of text chat for Loop conversations.

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

Ship it!

::: browser/app/profile/firefox.js
@@ +1703,5 @@
>  // (This is intentionally on the high side; see bug 746055.)
>  pref("image.mem.max_decoded_image_kb", 256000);
>  
>  pref("loop.enabled", true);
> +pref("loop.contextInConversations.enabled", true);

nit: I'm not sure why you chose to move this up here. Preferably I'd like to keep everything where it is.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/fb805c7a67c5

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fb805c7a67c5