1042060 - Desktop client needs automated default answering mode based on caller's mode

Status: VERIFIED FIXED

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

Description

[Romain Testard [:RT]]

It seems callees will want to use the same media as the ones used by the callers.

In order to make this choice simpler for the users, the desktop client should expose "default" and "behind chevron" answering modes in the incoming call window as follows:
* If the incoming call is audio only:
     * The main answering button allows to answer with audio only
     * Behind the chevron, the user can select to answer with audio+video
* If the incoming call is audio+video
     * The main answering button allows to answer with audio+video 
     * Behind the chevron, the user can select to answer with audio only

Comment 1

[:shell escalante]

Same question about if this needs to be in for the clean MVP experience.  It definitely makes sense - but looking at the list, a lot of bugs we'd pushed to Fx34 won't make it so we're trying to make the tough calls early so we have a more solid target.

Comment 2

[:shell escalante]

RT is going to discuss UX with Darrin when he comes back - as a group of bugs that are great - but we could still possibly release a good MVP without.

Comment 3

[Maire Reavy [:mreavy]]

This is definitely one I'd like to move out to Fx 35.

Comment 4

[Darrin Henein [:darrin]]

Not sure what exactly is needed from me here -- I agree with the proposed change: default answer mode is that of the incoming call, and chevron exposes variants.

Comment 5

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement default answering mode for desktop

Comment 6

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8469388 [details] [diff] [review]
Implement default answering mode for desktop

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

This is on the right track, though it feels incomplete until it's actually useable along with the server — hence why I'm giving the patch a feedback+ only.

::: browser/components/loop/content/shared/css/conversation.css
@@ +8,5 @@
>    position: relative;
>  }
>  
> +/* Hide the video placeholder if stream not available */
> +.conversation-audio-only .media {

.conversation.audio-only (see comment below)

::: browser/components/loop/content/shared/js/models.js
@@ +137,5 @@
>        this.set({
>          sessionId:    sessionData.sessionId,
>          sessionToken: sessionData.sessionToken,
> +        apiKey:       sessionData.apiKey,
> +        callType:     sessionData.callType || false

Please add a default value in the `defaults` model object property.

::: browser/components/loop/content/shared/js/views.jsx
@@ +219,5 @@
>  
>      getInitialState: function() {
>        return {
> +        video: {enabled: true},
> +        audio: {enabled: true},

How about adding matching props? That way you could update the UI components showcase to have the component when configured for an audio-only call (see below).

So please define getInitialProps and update getInitialState to use these by default, eg.:

    getInitialProps: function() {
      return {
        video: {enabled: true},
        audio: {enabled: true},
      };
    },

    getInitialState: function() {
      return {
        video: this.props.video,
        audio: this.props.audio,
      };
    },

@@ +224,5 @@
>        };
>      },
>  
> +    componentWillMount: function() {
> +      if (this.props.model.get("callType") == "audio") {

Actually, it feels to me like this shouldn't be handled here, rather in the conversation router when we first create the component instance, passing it the appropriate audio & video props as suggested above.

Nit: Please use strict equality check here.

@@ +225,5 @@
>      },
>  
> +    componentWillMount: function() {
> +      if (this.props.model.get("callType") == "audio") {
> +        this.publisherConfig['publishVideo'] = false;

Nit: this.publisherConfig.publishVideo = false is probably just fine.

@@ +342,5 @@
>      render: function() {
> +      var cx = React.addons.classSet;
> +      var conversationWindowClasses = cx({
> +        conversation: true,
> +        "conversation-audio-only": !this.state.video.enabled

How about just `audio-only`? Styles could target this using .conversation.audio-only.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +126,5 @@
>              <Example summary="Default">
>                <ConversationView video={{enabled: true}} audio={{enabled: true}} />
>              </Example>
> +            <Example summary="Audio only">
> +              <ConversationView video={{enabled: false}} audio={{enabled: true}} />

Props won't work here as the component doesn't currently handle them :)

Comment 7

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) from comment #6)
> This is on the right track, though it feels incomplete until it's actually
> useable along with the server — hence why I'm giving the patch a feedback+
> only.

Clarification with Nicolas over irc indicates that we should probably have done bug 1048333 first, as this could then be fully tested. Although he did say if the UI components showcase is fixed it's probably okay to land this.

Comment 8

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Add default answer mode based on caller's mode

Comment 9

[Mark Banner (:standard8)]

Andrei, what's the status of this, is it ready for review?

Comment 10

[Andrei Oprea [mention me on [e-mail]]]

I assumed this will land after the visual spike so I rebased off of that and am waiting on it to land. But otherwise it's ready.

Comment 11

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement default answering mode for desktop client

Comment 12

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8484772 [details] [diff] [review]
Implement default answering mode for desktop client

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

Too much code duplication, room for refactoring. Otherwise the global approach is good.

::: browser/components/loop/content/js/conversation.jsx
@@ +123,5 @@
>              <div className="fx-embedded-incoming-call-button-spacer"></div>
>  
> +            <AcceptCallButton hasVideoStream={this.props.video.enabled}
> +                      audioCallHandler={this.handleAccept("audio")}
> +                      videoCallHandler={this.handleAccept("audio-video")} />

We should pass a single handler here.

@@ +198,5 @@
> +          </div>
> +          /* jshint ignore:end */
> +        );
> +      }
> +    }

Lots of code duplication here; actually the logic shouldn't sit in a "button" component. The button component should be as much agnostic as possible.

I'd suggest moving the test for which click handler should be applied to an agnostic button into the parent component.

Comment 13

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement default answering mode for desktop client

Comment 14

[Andrei Oprea [mention me on [e-mail]]]

loop.properties is missing a string for answer with video tooltip [0]

[0] https://github.com/mozilla/gecko-dev/blob/fx-team/browser/locales/en-US/chrome/browser/loop/loop.properties#L176

Comment 15

[Mark Banner (:standard8)]

(In reply to Andrei Oprea [:andreio] from comment #14)
> loop.properties is missing a string for answer with video tooltip [0]

Ok, add it for trunk/central. I don't think this is an uplift required bug, but if it is, then we can just drop the tooltip when we uplift it, imo that isn't super-critical.

Comment 16

[Maire Reavy [:mreavy]]

I agree with Mark.  This bug isn't on the uplift list, and if we later decide we do want to uplift it, we could drop the tooltip when we uplift.

Comment 17

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement default answering mode for desktop client

Comment 18

[Andrei Oprea [mention me on [e-mail]]]

Added in the extra string needed.

Comment 19

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8485928 [details] [diff] [review]
Implement default answering mode for desktop client

Transfering review to :mikedeboer as per IRC discussion.

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8485928 [details] [diff] [review]
Implement default answering mode for desktop client

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

::: browser/components/loop/content/js/conversation.jsx
@@ +84,5 @@
> +    _answerModeProps: function() {
> +      var videoAnswer = this._handleAccept("audio-video");
> +      var audioAnswer = this._handleAccept("audio");
> +      var hasVideo = this.props.video.enabled;
> +      return {

I think you need to rethink this a little bit...

So if a video answer is not possible (video.enabled === false), the answer of type 'video' should be listed last and greyed out (disabled) and audio listed on top.
If a video answer IS possible, that answer should be the first listed answer and the audio-only answer should be placed second.

At least, that's what I'd expect.

IOW, I'd like to see this function return something like this:

```js
// Video enabled:
return {
  primary: {
    handler: videoAnswer,
    className: "fx-embedded-btn-icon-video",
    tooltip: "incoming_call_accept_audio_video_tooltip"
  },
  secondary: {
    handler: audioAnswer,
    className: "fx-embedded-btn-icon-audio-small",
    tooltip: "incoming_call_accept_audio_only_tooltip"
  }
};

// Video not available:
return {
  primary: {
    handler: audioAnswer,
    className: "fx-embedded-btn-icon-audio",
    tooltip: "incoming_call_accept_audio_only_tooltip"
  },
  secondary: {
    handler: function() {},
    className: "fx-embedded-btn-video-small disabled",
    tooltip: "incoming_call_accept_audio_video_tooltip"
  }
};
```

Comment 21

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement default answering mode for desktop client

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8486551 [details] [diff] [review]
Implement default answering mode for desktop client

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

::: browser/components/loop/content/js/conversation.jsx
@@ +107,5 @@
> +            className: "fx-embedded-btn-video-small",
> +            tooltip: "incoming_call_accept_audio_video_tooltip"
> +          }
> +        };
> +      }

What do you think about:

```js
var videoButton = {
  handler: this._handleAccept("audio-video"),
  className: "fx-embedded-btn-icon-video",
  tooltip: "incoming_call_acceopt_audio_video_tooltip"
};
var audioButton = {
  handler: this._handleAccept("audio"),
  className: "fx-embedded-btn-audio-small",
  tooltip: "incoming_call_accept_audio_only_tooltip"
};
var props = {};
props.primary = videoButton;
props.secondary = audioButton;

// When video is not enabled on this call, we swap the buttons around.
if (!this.props.video.enabled) {
  audioButton.className = "fx-embedded-btn-icon-audio";
  videoButton.className = "fx-embedded-btn-video-small";
  props.primary = audioButton;
  props.secondary = videoButton;
}

return props;
```

@@ +165,5 @@
>  
>    /**
> +   * Incoming call view accept button, renders different primary actions
> +   * (answer with video / with audio only) based on the props received
> +   * */

nits: superfluous space between asterisks

Comment 23

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement default answering mode for desktop client

Comment 24

[Mike de Boer [:mikedeboer]]

Comment on attachment 8486813 [details] [diff] [review]
Implement default answering mode for desktop client

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

::: browser/components/loop/content/js/conversation.jsx
@@ +28,5 @@
>  
>      getInitialProps: function() {
> +      return {
> +        showDeclineMenu: false,
> +        video: {enabled: true}

I thought you mentioned you'd change this to a Boolean property?

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +612,5 @@
> +    describe("default answer mode", function() {
> +      it("should display video as primary answer mode", function() {
> +        view = TestUtils.renderIntoDocument(loop.conversation.IncomingCallView({
> +          model: model,
> +          video: {enabled: true}

make sure to change these too.

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +173,5 @@
>  initiate_call_button_label2=Ready to start your conversation?
>  incoming_call_title2=Conversation Request
>  incoming_call_accept_button=Accept
>  incoming_call_accept_audio_only_tooltip=Accept with voice
> +incoming_call_accept_audio_video_tooltip=Accept with video

should this be more descriptive by adding 'voice'? So it'd become 'Accept with voice and video'.

Comment 25

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Implement default answering mode for desktop client

Comment 26

[Mike de Boer [:mikedeboer]]

Thanks Andrei!

Comment 27

[Andrei Oprea [mention me on [e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=2c8003ccf6fb

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[u279076]

Paul, can you please test this? It should be in tomorrow's Nightly.

Comment 30

[Phil Ringnalda (:philor)]

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

Comment 31

[Paul Silaghi, QA [:pauly]]

Verified fixed 35.0a1 (2014-09-14) Win 7, OS X 10.8.5, Ubuntu 13.04

Comment 32

[u279076]

(In reply to Paul Silaghi, QA [:pauly] from comment #31)
> Verified fixed 35.0a1 (2014-09-14) Win 7, OS X 10.8.5, Ubuntu 13.04

Thanks Paul. Can you please add a Moztrap test which covers this user story? Product is Loop, Version is 35.

Comment 33

[Paul Silaghi, QA [:pauly]]

Done - https://moztrap.mozilla.org/manage/case/14658/

Comment 34

[u279076]

Paul can you please test this again across platforms using the following Try-server build?
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9eb2cbac352

Comment 35

[Paul Silaghi, QA [:pauly]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #34)
> Paul can you please test this again across platforms using the following
> Try-server build?
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> f9eb2cbac352
Verified fixed Win 7, OS X 10.8.5, Ubuntu 13.04

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8487551 [details] [diff] [review]
Implement default answering mode for desktop client

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 37

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/701fcc5fcce9
Replaces tooltip string with an empty string (per above) to avoid localization issues

Comment 38

[Axel Hecht [:Pike]]

jesup, dropping a string is not the same thing as adding an empty string. If you reference comment 15.

Dropping the string is actually dropping it and it's call sites.

Note, I don't see Lawrence approving the l10n change in this bug, as it's suggested by the commit message.

Comment 39

[Francesco Lodolo [:flod]]

Besides the unclear approval, I'll add that this will clash on merge day with mozilla-central having a string with ID incoming_call_accept_audio_video_tooltip and an actual content.

The sooner you back-out this change the better. Next time pinging someone from l10n-drivers would seem a wiser choice.

Comment 40

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Axel Hecht [:Pike] from comment #38)
> jesup, dropping a string is not the same thing as adding an empty string. If
> you reference comment 15.
> 
> Dropping the string is actually dropping it and it's call sites.
> 
> Note, I don't see Lawrence approving the l10n change in this bug, as it's
> suggested by the commit message.

My apologies; I was merging the group of bugs and the NI got auto-cleared.

I misunderstood the direction from lawrence to remove the string before landing and didn't realize this would cause problems; again, my apologies.  There should be no l10n changes in this landing.

I'll prepare a patch to remove the string entirely and all the call-sites ASAP.  

Thanks

Comment 41

[Randell Jesup [:jesup] (needinfo me)]

Fully backed out tooltip (not "" anymore) (and tested) now:
https://hg.mozilla.org/releases/mozilla-aurora/rev/aa98eb8873a2
Landed under rs=me l10n=backout_string_change so as to minimize the impact of this on localizers per discussion with flod

Comment 42

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Francesco Lodolo [:flod] from comment #39)
> Besides the unclear approval, I'll add that this will clash on merge day
> with mozilla-central having a string with ID
> incoming_call_accept_audio_video_tooltip and an actual content.

Due to the large number of Loop changes that are landing, I reviewed the changes with Maire and Randell and granted approval for all, which I'm subsequently noting in Bugzilla.

(In reply to Randell Jesup [:jesup] from comment #37)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/701fcc5fcce9
> Replaces tooltip string with an empty string (per above) to avoid
> localization issues

I see that this was submitted with l10n=lmandel. To be clear, I did not grant l10n approval for this bug. l10n approval should only come when we're making String changes on Aurora. A failing commit due to the a string change should be taken as an indication that something is wrong with the patch that needs to be corrected before landing. Is the message from the l10n hook clear about the issue and how to proceed?

(In reply to Randell Jesup [:jesup] from comment #40)
> I misunderstood the direction from lawrence to remove the string before
> landing and didn't realize this would cause problems; again, my apologies. 
> There should be no l10n changes in this landing.

As Randell said, I did specifically call this out and requested that the string be removed. This is simply a misunderstanding that he has corrected.

Comment 43

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Randell Jesup [:jesup] from comment #41)
> Fully backed out tooltip (not "" anymore) (and tested) now:
> https://hg.mozilla.org/releases/mozilla-aurora/rev/aa98eb8873a2
> Landed under rs=me l10n=backout_string_change so as to minimize the impact
> of this on localizers per discussion with flod

Can you attach the Aurora specific patch to this bug and request approval for it instead of the m-c patch?

Comment 44

[Francesco Lodolo [:flod]]

(In reply to Lawrence Mandel [:lmandel] from comment #42)
> I see that this was submitted with l10n=lmandel. To be clear, I did not
> grant l10n approval for this bug. l10n approval should only come when we're
> making String changes on Aurora. A failing commit due to the a string change
> should be taken as an indication that something is wrong with the patch that
> needs to be corrected before landing. Is the message from the l10n hook
> clear about the issue and how to proceed?

This is the message currently displayed

> ************************** ERROR ****************************
> * File used for localization (FILENAME) altered in this changeset
> This repository is string frozen. Please request explicit permission from
> release managers to break string freeze in your bug.
> If you have that explicit permission, denote that by including in
> your commit message l10n=...
> *************************************************************

Comment 45

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fully remove the string change from Aurora (already landed as a bustage fix)

Note: applies on top of the existing patch which landed as part of the uplift that simply set the string to ""

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

Note: the rest of the main patch was still needed on Aurora, so that still should be a?

Comment 47

[Randell Jesup [:jesup] (needinfo me)]

The original incorrect removal was https://hg.mozilla.org/releases/mozilla-aurora/rev/701fcc5fcce9:

    1.11  incoming_call_accept_audio_only_tooltip=Accept with voice
    1.12 -incoming_call_accept_audio_video_tooltip=Accept with video
    1.13 +incoming_call_accept_audio_video_tooltip=
    1.14  incoming_call_cancel_button=Cancel

Comment 48

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Francesco Lodolo [:flod] from comment #44)
> (In reply to Lawrence Mandel [:lmandel] from comment #42)
> > I see that this was submitted with l10n=lmandel. To be clear, I did not
> > grant l10n approval for this bug. l10n approval should only come when we're
> > making String changes on Aurora. A failing commit due to the a string change
> > should be taken as an indication that something is wrong with the patch that
> > needs to be corrected before landing. Is the message from the l10n hook
> > clear about the issue and how to proceed?
> 
> This is the message currently displayed
> 
> > ************************** ERROR ****************************
> > * File used for localization (FILENAME) altered in this changeset
> > This repository is string frozen. Please request explicit permission from
> > release managers to break string freeze in your bug.
> > If you have that explicit permission, denote that by including in
> > your commit message l10n=...
> > *************************************************************

Right - I though I had removed the string change (erroneously - I never touch strings normally) and simply didn't think about it enough when I hit this late at night (3am) when finalizing the landing and doing the initial push.  Won't happen again.