content: Handle video previews #587

rajveermalviya · 2024-03-24T21:59:20Z

Implement thumbnail image based video previews for Youtube & Vimeo
video links, and inline video player preview using the video_player
package for user uploaded videos.

For user uploaded video, current implementation will fetch the metadata
when the message containing the video comes into view. This metadata
is used by the player to determine if video is supported on the device.
If it isn't then there will be no preview, and tapping on the play
button will open the video externally. If the video is supported, then
the first frame of the video will be presented as a preview in the
message container while tapping on the play button will start buffering
and playing the video in the lightbox.

There are still some quirks with the current implementation:

On iOS/macOS, there is a bug where whole video is downloaded before
playing: [video_player] iOS seems to downloads entire file before playing flutter/flutter#126760
On iOS/macOS, unlike on Android the first frame is not shown after
initialization: First Frame of Video Once VideoPlayer Controller doesn't show in iOS Safari/Chrome flutter/flutter#139107
Current implementation uses url_launcher for fallback in case video
is not supported by video_player, we should switch to webview
instead to correctly handle auth headers for private videos.

Fixes: #356

video-content.mp4

gnprice

Interesting! Looking forward to this feature.

This will need tests, naturally. I'm guessing that's the main reason you've marked it as draft.

I'll wait to do a detailed review until you've said the PR is ready for one. But from a quick skim just now, here's a few high-level points.

gnprice · 2024-03-24T23:07:58Z

pubspec.yaml

@@ -65,6 +65,7 @@ dependencies:
  url_launcher: ^6.1.11
  url_launcher_android: ">=6.1.0"
  sqlite3: ^2.4.0
+  video_player: ^2.8.3


The dependency should be added in its own commit. See git log --stat -p --full-diff pubspec.* for examples.

gnprice · 2024-03-24T23:11:38Z

macos/Podfile.lock

-COCOAPODS: 1.13.0
+COCOAPODS: 1.15.0


This CocoaPods bump is fine, but should be its own commit because it's logically independent of the other things happening. See git log --stat -p -G COCOAPODS for past examples.

I suspect that on your machine if you just do flutter run for iOS and macOS targets, starting from main, it'll already make these two changes.

gnprice · 2024-03-24T23:15:38Z

lib/model/content.dart

@@ -1024,6 +1083,10 @@ class _ZulipContentParser {
      return parseImageNode(element);
    }

+    if (localName == 'div' && classes.containsAll(['message_inline_image', 'message_inline_video'])) {


We'll want to use className rather than classes; we made that switch earlier this year. See git log -p -S classes lib/model/content.dart for examples and motivation.

This one will be a bit nontrivial (because there's more than one class), but see the latest couple of commits matched by that filter for examples of similar complexity.

rajveermalviya · 2024-03-26T01:49:41Z

Thanks for the initial comments @gnprice! It should be ready for the first round of review.

For the two iOS bugs, we could just move the initialization into the lightbox on iOS and don't care about first frame preview, but then we loose the ability to check if video is supported or not when user taps on play, if that happens when already in the lightbox then we will have to show a dialog that video's unsupported with a button to open it in webview, pretty weird UX.

Or just always open video in webview on iOS.

Or ofcourse get these bugs fixed, & live with them temporarily.

gnprice

Thanks @rajveermalviya! I've read through the parsing commit in detail, and it generally looks good; comments below. The two deps commits also look good. I haven't yet read through the widgets/playback commit.

Those two upstream issues are interesting. Are you observing their symptoms?

In issue

[video_player] iOS seems to downloads entire file before playing flutter/flutter#126760

it kind of sounds like flutter/packages#3826 may have fixed the issue last July. The original reporter (who had a Vimeo example) didn't reply after that. There's one person who showed up in January reporting similar symptoms, but it's not clear that their issue wasn't some specific interaction with the CDN they were using.

In issue

First Frame of Video Once VideoPlayer Controller doesn't show in iOS Safari/Chrome flutter/flutter#139107

there are several reports that things work fine in a Flutter iOS app and that the only trouble is in Flutter web on iOS browsers, and the issue is tagged that way. The thread is a bit scattered, as there are also reports of trouble on Flutter iOS.

gnprice · 2024-03-26T18:25:19Z

lib/model/content.dart

+  const VideoNode({
+    super.debugHtmlNode,
+    required this.srcUrl,
+    this.previewImageUrl,


Suggested change

this.previewImageUrl,

required this.previewImageUrl,

IOW let's treat this the same as our API types in lib/api/ as described at https://github.com/zulip/zulip-flutter#editing-api-types :

In our API types, constructors should generally avoid default values for their parameters, even null. This means writing e.g. required this.foo rather than just this.foo, even when foo is nullable. This is because it's common in the Zulip API for a null or missing value to be quite salient in meaning, and not a boring value appropriate for a default, so that it's best to ensure callers make an explicit choice. If passing explicit values in tests is cumbersome, a factory function in test/example_data.dart is an appropriate way to share defaults.

gnprice · 2024-03-26T18:28:36Z

lib/model/content.dart

+    const sourceType = r"(message_inline_video)?(youtube-video)?(embed-video)?";
+    return RegExp("^message_inline_image $sourceType|$sourceType message_inline_image\$");


This will match e.g. message_inline_image youtube-videoembed-video, which seems unintended.

Try the regexp "alternation" operator | instead:

Suggested change

const sourceType = r"(message_inline_video)?(youtube-video)?(embed-video)?";

return RegExp("^message_inline_image $sourceType|$sourceType message_inline_image\$");

const sourceType = r"(message_inline_video|youtube-video|embed-video)";

return RegExp("^message_inline_image $sourceType|$sourceType message_inline_image\$");

gnprice · 2024-03-26T18:29:23Z

lib/model/content.dart

+  bool _isVideoClassNameInlineVideo(String input) {
+    if (!_videoClassNameRegexp.hasMatch(input)) return false;
+    final groups = _videoClassNameRegexp.firstMatch(input)!.groups([1, 4]);
+    return groups.nonNulls.firstOrNull == 'message_inline_video';


I think this logic will get simpler with the (a|b|c) rather than (a)?(b)?(c)? regexp, too.

gnprice · 2024-03-26T18:31:42Z

lib/model/content.dart

+  bool _isVideoClassNameEmbedVideo(String input) {
+    if (!_videoClassNameRegexp.hasMatch(input)) return false;
+    final groups = _videoClassNameRegexp.firstMatch(input)!.groups([3, 6]);
+    return groups.nonNulls.firstOrNull == 'embed-video';
+  }
+
  InlineContentNode parseInlineContent(dom.Node node) {


nit: The existing regexps above this are here because they're helpers for parseInlineContent. These new members are helpers instead for parseVideoNode and its friends, so let's put them just above there.

gnprice · 2024-03-26T18:35:10Z

lib/model/content.dart

+      return _parseInlineVideo(divElement);
+    }
+    if (_isVideoClassNameYoutubeVideo(divElement.className)
+      || _isVideoClassNameEmbedVideo(divElement.className)) {
+      return _parseEmbedVideoWithPreviewImage(divElement);
+    }
+    return UnimplementedBlockContentNode(htmlNode: divElement);
+  }
+
+  BlockContentNode _parseInlineVideo(dom.Element divElement) {


nit: In this parsing code, the ordering is that specific pieces and building blocks go above, and pieces that are more general or sit higher in the ContentNode tree go below. Here _parseInlineVideo is a more-specific helper for parseVideoNode, so it should go above it.

gnprice · 2024-03-26T18:47:18Z

test/model/content_test.dart

+
+  static const videoEmbedYoutubeClassNameReordered = ContentExample(
+    'video preview for youtube embed with thumbnail, (hypothetical) class name reorder',
+    null,


(Fine to have hypothetical variations like this one, though.)

Oh but let's maintain a convention of, when this markdown field is null, having a comment right at this line that explains why. That helps maintain a practice of consistently including the Markdown source when there isn't a good reason we can't.

Fine for the comment to be terse, when the reason is a recurring straightforward one like here. See e.g. emojiUnicodeClassesFlipped.

gnprice · 2024-03-26T18:52:33Z

test/model/content_test.dart

+    ),
+  ]);
+
+  static const videoEmbedYoutubeClassNameReordered = ContentExample(


nit: this name is fine intrinsically, but it's useful to express the same idea consistently between different names that belong to the same list — so, following the existing emojiUnicodeClassesFlipped:

Suggested change

static const videoEmbedYoutubeClassNameReordered = ContentExample(

static const videoEmbedYoutubeClassesFlipped = ContentExample(

gnprice · 2024-03-26T18:57:02Z

test/model/content_test.dart

+  static const videoEmbedVimeoPreviewDisabled = ContentExample(
+    'video preview for vimeo embed with realm link previews disabled',
+    null,
+    '<p>'
+      '<a href="https://vimeo.com/1084537">https://vimeo.com/1084537</a></p>', [


This doesn't actually embed any video, right? It's just a link.

Fine to have it here as an extra example — could be helpful for someone trying to reproduce the videoEmbedVimeoPreviewEnabled case below, if the server they're using has realm link previews disabled. But let's make the story clear in the description. For example:

Suggested change

static const videoEmbedVimeoPreviewDisabled = ContentExample(

'video preview for vimeo embed with realm link previews disabled',

null,

'<p>'

'<a href="https://vimeo.com/1084537">https://vimeo.com/1084537</a></p>', [

static const videoEmbedVimeoPreviewDisabled = ContentExample(

'video non-preview for attempted vimeo embed with realm link previews disabled',

null,

'<p>'

'<a href="https://vimeo.com/1084537">https://vimeo.com/1084537</a></p>', [

gnprice · 2024-03-26T19:12:16Z

test/model/content_test.dart

+      '<a href="https://vimeo.com/1084537">Vimeo - Big Buck Bunny</a>'
+    '</p>\n'
+    '<div class="embed-video message_inline_image">'
+      '<a data-id="&lt;iframe src=&quot;https://player.vimeo.com/video/1084537?app_id=122963&quot; width=&quot;640&quot; height=&quot;360&quot; frameborder=&quot;0&quot; allow=&quot;autoplay; fullscreen; picture-in-picture; clipboard-write&quot; title=&quot;Big Buck Bunny&quot;&gt;&lt;/iframe&gt;" href="https://vimeo.com/1084537" title="Big Buck Bunny">'


The value of the data-id attribute is a blob of HTML? For an iframe element? Wacky. That's not much of an "ID".

If the Zulip server produces this, or has produced it in the past, then it's good to have in a test case. But it smells rather like a bug. It'd be good to report it in a chat thread in #issues and see what people say.

This has enough resemblance to a potential HTML injection that I waffled on whether to treat it as a potential security issue (which should be sent to security@zulip.com rather than discussed in public). So I poked around in the server implementation, and the logic for this is pretty explicit in zerver/lib/markdown/__init__.py:

elif extracted_data.type == "video": self.add_a( # … class_attr="embed-video message_inline_image", data_id=extracted_data.html,

That makes it look intentional enough that I'm OK with keeping the discussion public. But I'd still be interested in what the story is with calling this "id" when it's actually some HTML for an iframe.

gnprice · 2024-03-26T19:13:55Z

test/model/content_test.dart

+    ]),
+  ]);
+
+  static const videoEmbedVimeoPreviewEnabled = ContentExample(


Suggested change

static const videoEmbedVimeoPreviewEnabled = ContentExample(

static const videoEmbedVimeo = ContentExample(

Given that the preview-disabled case isn't really a "video embed" at all, we can treat this as the main Vimeo example and give it a shorter name.

rajveermalviya · 2024-03-26T22:12:35Z

Thanks @gnprice for the review! Rebased with comments addressed.

Are you observing their symptoms?

Yes I am able to reproduce them atleast on macOS and iOS Simulator, but haven't tested if this could be a CDN issue. I will test it using bare AVPlayer to see if it really is something to fix in Zulip's CDN config.

rajveermalviya · 2024-03-28T18:50:56Z

(rebased to main)

gnprice

Thanks @rajveermalviya for the revision! Here's a fresh round of review on the parsing commit.

I'll aim to get to the widgets/playback commit soon, too; but there's an unusually large rate of PRs coming in right now, so I'll cycle through some of the others first.

gnprice · 2024-03-28T19:31:58Z

lib/model/content.dart

+    assert(divElement.localName == 'div'
+      && _videoClassNameRegexp.hasMatch(divElement.className));
+
+    final match = _videoClassNameRegexp.firstMatch(divElement.className)!;


This will still mean running the regexp a second time, after doing so once in the condition at the call site. Let's get it down to just once (at least outside of asserts).

See above: #587 (comment)

gnprice · 2024-03-28T19:37:45Z

lib/model/content.dart

+    const sourceType = r"(message_inline_video|youtube-video|embed-video)?";
+    return RegExp("^message_inline_image $sourceType|$sourceType message_inline_image\$");


Suggested change

const sourceType = r"(message_inline_video|youtube-video|embed-video)?";

return RegExp("^message_inline_image $sourceType|$sourceType message_inline_image\$");

const sourceType = r"(message_inline_video|youtube-video|embed-video)";

return RegExp("^message_inline_image $sourceType|$sourceType message_inline_image\$");

Otherwise this matches message_inline_image and message_inline_image, which I don't think is intended.

gnprice · 2024-03-28T19:40:57Z

lib/model/content.dart

+    return switch (match.groups([1, 2])) {
+      ['message_inline_video', null] || [null, 'message_inline_video']


I don't love groups here because it's allocating a list, and there isn't fundamentally a need for a list because we know there should be exactly one of these groups that matched.

In particular, after removing the ? as in my comment above, it's an invariant of the regexp that if the regexp as a whole has a match, then it will always involve exactly one of those groups matching.

So we can use that to write:

Suggested change

return switch (match.groups([1, 2])) {

['message_inline_video', null] || [null, 'message_inline_video']

final videoClass = match.group(1) ?? match.group(2)!;

return switch (videoClass) {

'message_inline_video'

gnprice · 2024-03-28T19:43:08Z

lib/model/content.dart

+        => _parseInlineVideo(divElement),
+      ['youtube-video' || 'embed-video', null] || [null, 'youtube-video' || 'embed-video']
+        => _parseEmbedVideoWithPreviewImage(divElement),
+      _ => UnimplementedBlockContentNode(htmlNode: divElement),


Similarly, it's an invariant of the regexp that this case can't happen. So we can add an assertion to that effect.

gnprice · 2024-03-28T19:50:10Z

lib/model/content.dart

@@ -948,6 +976,90 @@ class _ZulipContentParser {
    return ImageNode(srcUrl: src, debugHtmlNode: debugHtmlNode);
  }

+  static final _videoClassNameRegexp = () {


nit: can move this down to just above parseVideoNode, past _parseInlineVideo and its sibling, since it's really a helper for the former and it isn't used by the latter

gnprice · 2024-03-28T19:52:16Z

test/model/content_test.dart

+  static const videoEmbedYoutubeClassesFlipped = ContentExample(
+    'video preview for youtube embed with thumbnail, (hypothetical) class name reorder',
+    'https://www.youtube.com/watch?v=aqz-KE-bpKQ',


(continuing from #587 (comment) )

This isn't the server's real output for this Markdown source, is it? Rather the output above is — that's why this one is hypothetical.

Again, see emojiUnicodeClassesFlipped for an example:

static final emojiUnicodeClassesFlipped = ContentExample.inline( 'Unicode emoji, encoded in span element, class order reversed', null, // ":thumbs_up:" (hypothetical server variation)

gnprice · 2024-03-28T19:58:44Z

test/model/content_test.dart

+    // Note that the server generates "data-id" attribute with value of a raw HTML,
+    // web client uses this to show Vimeo's video player in a sandbox iframe.
+    // The HTML blob is provided by Vimeo and them changing it wouldn't break the
+    // web client because it relies on this dynamic value.
+    //
+    // Discussion:
+    //  https://chat.zulip.org/#narrow/stream/9-issues/topic/Vimeo.20link.20previews.20HTML.20.22data-id.22.20isn't.20a.20.20Vimeo.20video.20ID/near/1767563


Thanks for writing this down! Here's a slightly tightened-up version (plus the explicit warning that we shouldn't try to parse this attribute, even though web's use of it is OK):

Suggested change

// Note that the server generates "data-id" attribute with value of a raw HTML,

// web client uses this to show Vimeo's video player in a sandbox iframe.

// The HTML blob is provided by Vimeo and them changing it wouldn't break the

// web client because it relies on this dynamic value.

//

// Discussion:

// https://chat.zulip.org/#narrow/stream/9-issues/topic/Vimeo.20link.20previews.20HTML.20.22data-id.22.20isn't.20a.20.20Vimeo.20video.20ID/near/1767563

// The server really does generate an attribute called "data-id" whose value

// is a blob of HTML. The web client uses this to show Vimeo's video player

// inside a sandbox iframe. The HTML comes from Vimeo and may change form;

// that's OK the way web uses it, but we shouldn't try to parse it. See:

// https://chat.zulip.org/#narrow/stream/9-issues/topic/Vimeo.20link.20previews.20HTML.20.22data-id.22.20isn't.20a.20.20Vimeo.20video.20ID/near/1767563

On "note that", see: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-empty-prose

rajveermalviya · 2024-03-29T22:18:49Z

@gnprice thanks for the review! Pushed a new revision.

gnprice

Thanks! Just one comment on the parsing commit in this revision.

gnprice · 2024-04-01T21:45:57Z

lib/model/content.dart

+    {
+      final match = _videoClassNameRegexp.firstMatch(className);
+      if (localName == 'div' && match != null) {
+        return parseVideoNode(element, match);


Continuing on the theme of staying paranoid about performance within this parsing code: this version now runs the regexp before checking the cheap localName condition. I'd like to keep the more expensive condition saved for after we know the other condition passes.

There's a few ways that could be arranged in the code. Here's one:

Suggested change

{

final match = _videoClassNameRegexp.firstMatch(className);

if (localName == 'div' && match != null) {

return parseVideoNode(element, match);

final RegExpMatch? videoMatch;

if (localName == 'div'

&& ((videoMatch = _videoClassNameRegexp.firstMatch(className)) != null)) {

return parseVideoNode(element, videoMatch!);

gnprice

OK, and here's a first review of the widgets/playback code!

I haven't yet looked at the lightbox, or at most of the stateful logic of MessageInlineVideoPreview. But I think this is enough for one round. There are a couple of aspects that would be good to restructure, which may cause some changes in the other code anyway.

gnprice · 2024-04-01T22:42:29Z

lib/widgets/content.dart

          return MessageImage(node: node);
+        } else if (node is VideoNode) {
+          return MessageVideo(node: node);


nit: this case gets added in one commit as a stub, then moved around in a later commit; instead when first added it should go in the place we intend to leave it, so the later commit doesn't have to move it.

Relatedly: in the intermediate state after the first commit and before the second, instead of a Container (which is likely invisible) we should show some version of the "unimplemented" display, since at that stage this feature is still unimplemented. This is part of the general principle that each commit should be coherent on its own:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#each-commit-must-be-coherent

Doesn't need to be particularly well abstracted, since it is temporary. In particular it needn't find a way to call _errorUnimplemented, and instead can just use errorStyle directly to make a Text widget that looks similar to something _errorUnimplemented might do.

gnprice · 2024-04-01T22:54:30Z

lib/widgets/content.dart

+    final resolvedSrc = store.tryResolveUrl(node.srcUrl);
+    return resolvedSrc != null
+      ? MessageInlineVideoPreview(src: resolvedSrc)
+      : Container();


This Container seems a bit too quiet if something's wrong — it could make it easy to not realize anything was supposed to be there in the first place.

In MessageImage if we can't resolve the URL I believe we end up showing a 150x100 rectangle that's just blank. That can be improved (as the comment there says), but would be adequate here.

gnprice · 2024-04-01T23:04:45Z

lib/widgets/content.dart

+      child: UnconstrainedBox(
+        alignment: Alignment.centerLeft,
+        child: Padding(
+          // TODO clean up this padding by imitating web less precisely;
+          //   in particular, avoid adding loose whitespace at end of message.
+          padding: const EdgeInsets.only(right: 5, bottom: 5),
+          child: LightboxHero(
+            message: message,
+            src: widget.src,
+            child: Container(
+              height: 100,
+              width: 150,
+              color: Colors.black,


There's a lot of details here that seem like they get duplicated into three places, namely MessageImage and the two video types:

a GestureDetector

contains an UnconstrainedBox aligned centerLeft

contains certain 5px padding, with a TODO comment

then there's a certain 150x100 shape.

It'd be great to abstract some of those so that they can be shared. I think it's not a coincidence these are similar — they're designed to look similar — so it's good to arrange the code in a way that will help them naturally remain similar as we make changes.

gnprice · 2024-04-01T23:06:16Z

lib/widgets/content.dart

+                    AspectRatio(
+                      aspectRatio: _controller!.value.aspectRatio,
+                      child: VideoPlayer(_controller!)),
+                    Container(color: const Color.fromRGBO(0, 0, 0, 0.30)),


Huh, if I'm reading this right, it puts a 30%-opacity black layer in front of the video (by putting it later in a Stack's children).

Is that intended? It doesn't seem like what we'd want.

gnprice · 2024-04-01T23:10:48Z

lib/widgets/content.dart

+                    const Icon(
+                      Icons.play_arrow_rounded,
+                      color: Colors.white,
+                      size: 25)


How did you choose 25px for the size?

It appears to be 32px on web and in zulip-mobile. (For the latter, see src/webview/static/base.css.)

gnprice · 2024-04-01T23:12:39Z

lib/widgets/content.dart

+    // For YouTube and Vimeo links, display a widget with a thumbnail.
+    // When the thumbnail is tapped, open the video link in an external browser or
+    // a supported external app.
+    if (node.previewImageUrl != null) {
+      return MessageEmbedVideoPreview(


The information in this comment would be good to turn into a short dartdoc for the MessageEmbedVideoPreview class.

gnprice · 2024-04-01T23:14:56Z

lib/widgets/content.dart

+            child: Stack(
+              alignment: Alignment.center,
+              children: [
+                if (previewImageUrl != null) ...[


Similar comment as on the Container fallback for the node.srcUrl case above.

gnprice · 2024-04-01T23:40:00Z

lib/widgets/content.dart

+class MessageEmbedVideoPreview extends StatelessWidget {
+  const MessageEmbedVideoPreview({
+    super.key,
+    required this.previewImage,
+    required this.src,


Looking at these two widget classes for the two kinds of videos, and at MessageVideo which dispatches to them, I think it'd probably be best to have two different ContentNode subclasses for them.

In particular there's a very important difference in semantics between these two, if I'm correctly understanding how these work:

For "inline" videos, node.srcUrl is a URL on the Zulip server, and so it's fine to load it when the user just scrolls passively past that message (and doing so is helpful for the reasons you mention in the PR description).

For "embed" videos, node.srcUrl is a URL on some other external service. For privacy reasons it's therefore important that we not go and talk to that service unless the user actually chooses to interact with the video. OTOH node.previewImageUrl is a URL back on the user's Zulip server (provided by Camo), and safe to eagerly fetch from.

I believe this PR already handles that distinction correctly. But in this version it's pretty subtle in the code, and so it'd be easy for some future change to break that privacy property without realizing there was something important there. We should therefore arrange the code so that the distinction is clear. As part of that, we should avoid having these two kinds of URLs with very different privacy properties both live as values of the same field on the same class.

Conversely, MessageVideo is doing very little, really just dispatching to these two related classes that have pretty different logic from each other. So even apart from the privacy issue, it feels like having two different ContentNode subclasses would better match up with how the logic actually works.

If there's anyplace where it would be helpful to do so, those two ContentNode subclasses could always have a common base class with a name like VideoNode.

gnprice · 2024-04-01T23:40:46Z

lib/widgets/content.dart

+              Navigator.of(context).push(getLightboxRoute(
+                context: context,
+                message: message,
+                src: widget.src,
+                videoController: _controller,


It looks like this means that playback in the lightbox picks up from right where it was. Neat!

rajveermalviya · 2024-04-05T11:58:16Z

@gnprice, thank you for your review. I've just pushed a new revision. Apologies for the delay

gnprice · 2024-04-05T22:28:59Z

Thanks for the revision!

I'm just about to head out on vacation, so it'll be a couple of weeks before I'm able to give this PR another review. But I'll look forward to picking it up when I'm back.

@chrisbobbe if you find some time to help move this forward with a review while I'm away, the key thing I'd highlight from the above review threads is this point about the important privacy-related difference between inline vs. embedded videos.

gnprice · 2024-05-06T20:49:44Z

Thanks for the revision! Comments above. Generally this structure all looks good, so this continues to get closer; the one area of code I haven't yet reviewed remains the tests in the last/main commit.

I've also just pushed a couple of added commits on top, described in my comments above. One is meant to remain a separate commit on top, and the other to have its changes squashed into a couple of earlier commits.

rajveermalviya · 2024-05-09T08:18:44Z

Thanks for the review! Pushed a new revision.

rajveermalviya · 2024-05-09T08:55:33Z

(rebased to main)

gnprice

Thanks for the revision!

With this review I've gotten through the whole thing now, including those widget tests toward the end. Various comments below; outside those newly-reviewed tests, we're down to just a couple of them. So this is quite close.

As mentioned below, I'll also push an added commit on top, meant to be squashed in.

gnprice · 2024-05-10T02:38:11Z

lib/widgets/lightbox.dart

+    super.dispose();
+  }
+
+  void _handleVideoControllerUpdates() {


nit:

Suggested change

void _handleVideoControllerUpdates() {

void _handleVideoControllerUpdate() {

(it handles each update one at a time; this is the same way we name other "handle" methods, like handleEvent)

gnprice · 2024-05-10T02:47:46Z

lib/widgets/lightbox.dart

+      // After 'controller.seekTo' is called in 'Slider.onChangeEnd' the
+      // position indicator switches back to the actual controller's position
+      // but since the call 'seekTo' completes before the actual controller
+      // updates are notified, the position indicator that switches to controller's
+      // position can show the older position before the call to 'seekTo' for a
+      // single frame, resulting in a glichty UX.
+      //
+      // To avoid that, we delay the position indicator switch from '_sliderValue' to
+      // happen when we are notified of the controller update.
+      if (_isSliderDragging && _sliderValue == widget.controller.value.position) {
+        _isSliderDragging = false;


Hmm interesting, I see. (This follows the subthread at #587 (comment) .)

Cool — let's run with this logic, then.

gnprice · 2024-05-10T02:59:22Z

lib/widgets/lightbox.dart

+    // to display an error dialog if the video loading fails.
+    final zulipLocalizations = ZulipLocalizations.of(context);
+
+    _initialize(store, zulipLocalizations);


Hmm, but this does reinitialize, right? It looks like it'll initialize unconditionally on any call to onNewStore.

I'll push an added commit on top that arranges this initialization the way I think we want — please take a look, and then squash it in.

For the localizations, that commit also just moves the ZulipLocalizations.of down to just before we actually use its data, with no await in between. That's the ideal behavior, and if we make a practice of doing that then I think it can go without comment.

gnprice · 2024-05-10T03:06:56Z

test/widgets/lightbox_test.dart

+        home: PerAccountStoreWidget(
+          accountId: eg.selfAccount.id,
+          child: VideoLightboxPage(
+            routeEntranceAnimation: const MockCompletedAnimation(),


/// An animation that is always considered complete and /// immediately notifies listeners. /// /// This is different from [kAlwaysCompleteAnimation] where /// it doesn't notify the listeners. class MockCompletedAnimation extends Animation<double> {

Hmm interesting. Is this the right scenario, though, then?

If kAlwaysCompleteAnimation in the framework doesn't notify listeners (because nothing changes), then maybe that's the same thing that would happen if for whatever reason our page doesn't get built until the animation is already complete. (E.g., if the device is super overloaded and a bunch of frames get dropped.)

Can we adjust the app code so the test passes with kAlwaysCompleteAnimation here? I think it may just be a matter of calling _handleRouteEntranceAnimationStatusChange once in initState.

gnprice · 2024-05-10T03:39:50Z

test/widgets/lightbox_test.dart

+
+  group("VideoLightboxPage", () {
+    // Setup the mock for video_player plugin
+    setupMockVideoPlayerPlatform();


nit: Let's make this a static on MockVideoPlayerPlatform. That groups it nicely under the class as a namespace.

Looking at call sites of VideoPlayerPlatform.instance=, a conventional name for this would be:

Suggested change

setupMockVideoPlayerPlatform();

MockVideoPlayerPlatform.registerWith();

gnprice · 2024-05-10T04:28:22Z

test/test_video_player.dart

+  VideoPlayerPlatform.instance = MockVideoPlayerPlatform();
+}
+
+class MockVideoPlayerPlatform extends Fake


Suggested change

class MockVideoPlayerPlatform extends Fake

class FakeVideoPlayerPlatform extends Fake

That is, let's call this a "fake" instead of a "mock".

Broadly, a mock's method implementations do nothing except record they were called, in a log the test will then inspect. A fake's method implementations do some facsimile of what a real implementation would do, just with all the expensive parts replaced by shortcuts.

This is already more of a fake than a mock, because e.g. the test looks at isPlaying rather than counting calls to play and pause. And that's a good thing — generally fakes are preferable to mocks. They make the tests less coupled to accidental implementation details of the code under test, and better aligned to the code's actual interface and spec.

gnprice · 2024-05-10T04:31:48Z

test/widgets/content_test.dart

+      final expectedResolvedPreviewUrl = eg
+        .store()
+        .tryResolveUrl(expectedVideo.previewImageSrcUrl)!;


nit:

Suggested change

final expectedResolvedPreviewUrl = eg

.store()

.tryResolveUrl(expectedVideo.previewImageSrcUrl)!;

final expectedResolvedPreviewUrl = eg.store()

.tryResolveUrl(expectedVideo.previewImageSrcUrl)!;

("eg" alone means very little, so keep it together with the name of what we're pulling off it)

gnprice · 2024-05-10T04:46:56Z

test/widgets/content_test.dart

+      const example = ContentExample.videoEmbedVimeo;
+      await prepareContent(tester, example.html);
+
+      final expectedTitle = (((example.expectedNodes[0] as ParagraphNode).nodes[0] as LinkNode).nodes[0] as TextNode).text;


nit: line very long

Suggested change

final expectedTitle = (((example.expectedNodes[0] as ParagraphNode).nodes[0] as LinkNode).nodes[0] as TextNode).text;

final expectedTitle = (((example.expectedNodes[0] as ParagraphNode)

.nodes.single as LinkNode).nodes.single as TextNode).text;

Also .single is good when applicable, as it makes more of the expectations clear.

gnprice · 2024-05-10T04:48:14Z

test/widgets/content_test.dart

+      await prepareContent(tester, example.html);
+
+      final expectedTitle = (((example.expectedNodes[0] as ParagraphNode).nodes[0] as LinkNode).nodes[0] as TextNode).text;
+      await tester.ensureVisible(find.text(expectedTitle));


Huh interesting — why is this needed in this test but not in the previous one?

More generally, if these test cases work exactly the same way, it'd be nice to express that — basically factor out most of their guts, after the await prepareContent line, into a local helper like checkEmbedVideo. Which maybe takes the title string and the EmbedVideoNode as arguments.

gnprice · 2024-05-10T04:52:37Z

test/widgets/content_test.dart

+    });
+  });
+
+  group("MessageInlineVideo", () {


nit: put these tests in same order as the code they're testing

rajveermalviya · 2024-05-10T19:07:54Z

Thanks for the review @gnprice, new revision pushed!

gnprice

Thanks @rajveermalviya for all your work on this feature! This all looks great — merging 🎉

There's one comment I remember writing yesterday but that somehow got lost on GitHub; not sure how. The comment was to ask for a few more tests, specifically some tests of the time slider. I'll write up a new version of that comment, but there's no need for it to block merging — please write those tests as a follow-up PR instead.

Used in tests for mocking video_player platform interface

Partially implements zulip#356, provides video thumbnail previews for embedded external videos (Youtube & Vimeo).

Fixes: zulip#356

Based on the reasoning at the one place we're using either of these options so far, it seems like we'll always want them coupled this way. (At least until some other reason comes along, and in that case we can revise this logic again.)

gnprice · 2024-05-10T20:20:27Z

test/widgets/lightbox_test.dart

+      await tester.ensureVisible(find.byType(VideoPlayer));
+    });
+
+    testWidgets('toggles between play and pause', (tester) async {


Let's have a couple of tests of the slider mechanism, too. In particular:

Slider starts at zero; let half the time pass, slider is at halfway point; let more than the full time pass, slider is at end. (E.g. it didn't jump back to zero.)

To manipulate time in a test, use our awaitFakeAsync helper; see existing examples. Then call FakeAsync.elapse.

Start dragging slider; drag it back and forth a few times; finish dragging. At each step, check its displayed position is the place you just dragged it to. Then at the end, check seekTo was only called once.
I.e., a regression test for the lag-causing issue you described avoiding at content: Handle video previews #587 (comment) .

Drag slider from one point to another. Check the slider appears at the place you dragged it to now, and at every frame until it starts advancing again because the video is playing. I.e., a regression test for the bug that the _isSliderDragging and _sliderValue logic is avoiding as explained in your comment quoted at content: Handle video previews #587 (comment) .

As described above at #587 (review) , please do those as a follow-up PR. Thanks!

gnprice · 2024-05-10T20:37:54Z

So let's file a followup issue for adding those, and link to your comment #587 (comment) — that will help us pick that work up later.

Filed this as #656.

gnprice reviewed Mar 24, 2024

View reviewed changes

rajveermalviya force-pushed the pr-video-content branch 2 times, most recently from e2882a7 to 2842628 Compare March 26, 2024 01:27

rajveermalviya marked this pull request as ready for review March 26, 2024 01:49

gnprice reviewed Mar 26, 2024

View reviewed changes

rajveermalviya force-pushed the pr-video-content branch 2 times, most recently from bf4e818 to b78af2f Compare March 26, 2024 22:06

rajveermalviya force-pushed the pr-video-content branch from b78af2f to af94ffd Compare March 28, 2024 18:49

rajveermalviya requested a review from gnprice March 28, 2024 18:51

gnprice reviewed Mar 28, 2024

View reviewed changes

rajveermalviya force-pushed the pr-video-content branch from af94ffd to 8a1a4ff Compare March 29, 2024 22:15

rajveermalviya requested a review from gnprice March 29, 2024 22:18

gnprice reviewed Apr 1, 2024

View reviewed changes

gnprice mentioned this pull request Apr 3, 2024

content: Handle blank text nodes after code blocks. #604

Merged

rajveermalviya force-pushed the pr-video-content branch 7 times, most recently from a9c7f70 to f7face3 Compare April 5, 2024 11:02

rajveermalviya requested a review from gnprice April 5, 2024 11:58

rajveermalviya force-pushed the pr-video-content branch from f7face3 to cac2d42 Compare April 6, 2024 06:50

rajveermalviya force-pushed the pr-video-content branch from 9ff8b44 to 98815ab Compare May 9, 2024 08:04

rajveermalviya requested a review from gnprice May 9, 2024 08:18

rajveermalviya force-pushed the pr-video-content branch from 98815ab to 86838a1 Compare May 9, 2024 08:54

gnprice reviewed May 10, 2024

View reviewed changes

rajveermalviya force-pushed the pr-video-content branch 2 times, most recently from 8acc35f to 6ac5a44 Compare May 10, 2024 19:07

rajveermalviya requested a review from gnprice May 10, 2024 19:08

gnprice reviewed May 10, 2024

View reviewed changes

rajveermalviya and others added 11 commits May 10, 2024 12:26

deps: Update CocoaPods to 1.15.2

8248db5

content: Handle html parsing of EmbedVideoNode

4ebdaa0

content: Handle html parsing of InlineVideoNode

c1861da

deps: Add video_player

6e8d8b0

deps: Add dev:plugin_platform_interface directly

0dbd24a

Used in tests for mocking video_player platform interface

deps: Add dev:video_player_platform_interface directly

b63db38

Used in tests for mocking video_player platform interface

content [nfc]: Pull out MessageMediaContainer widget

4b668ff

content: Implement embed video preview

1b7ae7f

Partially implements zulip#356, provides video thumbnail previews for embedded external videos (Youtube & Vimeo).

lightbox [nfc]: Pull out _LightboxPageLayout

9806c5a

content: Implement inline video preview

d5ece54

Fixes: zulip#356

gnprice force-pushed the pr-video-content branch from 6ac5a44 to 33b97ef Compare May 10, 2024 19:27

gnprice merged commit 33b97ef into zulip:main May 10, 2024
1 check passed

gnprice reviewed May 10, 2024

View reviewed changes

gnprice mentioned this pull request May 10, 2024

Preview inline videos directly in message list #656

Open

rajveermalviya deleted the pr-video-content branch May 11, 2024 05:16

rajveermalviya mentioned this pull request May 21, 2024

lightbox test: Add video player regression tests #694

Open

		const sourceType = r"(message_inline_video)?(youtube-video)?(embed-video)?";
		return RegExp("^message_inline_image $sourceType\|$sourceType message_inline_image\$");

	static const videoEmbedYoutubeClassNameReordered = ContentExample(
	static const videoEmbedYoutubeClassesFlipped = ContentExample(

	static const videoEmbedVimeoPreviewEnabled = ContentExample(
	static const videoEmbedVimeo = ContentExample(

		return switch (match.groups([1, 2])) {
		['message_inline_video', null] \|\| [null, 'message_inline_video']

	void _handleVideoControllerUpdates() {
	void _handleVideoControllerUpdate() {

	setupMockVideoPlayerPlatform();
	MockVideoPlayerPlatform.registerWith();

	class MockVideoPlayerPlatform extends Fake
	class FakeVideoPlayerPlatform extends Fake

	final expectedTitle = (((example.expectedNodes[0] as ParagraphNode).nodes[0] as LinkNode).nodes[0] as TextNode).text;
	final expectedTitle = (((example.expectedNodes[0] as ParagraphNode)
	.nodes.single as LinkNode).nodes.single as TextNode).text;

		COCOAPODS: 1.13.0
		COCOAPODS: 1.15.0

content: Handle video previews #587

content: Handle video previews #587

Conversation

rajveermalviya commented Mar 24, 2024 • edited

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Mar 26, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Mar 26, 2024

rajveermalviya commented Mar 28, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Mar 29, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Apr 5, 2024

gnprice commented Apr 5, 2024

gnprice commented May 6, 2024

rajveermalviya commented May 9, 2024

rajveermalviya commented May 9, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented May 10, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented May 10, 2024

rajveermalviya commented Mar 24, 2024 •

edited