-
Notifications
You must be signed in to change notification settings - Fork 309
content: Handle video previews #587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8248db5
4ebdaa0
c1861da
6e8d8b0
0dbd24a
b63db38
4b668ff
1b7ae7f
9806c5a
d5ece54
33b97ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -374,6 +374,83 @@ class ImageNode extends BlockContentNode { | |
} | ||
} | ||
|
||
class InlineVideoNode extends BlockContentNode { | ||
const InlineVideoNode({ | ||
super.debugHtmlNode, | ||
required this.srcUrl, | ||
}); | ||
|
||
/// A URL string for the video resource, on the Zulip server. | ||
/// | ||
/// This may be a relative URL string. It also may not work without adding | ||
/// authentication credentials to the request. | ||
/// | ||
/// Unlike [EmbedVideoNode.hrefUrl], this should always be a URL served by | ||
/// either the Zulip server itself or a service it trusts. It's therefore | ||
/// fine from a privacy perspective to eagerly request data from this resource | ||
/// when the user passively scrolls the video into view. | ||
final String srcUrl; | ||
|
||
@override | ||
bool operator ==(Object other) { | ||
return other is InlineVideoNode | ||
&& other.srcUrl == srcUrl; | ||
} | ||
|
||
@override | ||
int get hashCode => Object.hash('InlineVideoNode', srcUrl); | ||
|
||
@override | ||
void debugFillProperties(DiagnosticPropertiesBuilder properties) { | ||
super.debugFillProperties(properties); | ||
properties.add(StringProperty('srcUrl', srcUrl)); | ||
} | ||
} | ||
|
||
class EmbedVideoNode extends BlockContentNode { | ||
const EmbedVideoNode({ | ||
super.debugHtmlNode, | ||
required this.hrefUrl, | ||
required this.previewImageSrcUrl, | ||
}); | ||
|
||
/// A URL string for the video, typically on an external service. | ||
/// | ||
/// For example, this URL may be on youtube.com or vimeo.com. | ||
/// | ||
/// Unlike with [previewImageSrcUrl] or [InlineVideoNode.srcUrl], | ||
/// no requests should be made to this URL unless the user explicitly chooses | ||
/// to interact with the video, in order to protect the user's privacy. | ||
final String hrefUrl; | ||
|
||
/// A URL string for a thumbnail image for the video, on the Zulip server. | ||
/// | ||
/// This may be a relative URL string. It also may not work without adding | ||
/// authentication credentials to the request. | ||
/// | ||
/// Like [InlineVideoNode.srcUrl] and unlike [hrefUrl], this is suitable | ||
/// from a privacy perspective for eagerly fetching data when the user | ||
/// passively scrolls the video into view. | ||
final String previewImageSrcUrl; | ||
|
||
@override | ||
bool operator ==(Object other) { | ||
return other is EmbedVideoNode | ||
&& other.hrefUrl == hrefUrl | ||
&& other.previewImageSrcUrl == previewImageSrcUrl; | ||
} | ||
|
||
@override | ||
int get hashCode => Object.hash('EmbedVideoNode', hrefUrl, previewImageSrcUrl); | ||
|
||
@override | ||
void debugFillProperties(DiagnosticPropertiesBuilder properties) { | ||
super.debugFillProperties(properties); | ||
properties.add(StringProperty('hrefUrl', hrefUrl)); | ||
properties.add(StringProperty('previewImageSrcUrl', previewImageSrcUrl)); | ||
} | ||
} | ||
|
||
/// A content node that expects an inline layout context from its parent. | ||
/// | ||
/// When rendered into a Flutter widget tree, an inline content node | ||
|
@@ -968,6 +1045,83 @@ class _ZulipContentParser { | |
return ImageNode(srcUrl: src, debugHtmlNode: debugHtmlNode); | ||
} | ||
|
||
static final _videoClassNameRegexp = () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can move this down to just above |
||
const sourceType = r"(message_inline_video|youtube-video|embed-video)"; | ||
return RegExp("^message_inline_image $sourceType|$sourceType message_inline_image\$"); | ||
}(); | ||
|
||
BlockContentNode parseInlineVideoNode(dom.Element divElement) { | ||
assert(_debugParserContext == _ParserContext.block); | ||
assert(divElement.localName == 'div' | ||
&& _videoClassNameRegexp.hasMatch(divElement.className)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (This is fine, though, as it's debug-only.) |
||
|
||
final videoElement = () { | ||
if (divElement.nodes.length != 1) return null; | ||
final child = divElement.nodes[0]; | ||
if (child is! dom.Element) return null; | ||
if (child.localName != 'a') return null; | ||
if (child.className.isNotEmpty) return null; | ||
|
||
if (child.nodes.length != 1) return null; | ||
final grandchild = child.nodes[0]; | ||
if (grandchild is! dom.Element) return null; | ||
if (grandchild.localName != 'video') return null; | ||
if (grandchild.className.isNotEmpty) return null; | ||
return grandchild; | ||
}(); | ||
|
||
final debugHtmlNode = kDebugMode ? divElement : null; | ||
if (videoElement == null) { | ||
return UnimplementedBlockContentNode(htmlNode: divElement); | ||
} | ||
|
||
final src = videoElement.attributes['src']; | ||
if (src == null) { | ||
return UnimplementedBlockContentNode(htmlNode: divElement); | ||
} | ||
|
||
return InlineVideoNode(srcUrl: src, debugHtmlNode: debugHtmlNode); | ||
} | ||
|
||
BlockContentNode parseEmbedVideoNode(dom.Element divElement) { | ||
assert(_debugParserContext == _ParserContext.block); | ||
assert(divElement.localName == 'div' | ||
&& _videoClassNameRegexp.hasMatch(divElement.className)); | ||
|
||
final pair = () { | ||
if (divElement.nodes.length != 1) return null; | ||
final child = divElement.nodes[0]; | ||
if (child is! dom.Element) return null; | ||
if (child.localName != 'a') return null; | ||
if (child.className.isNotEmpty) return null; | ||
|
||
if (child.nodes.length != 1) return null; | ||
final grandchild = child.nodes[0]; | ||
if (grandchild is! dom.Element) return null; | ||
if (grandchild.localName != 'img') return null; | ||
if (grandchild.className.isNotEmpty) return null; | ||
return (child, grandchild); | ||
}(); | ||
|
||
final debugHtmlNode = kDebugMode ? divElement : null; | ||
if (pair == null) { | ||
return UnimplementedBlockContentNode(htmlNode: divElement); | ||
} | ||
final (anchorElement, imgElement) = pair; | ||
|
||
final imgSrc = imgElement.attributes['src']; | ||
if (imgSrc == null) { | ||
return UnimplementedBlockContentNode(htmlNode: divElement); | ||
} | ||
|
||
final href = anchorElement.attributes['href']; | ||
if (href == null) { | ||
return UnimplementedBlockContentNode(htmlNode: divElement); | ||
} | ||
|
||
return EmbedVideoNode(hrefUrl: href, previewImageSrcUrl: imgSrc, debugHtmlNode: debugHtmlNode); | ||
} | ||
|
||
BlockContentNode parseBlockContent(dom.Node node) { | ||
assert(_debugParserContext == _ParserContext.block); | ||
final debugHtmlNode = kDebugMode ? node : null; | ||
|
@@ -1048,6 +1202,19 @@ class _ZulipContentParser { | |
return parseImageNode(element); | ||
} | ||
|
||
if (localName == 'div') { | ||
final match = _videoClassNameRegexp.firstMatch(className); | ||
if (match != null) { | ||
final videoClass = match.group(1) ?? match.group(2)!; | ||
switch (videoClass) { | ||
case 'message_inline_video': | ||
return parseInlineVideoNode(element); | ||
case 'youtube-video' || 'embed-video': | ||
return parseEmbedVideoNode(element); | ||
} | ||
} | ||
} | ||
|
||
// TODO more types of node | ||
return UnimplementedBlockContentNode(htmlNode: node); | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -98,6 +98,10 @@ class BlockContentList extends StatelessWidget { | |||||||||
"It should be wrapped in [ImageNodeList]." | ||||||||||
); | ||||||||||
return MessageImage(node: node); | ||||||||||
} else if (node is InlineVideoNode) { | ||||||||||
return MessageInlineVideo(node: node); | ||||||||||
} else if (node is EmbedVideoNode) { | ||||||||||
return MessageEmbedVideo(node: node); | ||||||||||
} else if (node is UnimplementedBlockContentNode) { | ||||||||||
return Text.rich(_errorUnimplemented(node)); | ||||||||||
} else { | ||||||||||
|
@@ -374,11 +378,97 @@ class MessageImage extends StatelessWidget { | |||||||||
final resolvedSrc = store.tryResolveUrl(src); | ||||||||||
// TODO if src fails to parse, show an explicit "broken image" | ||||||||||
|
||||||||||
return GestureDetector( | ||||||||||
return MessageMediaContainer( | ||||||||||
onTap: resolvedSrc == null ? null : () { // TODO(log) | ||||||||||
Navigator.of(context).push(getLightboxRoute( | ||||||||||
context: context, message: message, src: resolvedSrc)); | ||||||||||
context: context, | ||||||||||
message: message, | ||||||||||
src: resolvedSrc, | ||||||||||
mediaType: MediaType.image)); | ||||||||||
}, | ||||||||||
child: resolvedSrc == null ? null : LightboxHero( | ||||||||||
message: message, | ||||||||||
src: resolvedSrc, | ||||||||||
child: RealmContentNetworkImage( | ||||||||||
resolvedSrc, | ||||||||||
filterQuality: FilterQuality.medium))); | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
class MessageInlineVideo extends StatelessWidget { | ||||||||||
const MessageInlineVideo({super.key, required this.node}); | ||||||||||
|
||||||||||
final InlineVideoNode node; | ||||||||||
|
||||||||||
@override | ||||||||||
Widget build(BuildContext context) { | ||||||||||
final message = InheritedMessage.of(context); | ||||||||||
final store = PerAccountStoreWidget.of(context); | ||||||||||
final resolvedSrc = store.tryResolveUrl(node.srcUrl); | ||||||||||
|
||||||||||
return MessageMediaContainer( | ||||||||||
onTap: resolvedSrc == null ? null : () { // TODO(log) | ||||||||||
Navigator.of(context).push(getLightboxRoute( | ||||||||||
context: context, | ||||||||||
message: message, | ||||||||||
src: resolvedSrc, | ||||||||||
mediaType: MediaType.video)); | ||||||||||
}, | ||||||||||
child: Container( | ||||||||||
color: Colors.black, | ||||||||||
alignment: Alignment.center, | ||||||||||
// To avoid potentially confusing UX, do not show play icon as | ||||||||||
// we also disable onTap above. | ||||||||||
child: resolvedSrc == null ? null : const Icon( // TODO(log) | ||||||||||
Icons.play_arrow_rounded, | ||||||||||
color: Colors.white, | ||||||||||
size: 32))); | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
class MessageEmbedVideo extends StatelessWidget { | ||||||||||
const MessageEmbedVideo({super.key, required this.node}); | ||||||||||
|
||||||||||
final EmbedVideoNode node; | ||||||||||
|
||||||||||
@override | ||||||||||
Widget build(BuildContext context) { | ||||||||||
final store = PerAccountStoreWidget.of(context); | ||||||||||
final previewImageSrcUrl = store.tryResolveUrl(node.previewImageSrcUrl); | ||||||||||
|
||||||||||
return MessageMediaContainer( | ||||||||||
onTap: () => _launchUrl(context, node.hrefUrl), | ||||||||||
child: Stack( | ||||||||||
alignment: Alignment.center, | ||||||||||
children: [ | ||||||||||
if (previewImageSrcUrl != null) // TODO(log) | ||||||||||
RealmContentNetworkImage( | ||||||||||
previewImageSrcUrl, | ||||||||||
filterQuality: FilterQuality.medium), | ||||||||||
// Show the "play" icon even when previewImageSrcUrl didn't resolve; | ||||||||||
// the action uses hrefUrl, which might still work. | ||||||||||
const Icon( | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like the two types of video differ in whether this "play" icon gets included when the URL fails to resolve. I was about to say they should be consistent (one way or the other), but then realized there is a relevant difference here: for the non-inline case, the URL we're trying to resolve (and learned does not resolve) isn't involved in the onTap action, so it's appropriate for the widget to show as non-disabled — it might still work if the user taps it. So I guess the conclusion is there should be a comment here mentioning that, since it's a bit subtle. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment added on the other case is fine, but this case is the one that feels more surprising to me, so it's where I think a comment would be most helpful. Here's a version:
Suggested change
|
||||||||||
Icons.play_arrow_rounded, | ||||||||||
color: Colors.white, | ||||||||||
size: 32), | ||||||||||
])); | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
class MessageMediaContainer extends StatelessWidget { | ||||||||||
const MessageMediaContainer({ | ||||||||||
super.key, | ||||||||||
required this.onTap, | ||||||||||
required this.child, | ||||||||||
}); | ||||||||||
|
||||||||||
final void Function()? onTap; | ||||||||||
final Widget? child; | ||||||||||
|
||||||||||
@override | ||||||||||
Widget build(BuildContext context) { | ||||||||||
return GestureDetector( | ||||||||||
onTap: onTap, | ||||||||||
child: UnconstrainedBox( | ||||||||||
alignment: Alignment.centerLeft, | ||||||||||
child: Padding( | ||||||||||
|
@@ -392,12 +482,7 @@ class MessageImage extends StatelessWidget { | |||||||||
child: SizedBox( | ||||||||||
height: 100, | ||||||||||
width: 150, | ||||||||||
child: resolvedSrc == null ? null : LightboxHero( | ||||||||||
message: message, | ||||||||||
src: resolvedSrc, | ||||||||||
child: RealmContentNetworkImage( | ||||||||||
resolvedSrc, | ||||||||||
filterQuality: FilterQuality.medium)))))))); | ||||||||||
child: child)))))); | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give this field a doc comment explaining the important way that it's different from what's on
EmbedVideoNode
, i.e. with the information from #587 (comment) . Similarly let's document the two fields onEmbedVideoNode
.I think even when one is aware of that difference, it's a bit hard to remember which way is which, so it's good to have it written down explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I started from the text in your revised version, and made some edits. I'll push those in an added commit on top; please squash that commit's changes into the respective commits in the branch.