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

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.

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.

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

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 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 :)

(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.

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

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 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.

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

(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.

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.

Added in the extra string needed.

Comment on attachment 8485928 [details] [diff] [review] Implement default answering mode for desktop client Transfering review to :mikedeboer as per IRC discussion.

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 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 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'.

Thanks Andrei!

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

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

(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.

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

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/rjesup@wgate.com-f9eb2cbac352

(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/rjesup@wgate.com- > f9eb2cbac352 Verified fixed Win 7, OS X 10.8.5, Ubuntu 13.04

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

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

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.

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.

(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

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

(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.

(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?

(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=... > *************************************************************

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

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

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

(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.