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

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Bug 1171969 may end up being relevant here or having some overlap.

Comment 3

[:shell escalante]

find spinner bug with assets or ask michael

Comment 4

[:shell escalante]

here is the original spinner from Michael bug 1077592 - sevaan is looking at

Comment 5

[Mark Banner (:standard8)]

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

Comment 6

[Sevaan Franks [:sevaan]]

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

Comment 7

[Andrei Oprea [:andreio]]

I am going forward with the first solution from Mark.

Comment 8

[Andrei Oprea [:andreio]]

Attachment: Bug 1171933: Add spinner indicator when waiting for video

Comment 9

[Andrei Oprea [:andreio]]

Attachment: Bug 1171933: Add spinner indicator when waiting for video

Comment 10

[Dan Mosedale (:dmosedale, :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.

Comment 11

[Andrei Oprea [:andreio]]

Attachment: Bug 1171933: Add loading spinner when waiting for video

Comment 12

[Andrei Oprea [:andreio]]

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.

Comment 13

[Dan Mosedale (:dmosedale, :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/[e-mail]                (content/shared/img/[e-mail])

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.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Reimplement spinners that Hello lost after markdown extraction, r=dmose

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

Comment 15

[Pulsebot]

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

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Andrei, can you please file a spin-off bug (and CC me) to implement this for direct calling as well?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25d7035e9fcd

Comment 18

[Andrei Oprea [:andreio]]

Here is the new bug for direct call https://bugzilla.mozilla.org/show_bug.cgi?id=1176162.