Skip to content

lightbox: Display thumbnail with original image sizing #833

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 3 commits into from
Jul 26, 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
43 changes: 41 additions & 2 deletions lib/model/content.dart
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,8 @@ class ImageNode extends BlockContentNode {
required this.srcUrl,
required this.thumbnailUrl,
required this.loading,
required this.originalWidth,
required this.originalHeight,
});

/// The canonical source URL of the image.
Expand All @@ -379,23 +381,34 @@ class ImageNode extends BlockContentNode {
/// Typically it will be `true` while Server is generating thumbnails.
final bool loading;

/// The width of the canonical image.
final double? originalWidth;

/// The height of the canonical image.
final double? originalHeight;

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

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

@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"));
properties.add(DoubleProperty('originalWidth', originalWidth));
properties.add(DoubleProperty('originalHeight', originalHeight));
}
}

Expand Down Expand Up @@ -1037,6 +1050,8 @@ class _ZulipContentParser {
return CodeBlockNode(spans, debugHtmlNode: debugHtmlNode);
}

static final _imageDimensionsRegExp = RegExp(r'^(\d+)x(\d+)$');

BlockContentNode parseImageNode(dom.Element divElement) {
assert(_debugParserContext == _ParserContext.block);
final elements = () {
Expand Down Expand Up @@ -1071,6 +1086,8 @@ class _ZulipContentParser {
srcUrl: href,
thumbnailUrl: null,
loading: true,
originalWidth: null,
originalHeight: null,
debugHtmlNode: debugHtmlNode);
}
final src = imgElement.attributes['src'];
Expand All @@ -1093,10 +1110,32 @@ class _ZulipContentParser {
} else {
return UnimplementedBlockContentNode(htmlNode: divElement);
}

double? originalWidth, originalHeight;
final originalDimensions = imgElement.attributes['data-original-dimensions'];
if (originalDimensions != null) {
// Server encodes this string as "{width}x{height}" (eg. "300x400")
final match = _imageDimensionsRegExp.firstMatch(originalDimensions);
if (match != null) {
final width = int.tryParse(match.group(1)!, radix: 10);
final height = int.tryParse(match.group(2)!, radix: 10);
if (width != null && height != null) {
originalWidth = width.toDouble();
originalHeight = height.toDouble();
}
}

if (originalWidth == null || originalHeight == null) {
return UnimplementedBlockContentNode(htmlNode: divElement);
}
}

return ImageNode(
srcUrl: srcUrl,
thumbnailUrl: thumbnailUrl,
loading: false,
originalWidth: originalWidth,
originalHeight: originalHeight,
debugHtmlNode: debugHtmlNode);
}

Expand Down
11 changes: 5 additions & 6 deletions lib/widgets/content.dart
Original file line number Diff line number Diff line change
Expand Up @@ -572,12 +572,13 @@ class MessageImage extends StatelessWidget {

return MessageMediaContainer(
onTap: resolvedSrcUrl == null ? null : () { // TODO(log)
Navigator.of(context).push(getLightboxRoute(
Navigator.of(context).push(getImageLightboxRoute(
context: context,
message: message,
src: resolvedSrcUrl,
thumbnailUrl: resolvedThumbnailUrl,
mediaType: MediaType.image));
originalWidth: node.originalWidth,
originalHeight: node.originalHeight));
},
child: node.loading
? const CupertinoActivityIndicator()
Expand All @@ -603,12 +604,10 @@ class MessageInlineVideo extends StatelessWidget {

return MessageMediaContainer(
onTap: resolvedSrc == null ? null : () { // TODO(log)
Navigator.of(context).push(getLightboxRoute(
Navigator.of(context).push(getVideoLightboxRoute(
context: context,
message: message,
src: resolvedSrc,
thumbnailUrl: null,
mediaType: MediaType.video));
src: resolvedSrc));
Comment on lines -609 to +610
Copy link
Member

Choose a reason for hiding this comment

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

Cool, I like this refactor splitting into two different entry-point functions. The resulting API feels more thoroughly well-typed.

},
child: Container(
color: Colors.black, // Web has the same color in light and dark mode.
Expand Down
88 changes: 58 additions & 30 deletions lib/widgets/lightbox.dart
Original file line number Diff line number Diff line change
Expand Up @@ -217,12 +217,16 @@ class _ImageLightboxPage extends StatefulWidget {
required this.message,
required this.src,
required this.thumbnailUrl,
required this.originalWidth,
required this.originalHeight,
});

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

@override
State<_ImageLightboxPage> createState() => _ImageLightboxPageState();
Expand Down Expand Up @@ -259,8 +263,14 @@ class _ImageLightboxPageState extends State<_ImageLightboxPage> {
if (frame != null) return child;

// Display the thumbnail image while original image is downloading.
return RealmContentNetworkImage(widget.thumbnailUrl!,
filterQuality: FilterQuality.medium);
return FittedBox(
fit: BoxFit.scaleDown,
child: SizedBox(
width: widget.originalWidth,
height: widget.originalHeight,
child: RealmContentNetworkImage(widget.thumbnailUrl!,
Comment on lines +266 to +271
Copy link
Member

Choose a reason for hiding this comment

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

Cool, this seems nice and clean.

filterQuality: FilterQuality.medium,
fit: BoxFit.contain)));
}

Widget _loadingBuilder(BuildContext context, Widget child, ImageChunkEvent? loadingProgress) {
Expand Down Expand Up @@ -301,11 +311,7 @@ class _ImageLightboxPageState extends State<_ImageLightboxPage> {
child: RealmContentNetworkImage(widget.src,
filterQuality: FilterQuality.medium,
frameBuilder: _frameBuilder,
loadingBuilder: _loadingBuilder),
),
),
),
));
loadingBuilder: _loadingBuilder))))));
}
}

Expand Down Expand Up @@ -536,18 +542,10 @@ class _VideoLightboxPageState extends State<VideoLightboxPage> with PerAccountSt
}
}

enum MediaType {
video,
image
}

Route<void> getLightboxRoute({
int? accountId,
BuildContext? context,
required Message message,
required Uri src,
required Uri? thumbnailUrl,
required MediaType mediaType,
Route<void> _getLightboxRoute({
required int? accountId,
required BuildContext? context,
required RoutePageBuilder pageBuilder,
}) {
return AccountPageRouteBuilder(
accountId: accountId,
Expand All @@ -559,17 +557,7 @@ Route<void> getLightboxRoute({
Animation<double> secondaryAnimation,
) {
// TODO(#40): Drag down to close?
return switch (mediaType) {
MediaType.image => _ImageLightboxPage(
routeEntranceAnimation: animation,
message: message,
src: src,
thumbnailUrl: thumbnailUrl),
MediaType.video => VideoLightboxPage(
routeEntranceAnimation: animation,
message: message,
src: src),
};
return pageBuilder(context, animation, secondaryAnimation);
},
transitionsBuilder: (
BuildContext context,
Expand All @@ -583,3 +571,43 @@ Route<void> getLightboxRoute({
},
);
}

Route<void> getImageLightboxRoute({
int? accountId,
BuildContext? context,
required Message message,
required Uri src,
required Uri? thumbnailUrl,
Copy link
Member

Choose a reason for hiding this comment

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

nit: include trailing comma in intermediate commit

(that helps reduce the diff both for that commit and the one after it)

required double? originalWidth,
required double? originalHeight,
}) {
return _getLightboxRoute(
accountId: accountId,
context: context,
pageBuilder: (context, animation, secondaryAnimation) {
return _ImageLightboxPage(
routeEntranceAnimation: animation,
message: message,
src: src,
thumbnailUrl: thumbnailUrl,
originalWidth: originalWidth,
originalHeight: originalHeight);
});
}

Route<void> getVideoLightboxRoute({
int? accountId,
BuildContext? context,
required Message message,
required Uri src,
}) {
return _getLightboxRoute(
accountId: accountId,
context: context,
pageBuilder: (context, animation, secondaryAnimation) {
return VideoLightboxPage(
routeEntranceAnimation: animation,
message: message,
src: src);
});
}
Loading