Skip to content

content: Handle thumbnails #820

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

Merged
merged 2 commits into from
Jul 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 65 additions & 10 deletions lib/model/content.dart
Original file line number Diff line number Diff line change
Expand Up @@ -351,26 +351,51 @@ class ImageNodeList extends BlockContentNode {
}

class ImageNode extends BlockContentNode {
const ImageNode({super.debugHtmlNode, required this.srcUrl});
const ImageNode({
super.debugHtmlNode,
required this.srcUrl,
required this.thumbnailUrl,
required this.loading,
});
Comment on lines +354 to +359
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's split this into two constructors:

Suggested change
const ImageNode({
super.debugHtmlNode,
required this.srcUrl,
required this.thumbnailUrl,
required this.loading,
});
const ImageNode({
super.debugHtmlNode,
required String this.srcUrl,
required String this.thumbnailUrl,
}) : loading = false;

and then an ImageNode.loading that makes those other fields null.

That way the constructors enforce that there are these two mutually exclusive configurations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… Actually, though, scratch that — because I think we want to model this a bit differently.

Even if the thumbnail is still pending, that's no reason to stop the user from opening the lightbox. So let's always have the original URL, which is the one from the a element.


/// The unmodified `src` attribute for the image.
/// The canonical source URL of the image.
///
/// This may be a relative URL string. It also may not work without adding
/// This may be a relative URL string. It also may not work without adding
/// authentication credentials to the request.
final String srcUrl;

/// The thumbnail URL of the image.
///
/// This may be a relative URL string. It also may not work without adding
/// authentication credentials to the request.
///
/// This will be null if the server hasn't yet generated a thumbnail,
/// or is a version that doesn't offer thumbnails.
/// It will also be null when [loading] is true.
final String? thumbnailUrl;

/// A flag to indicate whether to show the placeholder.
///
/// Typically it will be `true` while Server is generating thumbnails.
final bool loading;

@override
bool operator ==(Object other) {
return other is ImageNode && other.srcUrl == srcUrl;
return other is ImageNode
&& other.srcUrl == srcUrl
&& other.thumbnailUrl == thumbnailUrl
&& other.loading == loading;
}

@override
int get hashCode => Object.hash('ImageNode', srcUrl);
int get hashCode => Object.hash('ImageNode', srcUrl, thumbnailUrl, loading);

@override
void debugFillProperties(DiagnosticPropertiesBuilder properties) {
super.debugFillProperties(properties);
properties.add(StringProperty('srcUrl', srcUrl));
properties.add(StringProperty('thumbnailUrl', thumbnailUrl));
properties.add(FlagProperty('loading', value: loading, ifTrue: "is loading"));
}
}

Expand Down Expand Up @@ -1014,7 +1039,7 @@ class _ZulipContentParser {

BlockContentNode parseImageNode(dom.Element divElement) {
assert(_debugParserContext == _ParserContext.block);
final imgElement = () {
final elements = () {
assert(divElement.localName == 'div'
&& divElement.className == 'message_inline_image');

Expand All @@ -1028,21 +1053,51 @@ class _ZulipContentParser {
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 grandchild;
return (child, grandchild);
}();

final debugHtmlNode = kDebugMode ? divElement : null;
if (imgElement == null) {
if (elements == null) {
return UnimplementedBlockContentNode(htmlNode: divElement);
}

final (linkElement, imgElement) = elements;
final href = linkElement.attributes['href'];
if (href == null) {
return UnimplementedBlockContentNode(htmlNode: divElement);
}
if (imgElement.className == 'image-loading-placeholder') {
return ImageNode(
srcUrl: href,
thumbnailUrl: null,
loading: true,
debugHtmlNode: debugHtmlNode);
}
final src = imgElement.attributes['src'];
if (src == null) {
return UnimplementedBlockContentNode(htmlNode: divElement);
}

return ImageNode(srcUrl: src, debugHtmlNode: debugHtmlNode);
final String srcUrl;
final String? thumbnailUrl;
if (src.startsWith('/user_uploads/thumbnail/')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this condition is false, let's have a check that src == href, and return unimplemented if they're unequal.

I believe those were always the same when thumbnailing isn't involved; if there are other cases that exist, I'd like to learn about them, either in manual testing as we use the app or as part of #190.

Copy link
Member Author

@rajveermalviya rajveermalviya Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be multiple cases where src != href, for example src urls starting with /external_content/ and https://uploads.zulipusercontent.net/.
Should they be added one by one explicitly (using startsWith)?

Copy link
Member Author

@rajveermalviya rajveermalviya Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they be added one by one explicitly

Did this for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that sounds good.

Oh, it looks like we don't have a test case that exercises the /external_content/ case, though. Do you have an example? That'd be good to add.

srcUrl = href;
thumbnailUrl = src;
} else if (src.startsWith('/external_content/')
|| src.startsWith('https://uploads.zulipusercontent.net/')) {
srcUrl = src;
thumbnailUrl = null;
} else if (href == src) {
srcUrl = src;
thumbnailUrl = null;
} else {
return UnimplementedBlockContentNode(htmlNode: divElement);
}
return ImageNode(
srcUrl: srcUrl,
thumbnailUrl: thumbnailUrl,
loading: false,
debugHtmlNode: debugHtmlNode);
}

static final _videoClassNameRegexp = () {
Expand Down
30 changes: 19 additions & 11 deletions lib/widgets/content.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'package:flutter/cupertino.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter/gestures.dart';
import 'package:flutter/material.dart';
Expand Down Expand Up @@ -571,26 +572,32 @@ class MessageImage extends StatelessWidget {
final message = InheritedMessage.of(context);

// TODO image hover animation
final src = node.srcUrl;

final srcUrl = node.srcUrl;
final thumbnailUrl = node.thumbnailUrl;
final store = PerAccountStoreWidget.of(context);
final resolvedSrc = store.tryResolveUrl(src);
final resolvedSrcUrl = store.tryResolveUrl(srcUrl);
final resolvedThumbnailUrl = thumbnailUrl == null
? null : store.tryResolveUrl(thumbnailUrl);

// TODO if src fails to parse, show an explicit "broken image"

return MessageMediaContainer(
onTap: resolvedSrc == null ? null : () { // TODO(log)
onTap: resolvedSrcUrl == null ? null : () { // TODO(log)
Navigator.of(context).push(getLightboxRoute(
context: context,
message: message,
src: resolvedSrc,
src: resolvedSrcUrl,
thumbnailUrl: resolvedThumbnailUrl,
mediaType: MediaType.image));
},
child: resolvedSrc == null ? null : LightboxHero(
message: message,
src: resolvedSrc,
child: RealmContentNetworkImage(
resolvedSrc,
filterQuality: FilterQuality.medium)));
child: node.loading
? const CupertinoActivityIndicator()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also forgot to flag this earlier, the reason for not using the loader-{black,white}.svg asset here is that, currently animated icons can't be included in ZulipIcons font (don't know if animated fonts are even possible), I tried and it just displayed a single (first) frame.

So, for now used the CupertinoActivityIndicator widget which conveniently is similar to the svg.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handy that that's there!

Yeah, I suspect animated fonts are not possible. If we were determined to use the icons from those SVGs, we could find a way (if nothing else, we could rasterize them to high-enough-resolution PNGs). But using this widget instead is a lot simpler and SGTM.

Would you post a screenshot and a video showing what this placeholder looks like? That doesn't require any fiddling on the server — the quick hack I used just now is:

--- lib/model/content.dart
+++ lib/model/content.dart
@@ -1064,7 +1064,7 @@ class _ZulipContentParser {
     if (href == null) {
       return UnimplementedBlockContentNode(htmlNode: divElement);
     }
-    if (imgElement.className == 'image-loading-placeholder') {
+    if (true || imgElement.className == 'image-loading-placeholder') {
       return ImageNode(
         srcUrl: href,
         thumbnailUrl: null,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a video showing placeholders in the PR description.

: resolvedSrcUrl == null ? null : LightboxHero(
message: message,
src: resolvedSrcUrl,
child: RealmContentNetworkImage(
resolvedThumbnailUrl ?? resolvedSrcUrl,
filterQuality: FilterQuality.medium)));
}
}

Expand All @@ -611,6 +618,7 @@ class MessageInlineVideo extends StatelessWidget {
context: context,
message: message,
src: resolvedSrc,
thumbnailUrl: null,
mediaType: MediaType.video));
},
child: Container(
Expand Down
70 changes: 67 additions & 3 deletions lib/widgets/lightbox.dart
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import 'package:flutter/material.dart';
import 'package:flutter/scheduler.dart';
import 'package:flutter/services.dart';
import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
import 'package:intl/intl.dart';
Expand Down Expand Up @@ -91,12 +92,17 @@ class _LightboxPageLayout extends StatefulWidget {
const _LightboxPageLayout({
required this.routeEntranceAnimation,
required this.message,
required this.buildAppBarBottom,
required this.buildBottomAppBar,
required this.child,
});

final Animation<double> routeEntranceAnimation;
final Message message;

/// For [AppBar.bottom].
final PreferredSizeWidget? Function(BuildContext context) buildAppBarBottom;

final Widget? Function(
BuildContext context, Color color, double elevation) buildBottomAppBar;
final Widget child;
Expand Down Expand Up @@ -171,7 +177,8 @@ class _LightboxPageLayoutState extends State<_LightboxPageLayout> {

// Make smaller, like a subtitle
style: themeData.textTheme.titleSmall!.copyWith(color: appBarForegroundColor)),
])));
])),
bottom: widget.buildAppBarBottom(context));
}

Widget? bottomAppBar;
Expand Down Expand Up @@ -209,17 +216,30 @@ class _ImageLightboxPage extends StatefulWidget {
required this.routeEntranceAnimation,
required this.message,
required this.src,
required this.thumbnailUrl,
});

final Animation<double> routeEntranceAnimation;
final Message message;
final Uri src;
final Uri? thumbnailUrl;

@override
State<_ImageLightboxPage> createState() => _ImageLightboxPageState();
}

class _ImageLightboxPageState extends State<_ImageLightboxPage> {
double? _loadingProgress;

PreferredSizeWidget? _buildAppBarBottom(BuildContext context) {
if (_loadingProgress == null) {
return null;
Comment on lines +234 to +236
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, moving this here (from the call site) is a nice simplification too.

}
return PreferredSize(
preferredSize: const Size.fromHeight(4.0),
child: LinearProgressIndicator(minHeight: 4.0, value: _loadingProgress));
}

Widget _buildBottomAppBar(BuildContext context, Color color, double elevation) {
return BottomAppBar(
color: color,
Expand All @@ -232,19 +252,60 @@ class _ImageLightboxPageState extends State<_ImageLightboxPage> {
);
}

Widget _frameBuilder(BuildContext context, Widget child, int? frame, bool wasSynchronouslyLoaded) {
if (widget.thumbnailUrl == null) return child;

// The full image is available, so display it.
if (frame != null) return child;

// Display the thumbnail image while original image is downloading.
return RealmContentNetworkImage(widget.thumbnailUrl!,
filterQuality: FilterQuality.medium);
}

Widget _loadingBuilder(BuildContext context, Widget child, ImageChunkEvent? loadingProgress) {
if (widget.thumbnailUrl == null) return child;

// `loadingProgress` becomes null when Image has finished downloading.
final double? progress = loadingProgress?.expectedTotalBytes == null ? null
: loadingProgress!.cumulativeBytesLoaded / loadingProgress.expectedTotalBytes!;

if (progress != _loadingProgress) {
_loadingProgress = progress;
// The [Image.network] API lets us learn progress information only at
// its build time. That's too late for updating the progress indicator,
// so delay that update to the next frame. For discussion, see:
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/addPostFrameCallback/near/1893539
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/addPostFrameCallback/near/1894124
SchedulerBinding.instance.scheduleFrameCallback((_) {
if (!mounted) return;
setState(() {});
});
}
return child;
}

@override
Widget build(BuildContext context) {
return _LightboxPageLayout(
routeEntranceAnimation: widget.routeEntranceAnimation,
message: widget.message,
buildAppBarBottom: _buildAppBarBottom,
buildBottomAppBar: _buildBottomAppBar,
child: SizedBox.expand(
child: InteractiveViewer(
child: SafeArea(
child: LightboxHero(
message: widget.message,
src: widget.src,
child: RealmContentNetworkImage(widget.src, filterQuality: FilterQuality.medium))))));
child: RealmContentNetworkImage(widget.src,
filterQuality: FilterQuality.medium,
frameBuilder: _frameBuilder,
loadingBuilder: _loadingBuilder),
),
),
),
));
}
}

Expand Down Expand Up @@ -457,6 +518,7 @@ class _VideoLightboxPageState extends State<VideoLightboxPage> with PerAccountSt
return _LightboxPageLayout(
routeEntranceAnimation: widget.routeEntranceAnimation,
message: widget.message,
buildAppBarBottom: (context) => null,
buildBottomAppBar: _buildBottomAppBar,
child: SafeArea(
child: Center(
Expand Down Expand Up @@ -484,6 +546,7 @@ Route<void> getLightboxRoute({
BuildContext? context,
required Message message,
required Uri src,
required Uri? thumbnailUrl,
required MediaType mediaType,
}) {
return AccountPageRouteBuilder(
Expand All @@ -500,7 +563,8 @@ Route<void> getLightboxRoute({
MediaType.image => _ImageLightboxPage(
routeEntranceAnimation: animation,
message: message,
src: src),
src: src,
thumbnailUrl: thumbnailUrl),
MediaType.video => VideoLightboxPage(
routeEntranceAnimation: animation,
message: message,
Expand Down
Loading