Closed Bug 1126321 Opened 5 years ago Closed 5 years ago

Standalone should display both of the remote video and screen when screen sharing is active

Categories

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

defect
Points:
5

Tracking

(firefox38 verified)

VERIFIED FIXED
mozilla38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- verified
Blocking Flags:
backlog Fx38+

People

(Reporter: standard8, Assigned: standard8)

References

()

Details

Attachments

(1 file, 3 obsolete files)

As per the UX, the standalone display should be showing both of the remote screen and video when screen sharing is active.
Priority: -- → P1
Depends on: 1130356
No longer depends on: 1130356
I'm going to take a stab at this.
Assignee: nobody → standard8
backlog: --- → Fx38+
Depends on: 1131688
Depends on: 1132064
This first part renames the existing classes from local-stream and remote-stream to focus-stream and inset-stream. I'm still not quite sure about the focus stream name.

The elements still have .local and .remote on them as names. I'd prefer not to have things such as local-inset-stream and remote-inset-stream as that feels like duplicating classnames for no particular reason with our current UI.

Still working on this and part 2, so not requesting review yet, but feedback welcome.
Attachment #8563392 - Flags: feedback?(mdeboer)
Draft of part 2 adding the "sub-inset" stream. It doesn't work properly yet as I need to do some more adjustments to the calculations and decide how we're going to cope with the sdk's minimum stream size values.

However I think the structure is right now, so requesting feedback on that before I finish it off.
Attachment #8563395 - Flags: feedback?(mdeboer)
Comment on attachment 8563392 [details] [diff] [review]
Part 1. Redefine the classnames for remote and local streams to focus and inset to reflect the real display better.

Ok, I'm currently discussing a different method with Sevaan due to the local video being extremely small in the designed set-up. Hence cancelling feedback for now.
Attachment #8563392 - Flags: feedback?(mdeboer)
Attachment #8563395 - Flags: feedback?(mdeboer)
Depends on: 1132882
New version that works for the wider screens only (wider than 640px). To do smaller is going to be harder as it involves a lot more calculations as the DOM could do with being set up differently. I'm going to split that out to a separate bug.

The remote and local video are stacked here - as per my discussions with Sevaan; doing a sub-inset makes it a bit too small to be useable/visable.

I'll finish up the tests on Monday.
Attachment #8563392 - Attachment is obsolete: true
Attachment #8563395 - Attachment is obsolete: true
Updated with unit tests and to fix a couple of minor issues.
Attachment #8564242 - Attachment is obsolete: true
Attachment #8564570 - Flags: review?(mdeboer)
Iteration: --- → 38.3 - 23 Feb
Flags: qe-verify+
Comment on attachment 8564570 [details] [diff] [review]
Loop Standalone should display both of the remote video and screen when screen sharing is active.

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

Nice one Mark! It seems to work fine when I tested it, I didn't see any incorrect behavior.

You mentioned that you'll be making the inset-view smaller in a follow-up, right? With that bug filed and comments addressed, r=me.

::: browser/components/loop/content/shared/js/mixins.js
@@ +276,5 @@
>       * Note: Once we support multiple remote video streams, this function will
>       *       need to be updated.
> +     *
> +     * @param {string} videoType The video type according to the sdk, e.g. "camera" or
> +     *                           "stream".

ITYM 'screen'

@@ +329,5 @@
>              remoteVideoDimensions.streamHeight = leadingAxis === "width" ?
>                remoteVideoDimensions.height: leadingAxisSize;
>            }
>          }
> +      };

please remove this semicolon.

::: browser/components/loop/test/shared/mixins_test.js
@@ +267,5 @@
>  
>        it("should fetch the correct stream sizes for leading axis width and full",
>          function() {
>            remoteVideoDimensions = {
> +            stream: {

ITYM 'screen' throughout this file...

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +258,5 @@
> +          style: {}
> +        };
> +        remoteElement = {
> +          style: {},
> +          removeAttribute: sinon.spy()

I think it'd be nice to check if this function gets called indeed, for extra coverage.
Attachment #8564570 - Flags: review?(mdeboer) → review+
Blocks: 1133534
(In reply to Mike de Boer [:mikedeboer] from comment #7)
> You mentioned that you'll be making the inset-view smaller in a follow-up,
> right? With that bug filed and comments addressed, r=me.

I just filed bug 1133532 and bug 1133534 as follow-ups.
https://hg.mozilla.org/integration/fx-team/rev/1d9dcfdb7c08
Target Milestone: --- → mozilla38
https://hg.mozilla.org/mozilla-central/rev/1d9dcfdb7c08
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
QA Contact: bogdan.maris
Verified as fixed using Firefox 38 beta 8 (after enabling the feature) on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.