Closed Bug 1171933 Opened 4 years ago Closed 4 years ago

Re-implement spinners (pending notifications) when waiting for video/audio to be connected/displayed

Categories

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

defect

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Iteration:
41.3 - Jun 29
Tracking Status
firefox41 --- fixed

People

(Reporter: standard8, Assigned: andreio)

References

Details

(Whiteboard: [ux bug, spinners])

User Story

As a user, I should see visual feedback when audio, screen, or video transmission is on its way, but hasn't started playing/rendering yet.

Notes:

- Up until the markup extraction, the SDK did this for us as spinners.  It appears to use a single spinner in each incoming visual media space. I _think_ the audio data is (effectively, if not actually) lumped in to the video spinner (eg in our current SDK-based deployedment, if a call is started with audio but no video, there is a spinner in the video space which is replaced after a few seconds by the avatar).

- When we get notifications of new remote streams, before subscribing to them, send a notification to the store. This will mark the start of the in-progress period. When subscription is complete, send another notification.

- First step is probably to TDD splitting the receivingScreenShare action into two actions in otSdkDriver.js and activeRoomStore.js.  Figure out exactly how we want to split them may take a bit of thinking.

- Screenshare currently has the first notification disabled due to layout issues. Bug 1168829 may help with this.  ReceivingScreenShare will likely need to split into two actions to do this so that we can tell the difference between "screen share being transmitted but not here yet" and "screen share video available".

Attachments

(1 file, 3 obsolete files)

In bug 1141296, we temporarily dropped the pending notification spinners that were previously supplied by the sdk. We need to add these back in, so that users have a better idea of what is happening.

There's a few things that we might want to do, see the user story for more details.
Flags: firefox-backlog+
Priority: -- → P1
Sevaan: this bug currently describes re-implementing the SDK's visual notifications for pending streams.  Would you prefer that we implement something else instead?  If so, now would be a great time to suggest that... :-)
User Story: (updated)
Flags: needinfo?(sfranks)
Bug 1171969 may end up being relevant here or having some overlap.
See Also: → 1171969
User Story: (updated)
find spinner bug with assets or ask michael
Rank: 18
Flags: needinfo?(sfranks) → needinfo?(sescalante)
Whiteboard: [ux bug]
Flags: needinfo?(sescalante)
Summary: Re-implement pending notifications when waiting for video/audio to be connected/displayed → Re-implement spinners (pending notifications) when waiting for video/audio to be connected/displayed
Whiteboard: [ux bug] → [ux bug, spinners]
here is the original spinner from Michael bug 1077592 - sevaan is looking at
Flags: needinfo?(sfranks)
I think what needs to be done, is to extend the current media-view, with a "displayPending" option.

It should probably be rendered in a similar way as to the avatar, as just a div (or more, as needed).

For local video, we can key the pending display off of the room state - so MEDIA_WAIT is when we start displaying it, and we stop displaying when we've actually got the localSrcVideoObject.

For remote video, we'll probably need to do similar, but this time HAS_PARTICIPANTS is the place to key it from.


The alternative to the above, would be to more directly key it to the actions the sdk sends out (via the store, of course). This would mean reworking the RemoteVideoEnabled/RemoteVideoDisabled/LocalVideoEnabled actions to potentially be multiple actions, i.e. "RemoteVideoIncoming", "RemoteVideoReceived", "RemoteVideoMuted" and the same for local.

This would be more extensive work, that we probably want to split out to a separate bug, and to do it later (I think we should do it at some stage, just not necessarily now).
(In reply to :shell escalante from comment #4)
> here is the original spinner from Michael bug 1077592 - sevaan is looking at

This spinner works fine for now.
Flags: needinfo?(sfranks)
I am going forward with the first solution from Mark.
Assignee: nobody → andrei.br92
Attachment #8624287 - Flags: review?(dmose)
Attachment #8624287 - Attachment is obsolete: true
Attachment #8624287 - Flags: review?(dmose)
Attachment #8624455 - Flags: review?(dmose)
Comment on attachment 8624455 [details] [diff] [review]
Bug 1171933: Add spinner indicator when waiting for video

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

As we've discussed F2F, somehow the animation appears to be shifting slightly as it rotates, so that will want to be fixed before we land.

We also don't want to commit gecko.log.  :-)

Please file a spin-off bug to add the spinners to direct calls.

Looking good.

::: browser/components/loop/content/js/roomViews.jsx
@@ +675,5 @@
>            return true;
>        }
>      },
>  
> +    shouldRenderLoading: function() {

Same comment as shouldRenderLoading in standalone.jsx.

::: browser/components/loop/content/shared/css/conversation.css
@@ +553,5 @@
>    width: 100%;
>  }
>  
> +.loading-stream {
> +  background-image: url("../img/spinner.svg");

I'll review this on the next iteration of the patch, since it will have changed.

::: browser/components/loop/content/shared/js/actions.js
@@ +289,5 @@
>      /**
>       * Used to notify that a shared screen is being received (or not).
>       *
>       * XXX this is going to need to be split into two actions so when
> +     * can display a spinner when the screen share is pending

Maybe change the above to read:

XXX this should be split into multiple actions to make the code clearer.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +612,5 @@
>      /**
>       * Used to note the current state of receiving screenshare data.
>       *
>       * XXX this is going to need to be split into two actions so when
> +     * can display a spinner when the screen share is pending

Same suggestion as actions.js.

::: browser/components/loop/content/shared/js/views.jsx
@@ +864,5 @@
>        videoElement.play();
>      },
>  
>      render: function() {
> +      if (this.props.isLoading) {

Need to add this to the propTypes, as you pointed out.

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +353,5 @@
>  
>        }
>      },
>  
> +    shouldRenderLoading: function() {

I think this wants to be three separate functions; this is a pretty unusual API, and these things aren't really related.

Each one wants its own JSDoc, and some or all of them may want commentary in the function or JSDoc describing why they work the way they do.

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +255,5 @@
> +        it("should not show loading screen if receivingScreenShare is false ",
> +           function() {
> +             view.setState({
> +               "receivingScreenShare": false,
> +               "screenShareVideoObject": null

Maybe set screenShareVideoObject to something here, since this would test an invocation that is probably more likely susceptible to bugs.

::: browser/components/loop/ui/ui-showcase.css
@@ +10,5 @@
>  }
>  
> +/*
> + * Some situations where this is needed. Explained in the code
> + */

Probably no need for a comment here.  The code itself is pretty clear.
Attachment #8624455 - Flags: review?(dmose) → review-
Attachment #8624455 - Attachment is obsolete: true
Comment on attachment 8624550 [details] [diff] [review]
Bug 1171933: Add loading spinner when waiting for video

Tested some more and it turns out that I could not remove absolute positioning. The spinner still broke when you resized the window.
Attachment #8624550 - Flags: review?(dmose)
Comment on attachment 8624550 [details] [diff] [review]
Bug 1171933: Add loading spinner when waiting for video

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

Looks good.  r=dmose with the commented changes.

::: browser/components/loop/content/js/roomViews.jsx
@@ +686,5 @@
> +        remote: this.state.roomState === ROOM_STATES.HAS_PARTICIPANTS &&
> +                !this.state.remoteSrcVideoObject && !this.state.mediaConnected
> +      };
> +    },
> +

This still needs to be split into separate functions and documented.

::: browser/components/loop/content/shared/css/conversation.css
@@ +575,5 @@
> +  background-repeat: no-repeat;
> +  background-size: 40%;
> +  /* spinner rotation. 12 is the number of lines in the spinner */
> +  animation: rotate-spinner 1s steps(12, end) infinite;
> +}

A bit of vertical white-space between the sections here would make it notably more readable.

::: browser/components/loop/content/shared/js/views.jsx
@@ +690,5 @@
>    });
>  
> +  /*
> +   * Renders a loading spinner for when video content is not yet available.
> +   */

Should be in JSDoc format.

::: browser/components/loop/jar.mn
@@ +40,1 @@
>    content/browser/loop/shared/img/spinner@2x.png                (content/shared/img/spinner@2x.png)

Probably worth adding an XXX comment here to check whether the two .pngs could be gotten rid of in favor of the svg.

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +365,5 @@
> +                !this.state.remoteSrcVideoObject && !this.state.mediaConnected,
> +        screenShare: this.state.receivingScreenShare &&
> +                     !this.state.screenShareVideoObject
> +      };
> +    },

This still needs to be split out into 3 functions with appropriate commentary.

::: browser/components/loop/test/desktop-local/roomViews_test.js
@@ +527,5 @@
>            TestUtils.findRenderedComponentWithType(view,
>              loop.shared.views.FeedbackView);
>          });
>  
> +      it("should display loading spinner when remote local is not available",

I think this wants to read "when localSrcVideoObject is not..."

::: browser/components/loop/ui/ui-showcase.jsx
@@ +226,5 @@
> +  loadingRemoteLoadingScreenStore.receivingScreenShare({
> +    receiving: true,
> +    srcVideoObject: false
> +  });
> +

A bit of vertical white space separating these things would help readability.

@@ +909,5 @@
>  
>              <FramedExample width={644} height={483} dashed={true}
> +              summary="Standalone room conversation (loading remote)"
> +              cssClass="standalone"
> +              onContentsRendered={joinedRoomStore.forcedUpdate}>

This looks like it's pointing to the wrong forcedUpdate.

@@ +964,5 @@
>              </FramedExample>
>  
>              <FramedExample width={800} height={660} dashed={true}
>                             cssClass="standalone"
>                             onContentsRendered={updatingSharingRoomStore.forcedUpdate}

Wrong forcedUpdate here.

@@ +983,5 @@
> +            </FramedExample>
> +
> +            <FramedExample width={800} height={660} dashed={true}
> +                           cssClass="standalone"
> +                           onContentsRendered={updatingSharingRoomStore.forcedUpdate}

Wrong forcedUpdate here too.  This isn't really your fault, these APIs need to be DRYed up.
Attachment #8624550 - Flags: review?(dmose) → review+
This addresses all the review comments.  It also adds some markup to ensure
that the spinner background is always black (it had been white in the
DesktopRoomConversationView).
Attachment #8624550 - Attachment is obsolete: true
Attachment #8624609 - Flags: review+
Andrei, can you please file a spin-off bug (and CC me) to implement this for direct calling as well?
https://hg.mozilla.org/mozilla-central/rev/25d7035e9fcd
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Here is the new bug for direct call https://bugzilla.mozilla.org/show_bug.cgi?id=1176162.
Iteration: --- → 41.3 - Jun 29
You need to log in before you can comment on or make changes to this bug.