-
Notifications
You must be signed in to change notification settings - Fork 310
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
Conversation
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.
Great, thanks @rajveermalviya for building this!
About to go into a meeting, but here's a couple of comments from beginning to read the PR.
lib/model/content.dart
Outdated
final width = double.tryParse(cutsets[0]); | ||
final height = double.tryParse(cutsets[1]); |
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.
This seems a bit too widely accepting — let's expect ints rather than doubles.
lib/model/content.dart
Outdated
final originalDimensions = imgElement.attributes['data-original-dimensions']; | ||
if (originalDimensions != null) { | ||
// Server encodes this string as "{width}x{height}" (eg. "300x400") | ||
final cutsets = originalDimensions.split('x'); |
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.
As part of my usual performance paranoia for the content parser, let's avoid split
and allocating a list here: instead can find the index of the first 'x' and parse the substrings before and after (if there's a second x
, it'll cause the int parsing to fail), or can use a regexp like ^(\d+)x(\d+)$
.
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.
OK, finished a review — comments below.
lib/model/content.dart
Outdated
final bool loading; | ||
|
||
|
||
/// The width of the canonical image. |
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.
nit: extra blank line
BuildContext? context, | ||
required Message message, | ||
required Uri src, | ||
required Uri? thumbnailUrl, |
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.
nit: include trailing comma in intermediate commit
(that helps reduce the diff both for that commit and the one after it)
src: resolvedSrc, | ||
thumbnailUrl: null, | ||
mediaType: MediaType.video)); | ||
src: resolvedSrc)); |
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.
Cool, I like this refactor splitting into two different entry-point functions. The resulting API feels more thoroughly well-typed.
lib/widgets/lightbox.dart
Outdated
// Use `BoxFit.contain` if the original image size is larger than | ||
// the device screen size, otherwise `null` (default). |
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.
What's the behavior of the default? If we can it's probably best to pass an explicit value that specifies the behavior we want — if nothing else, that should make it easier to reason about this logic where we're choosing between different values for this option.
lib/widgets/lightbox.dart
Outdated
final physicalSize = | ||
MediaQuery.sizeOf(context) * MediaQuery.devicePixelRatioOf(context); |
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.
Hmm, the meaning of MediaQuery.sizeOf
is that it gives the size of the whole screen.
That's often the same as the size available to this widget, but it may not be — in particular we have a SafeArea
around this, so I believe if you use a device with a notched display then this will give a larger size than what's actually available to this widget. That seems like it could lead the below logic astray.
Ideally we'd specify a single value for this option that tells Image
to do the same thing that you explain in the comment above. Is this BoxFit.scaleDown
?
https://main-api.flutter.dev/flutter/painting/BoxFit.html
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.
Ah I see, but then it's not clear there's a good way to tell Image
about originalWidth
and originalHeight
.
In that case, how about we handle the sizing of the image directly? I think putting it in a SizedBox inside a FittedBox, or something along those lines, should be able to control the size it's displayed at.
lib/widgets/lightbox.dart
Outdated
// This allows the thumbnail image to mimic the same behaviour as | ||
// when the original image is loaded, which is, keep small images | ||
// small (and in the center) and large images contained in the | ||
// viewport. |
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 add a test case or two for this behavior, since it seems like nontrivial logic.
I see we don't have any existing tests for the image lightbox. We don't have to add tests for the rest of its behavior now; but it'd be good to test this, and in particular to have a test exercising the situation I mentioned in another comment with a notched display.
If I find time today I can try to add a couple of basic tests for the existing layout of the image lightbox, to help set the framework for the new tests.
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.
Sent that as #835. So you can rebase this PR atop that one, and then add the new couple of test cases.
7e83ad1
to
6956b80
Compare
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 for the revision!
I see you hadn't marked this as ready for the next round of review, but took a look at it anyway just to help keep it moving along given its connection to the imminent server-9.0 release.
One small comment below, and otherwise everything here looks good — all it needs is those couple of new test cases as discussed above.
lib/model/content.dart
Outdated
final width = int.tryParse(match.group(1)!); | ||
final height = int.tryParse(match.group(2)!); |
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.
There's a subtle and unfortunate gotcha in Dart's int.parse
and int.tryParse
, which it's best to explicitly disable:
final width = int.tryParse(match.group(1)!); | |
final height = int.tryParse(match.group(2)!); | |
final width = int.tryParse(match.group(1)!, radix: 10); | |
final height = int.tryParse(match.group(2)!, radix: 10); |
For discussion, see f547771.
In this case I believe it doesn't have any effect on the behavior, because we already know from the regexp that each group matches exactly \d+
and so can't trigger this gotcha. But that's a pretty subtle and nonlocal property, so it's best to avoid the gotcha more directly too: that way (a) the gotcha won't easily be reintroduced by an otherwise-innocent change elsewhere in this logic, and (b) it maintains the pattern for the benefit of anyone copy-pasting or adapting this code for somewhere else in the codebase.
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.
(Separately I was tempted to say tryParse
can be just parse
, because we already know they match exactly \d+
. But if we got an image with something like data-original-dimensions="1234123412341234123412341234x5"
, the parse would indeed fail because the number is too big for an int. So tryParse
is more correct — better to return unimplemented than to throw — and it's good we have it.)
return FittedBox( | ||
fit: BoxFit.scaleDown, | ||
child: SizedBox( | ||
width: widget.originalWidth, | ||
height: widget.originalHeight, | ||
child: RealmContentNetworkImage(widget.thumbnailUrl!, |
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.
Cool, this seems nice and clean.
6956b80
to
ec4078d
Compare
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 for the review @gnprice, I've addressed your comments in the latest revision.
But haven't gotten tests to work correctly yet.
test/widgets/lightbox_test.dart
Outdated
// final renderImage = tester.renderObject<RenderImage>(find.descendant( | ||
// of: find.byType(RealmContentNetworkImage), | ||
// matching: find.byType(RawImage))); | ||
// check(renderImage.size).equals(const Size(100, 100)); |
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.
I can't get this test to pass because seems like flutter test enviroment doesn't report correct image sizes?
test/widgets/lightbox_test.dart
Outdated
// print('pump'); | ||
// await tester.pump(); | ||
// print('done pump'); | ||
// tester.widgetList<RealmContentNetworkImage>(find.byType(RealmContentNetworkImage)).forEach((w) => print(w.src)); |
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.
Same for this test, the frame arg in _frameBuilder
never changes from null
, even though _loadingBuilder
's progress
become 1.0.
Thanks. Chat thread for debugging those tests: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23F833.20lightbox.20tests/near/1902807 |
OK, merging this without those tests, in order to get it into a v0.0.18 beta today since the server 9.0 release just came out. Thanks again @rajveermalviya! I made a couple of commit-message tweaks:
The second commit can be a bit more specific about what changed, since "[nfc]" already means it was a refactor; and for the first, the summary prefix can be more specific in that we usually mark a change to lib/model/foo.dart as "foo:" rather than "model:". I definitely appreciate the work you've done already to write those draft tests, and would be glad to get tests along those lines into the tree. So please go ahead and send that as a follow-up PR, using what we found in that chat thread linked above. (And maybe we still won't have a test for the situation where the frame argument is never changing from null, since that remains mysterious. In that case we can leave a TODO comment for it, perhaps including a draft of the test just with |
ec4078d
to
4efcaa7
Compare
I've had a tab in my browser open to the end of this #833 thread. Here seems like the right place to serialize out that information so that I can close the tab.
flutter-thumbnails-sizing.mp4
(Initial image opens are older messages missing
data-original-dimensions
attribute)Fixes #830