Skip to content

Commit df60c98

Browse files
committed
content [nfc]: Record href and src straightforwardly on ImagePreviewNode
I've frequently gotten confused about the roles of `srcUrl` and `thumbnail`. The former has been especially confusing while thinking through all the cases we have to handle. The name `srcUrl` seemed to suggest (a) parsing something into a Uri object and (b) representing the `src` attribute in the HTML. Neither was true: it was a String, and it held the HTML's `href` at least as commonly as `src`. To straighten this out, just have ImagePreviewNode hold `href` and `src`, and transcribe the fields' meanings from the API doc and CZO discussions. The `src` field is a new type ImagePreviewNodeSrc with two subclasses corresponding to the thumbnail- and not-thumbnail-URL cases.
1 parent a048bda commit df60c98

File tree

4 files changed

+243
-140
lines changed

4 files changed

+243
-140
lines changed

lib/model/content.dart

Lines changed: 105 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -539,8 +539,8 @@ class ImagePreviewNodeList extends BlockContentNode {
539539
class ImagePreviewNode extends BlockContentNode {
540540
const ImagePreviewNode({
541541
super.debugHtmlNode,
542-
required this.srcUrl,
543-
required this.thumbnail,
542+
required this.originalSrc,
543+
required this.src,
544544
required this.loading,
545545
required this.originalWidth,
546546
required this.originalHeight,
@@ -550,51 +550,129 @@ class ImagePreviewNode extends BlockContentNode {
550550
///
551551
/// This may be a relative URL string. It also may not work without adding
552552
/// authentication credentials to the request.
553-
final String srcUrl;
553+
///
554+
/// Clients are expected to use this URL when saving the image to the device.
555+
///
556+
/// For images processed in modern thumbnailing (as of 2026-01),
557+
/// this is also meant for viewing the image by itself, in a lightbox
558+
/// (but if `data-transcoded-image` is present, it's better to use that [1]).
559+
/// The modern-thumbnailing case is recognized when [loading] is true
560+
/// or when [src] is an [ImagePreviewNodeSrcThumbnail].
561+
/// From discussion:
562+
/// https://chat.zulip.org/#narrow/channel/412-api-documentation/topic/documenting.20inline.20images/near/2279483
563+
///
564+
/// [1] The "transcoded image" feature is meant to keep the lightbox working
565+
/// when the original image is in an uncommon format, like TIFF.
566+
/// This isn't implemented yet; it's #1268.
567+
// TODO(#1268) implement transcoded-image feature; update dartdoc
568+
final String originalSrc;
554569

555-
/// The thumbnail URL of the image and whether it has an animated version.
570+
/// A URL for the image intended to be shown here in Zulip content.
571+
///
572+
/// If [loading] is true, this will point to a "spinner" image.
573+
/// Clients are invited to show a custom loading indicator instead; we do.
556574
///
557-
/// This will be null if the server hasn't yet generated a thumbnail,
558-
/// or is a version that doesn't offer thumbnails.
559-
/// It will also be null when [loading] is true.
560-
final ImageThumbnailLocator? thumbnail;
575+
/// Except for images processed in modern thumbnailing (2026-01),
576+
/// this is also meant for viewing the image by itself, in a lightbox.
577+
/// For how to recognize that case, see [originalSrc].
578+
final ImagePreviewNodeSrc src;
561579

562-
/// A flag to indicate whether to show the placeholder.
580+
/// Whether the img has the "image-loading-placeholder" classname.
563581
///
564-
/// Typically it will be `true` while Server is generating thumbnails.
582+
/// This is expected to be true (as of 2026-01)
583+
/// while an uploaded image is being thumbnailed.
584+
///
585+
/// When this is true, [src] will point to a "spinner" image.
586+
/// Clients are invited to show a custom loading indicator instead; we do.
565587
final bool loading;
566588

567-
/// The width of the canonical image.
589+
/// The width part of data-original-dimensions, if that attribute is present.
568590
final double? originalWidth;
569591

570-
/// The height of the canonical image.
592+
/// The height part of data-original-dimensions, if that attribute is present.
571593
final double? originalHeight;
572594

573595
@override
574596
bool operator ==(Object other) {
575597
return other is ImagePreviewNode
576-
&& other.srcUrl == srcUrl
577-
&& other.thumbnail == thumbnail
598+
&& other.originalSrc == originalSrc
599+
&& other.src == src
578600
&& other.loading == loading
579601
&& other.originalWidth == originalWidth
580602
&& other.originalHeight == originalHeight;
581603
}
582604

583605
@override
584606
int get hashCode => Object.hash('ImagePreviewNode',
585-
srcUrl, thumbnail, loading, originalWidth, originalHeight);
607+
originalSrc, src, loading, originalWidth, originalHeight);
586608

587609
@override
588610
void debugFillProperties(DiagnosticPropertiesBuilder properties) {
589611
super.debugFillProperties(properties);
590-
properties.add(StringProperty('srcUrl', srcUrl));
591-
properties.add(DiagnosticsProperty<ImageThumbnailLocator>('thumbnail', thumbnail));
612+
properties.add(StringProperty('originalSrc', originalSrc));
613+
properties.add(DiagnosticsProperty<ImagePreviewNodeSrc>('src', src));
592614
properties.add(FlagProperty('loading', value: loading, ifTrue: "is loading"));
593615
properties.add(DoubleProperty('originalWidth', originalWidth));
594616
properties.add(DoubleProperty('originalHeight', originalHeight));
595617
}
596618
}
597619

620+
/// A value of [ImagePreviewNode.src].
621+
sealed class ImagePreviewNodeSrc extends DiagnosticableTree {
622+
const ImagePreviewNodeSrc();
623+
}
624+
625+
/// A thumbnail URL, starting with [ImageThumbnailLocator.srcPrefix].
626+
class ImagePreviewNodeSrcThumbnail extends ImagePreviewNodeSrc {
627+
const ImagePreviewNodeSrcThumbnail(this.value);
628+
629+
final ImageThumbnailLocator value;
630+
631+
@override
632+
bool operator ==(Object other) {
633+
return other is ImagePreviewNodeSrcThumbnail && other.value == value;
634+
}
635+
636+
@override
637+
int get hashCode => Object.hash('ImagePreviewNodeSrcThumbnail', value);
638+
639+
@override
640+
void debugFillProperties(DiagnosticPropertiesBuilder properties) {
641+
super.debugFillProperties(properties);
642+
properties.add(DiagnosticsProperty<ImageThumbnailLocator>('value', value));
643+
}
644+
}
645+
646+
/// A `src` that does not start with [ImageThumbnailLocator.srcPrefix].
647+
///
648+
/// This may be a relative URL string. It also may not work without adding
649+
/// authentication credentials to the request.
650+
// In 2026-01, this class covers these known cases:
651+
// - This may be a hard-coded "spinner" image,
652+
// when thumbnailing of an uploaded image is in progress.
653+
// - This may match `href`, e.g. from pre-thumbnailing servers.
654+
// - This may start with CAMO_URI, a server variable (e.g. on Zulip Cloud
655+
// it's "https://uploads.zulipusercontent.net/" in 2025-10).
656+
class ImagePreviewNodeSrcOther extends ImagePreviewNodeSrc {
657+
const ImagePreviewNodeSrcOther(this.value);
658+
659+
final String value;
660+
661+
@override
662+
bool operator ==(Object other) {
663+
return other is ImagePreviewNodeSrcOther && other.value == value;
664+
}
665+
666+
@override
667+
int get hashCode => Object.hash('ImagePreviewNodeSrcOther', value);
668+
669+
@override
670+
void debugFillProperties(DiagnosticPropertiesBuilder properties) {
671+
super.debugFillProperties(properties);
672+
properties.add(StringProperty('value', value));
673+
}
674+
}
675+
598676
/// Data to locate an image thumbnail,
599677
/// and whether the image has an animated version.
600678
///
@@ -1436,31 +1514,15 @@ class _ZulipContentParser {
14361514
final src = imgElement.attributes['src'];
14371515
if (src == null) return null;
14381516
final loading = imgElement.className == 'image-loading-placeholder';
1439-
1440-
final String srcUrl;
1441-
final ImageThumbnailLocator? thumbnail;
1442-
if (loading) {
1443-
// This lets us offer a lightbox showing the full image,
1444-
// even while the thumbnail is loading.
1445-
srcUrl = href;
1446-
thumbnail = null; // (The thumbnail is the thing that's loading.)
1447-
} else if (src.startsWith(ImageThumbnailLocator.srcPrefix)) {
1448-
final parsedSrc = Uri.tryParse(src);
1449-
if (parsedSrc == null) return null;
1450-
1517+
ImageThumbnailLocator? thumbnailSrc;
1518+
if (src.startsWith(ImageThumbnailLocator.srcPrefix)) {
1519+
final srcUrl = Uri.tryParse(src);
1520+
if (srcUrl == null) return null;
14511521
// For why we recognize this as the thumbnail form, see discussion:
14521522
// https://chat.zulip.org/#narrow/channel/412-api-documentation/topic/documenting.20inline.20images/near/2279872
1453-
srcUrl = href;
1454-
thumbnail = ImageThumbnailLocator(
1455-
defaultFormatSrc: parsedSrc,
1523+
thumbnailSrc = ImageThumbnailLocator(
1524+
defaultFormatSrc: srcUrl,
14561525
animated: imgElement.attributes['data-animated'] == 'true');
1457-
} else {
1458-
// Known cases this handles:
1459-
// - `src` starts with CAMO_URI, a server variable (e.g. on Zulip Cloud
1460-
// it's "https://uploads.zulipusercontent.net/" in 2025-10).
1461-
// - `src` matches `href`, e.g. from pre-thumbnailing servers.
1462-
srcUrl = src;
1463-
thumbnail = null;
14641526
}
14651527

14661528
double? originalWidth, originalHeight;
@@ -1483,8 +1545,10 @@ class _ZulipContentParser {
14831545
}
14841546

14851547
return ImagePreviewNode(
1486-
srcUrl: srcUrl,
1487-
thumbnail: thumbnail,
1548+
originalSrc: href,
1549+
src: thumbnailSrc != null
1550+
? ImagePreviewNodeSrcThumbnail(thumbnailSrc)
1551+
: ImagePreviewNodeSrcOther(src),
14881552
loading: loading,
14891553
originalWidth: originalWidth,
14901554
originalHeight: originalHeight,

lib/widgets/content.dart

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -636,15 +636,18 @@ class MessageImagePreview extends StatelessWidget {
636636
final store = PerAccountStoreWidget.of(context);
637637
final message = InheritedMessage.of(context);
638638

639-
final resolvedThumbnailUrl = node.thumbnail?.resolve(context,
640-
width: MessageMediaContainer.width,
641-
height: MessageMediaContainer.height,
642-
animationMode: ImageAnimationMode.animateConditionally);
643-
644-
final urlInPreview = node.thumbnail != null
645-
? resolvedThumbnailUrl
646-
: store.tryResolveUrl(node.srcUrl);
647-
final child = switch ((node.loading, urlInPreview)) {
639+
final resolvedSrc = switch (node.src) {
640+
ImagePreviewNodeSrcThumbnail(:final value) => value.resolve(context,
641+
width: MessageMediaContainer.width,
642+
height: MessageMediaContainer.height,
643+
animationMode: .animateConditionally),
644+
ImagePreviewNodeSrcOther(:final value) => store.tryResolveUrl(value),
645+
};
646+
final resolvedOriginalSrc = store.tryResolveUrl(node.originalSrc);
647+
648+
final child = switch ((node.loading, resolvedSrc)) {
649+
// resolvedSrc would be a "spinner" image URL.
650+
// Use our own progress indicator instead.
648651
(true, _) => const CupertinoActivityIndicator(),
649652

650653
// TODO(#265) use an error-case placeholder
@@ -654,10 +657,12 @@ class MessageImagePreview extends StatelessWidget {
654657
(false, Uri()) => RealmContentNetworkImage(
655658
// TODO(#265) use an error-case placeholder for `errorBuilder`
656659
filterQuality: FilterQuality.medium,
657-
urlInPreview!),
660+
resolvedSrc!),
658661
};
659662

660-
final lightboxDisplayUrl = store.tryResolveUrl(node.srcUrl);
663+
final lightboxDisplayUrl = (node.loading || node.src is ImagePreviewNodeSrcThumbnail)
664+
? resolvedOriginalSrc
665+
: resolvedSrc;
661666
if (lightboxDisplayUrl == null) {
662667
// TODO(log)
663668
return MessageMediaContainer(onTap: null, child: child);
@@ -670,7 +675,9 @@ class MessageImagePreview extends StatelessWidget {
670675
message: message,
671676
messageImageContext: context,
672677
src: lightboxDisplayUrl,
673-
thumbnailUrl: resolvedThumbnailUrl,
678+
thumbnailUrl: (node.src is ImagePreviewNodeSrcThumbnail && !node.loading)
679+
? resolvedSrc
680+
: null,
674681
originalWidth: node.originalWidth,
675682
originalHeight: node.originalHeight));
676683
},

0 commit comments

Comments
 (0)