[DecorationImage] adds scale property #54258

AyushBherwani1998 · 2020-04-08T07:07:04Z

Description

Adds scale property to DecorationImage. The scale property is then passed to paintImage function to scale the image.

If users want to scale the DecorationImage they use the scale property now.

Screenshot

Related Issues

#12452

Tests

I have added the test case to check assertion error for scale. To test the scale property is working properly or not, there are already test cases written for paintImage function.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: Replace with a link to your migration guide

shihaohong · 2020-04-08T16:51:27Z

packages/flutter/test/painting/decoration_test.dart

+      expect(error.toString(), contains('is not true'));
+      return;
+    }
+    fail('DecorationImage didnot throw AssertionError when scale was null');


Suggested change

fail('DecorationImage didnot throw AssertionError when scale was null');

fail('DecorationImage did not throw AssertionError when scale was null');

shihaohong · 2020-04-08T16:52:14Z

packages/flutter/lib/src/painting/decoration_image.dart

@@ -175,6 +183,7 @@ class DecorationImage {
        '$repeat',
      if (matchTextDirection)
        'match text direction',
+      '$scale'


Suggested change

'$scale'

'scale: $scale'

shihaohong · 2020-04-08T16:52:20Z

packages/flutter/test/painting/decoration_test.dart

@@ -279,7 +279,7 @@ void main() {
      '   direction provided in the ImageConfiguration object to match.\n'
      '   The DecorationImage was:\n'
      '     DecorationImage(SynchronousTestImageProvider(), center, match\n'
-      '     text direction)\n'
+      '     text direction, 1.0)\n'


I think it's weird that it just says 1.0. It should definitely say something like scale: 1.0.

Makes sense, the above change you suggested will resolve this. I'll do the changes.

shihaohong · 2020-04-08T17:35:36Z

packages/flutter/lib/src/painting/decoration_image.dart

@@ -129,6 +131,11 @@ class DecorationImage {
  /// in the top right.
  final bool matchTextDirection;

+  /// Defines image pixels to be shown per logical pixels


Suggested change

/// Defines image pixels to be shown per logical pixels

/// Defines image pixels to be shown per logical pixels.

shihaohong · 2020-04-08T17:35:50Z

packages/flutter/lib/src/painting/decoration_image.dart

@@ -129,6 +131,11 @@ class DecorationImage {
  /// in the top right.
  final bool matchTextDirection;

+  /// Defines image pixels to be shown per logical pixels
+  ///
+  /// By default the the value of scale is 1.0


Suggested change

/// By default the the value of scale is 1.0

/// By default, the value of scale is 1.0.

shihaohong · 2020-04-08T17:41:42Z

packages/flutter/test/painting/decoration_test.dart

@@ -583,4 +583,15 @@ void main() {
      expect(call.positionalArguments[2].center, outputRect.center);
    }
  });
+


You should also add a scale test similar to this paintImage scale test. The test in particular tests paintImage, but you can combine elements from this BoxDecoration test with the scale test for paintImage to achieve the outcome you need.

goderbauer · 2020-04-08T22:28:32Z

packages/flutter/lib/src/painting/decoration_image.dart

@@ -263,7 +272,7 @@ class DecorationImagePainter {
      canvas: canvas,
      rect: rect,
      image: _image.image,
-      scale: _image.scale,
+      scale: _details.scale,


It seems odd to me that we now completely ignore the scale of the actual image.

/cc @dnfield

This is almost certainly wrong, and unfortunately it doesn't look like we have a test covering it.

I would expect this to give you strange results if you now tried to use a DecorationImage with an AssetImage of a non-1.0 scale.

@dnfield @goderbauer I understand the issue with this approach. It completely ignores the scale given to NetworkImage and ExactAssetImage.

But, if we are using scale property of ImageInfo instead of DecorationImage, we will have to create a new ImageProvider before creating an ImageStream, which means we would have to overwrite the scale of ImageInfo with DecorationImage, which we are doing in the current approach.

What if we document this behaviour in the API docs?

Also, let me know if there is a different approach that we can try here.

I think if we just multiplied the scale property from the decoration against the image scale, it should work out.

We should also add some tests that show that that works ok with and without this parameter.

And we should document this behavior - someone could then override it by wrapping the incoming provider with a new provider that sets the scale to 1.0.

@dnfield I'll do the required changes. I'll add the test which shows that it works properly with scale property. We already have test cases which don't use scale property, example

…ration_image

AyushBherwani1998 · 2020-04-18T19:58:12Z

@dnfield should I add a option to change the scale of SynchronousTestImageProvider? It will give us a more accurate test. WDYT?

dnfield

LGTM

fluttergithubbot · 2020-04-18T20:55:01Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite hostonly_devicelab_tests-0-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

adds scale property to DecorationImage

3458ebc

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Apr 8, 2020

googlebot added the cla: yes label Apr 8, 2020

fixes test

5df8e74

shihaohong reviewed Apr 8, 2020

View reviewed changes

AyushBherwani1998 requested a review from shihaohong April 8, 2020 17:22

shihaohong reviewed Apr 8, 2020

View reviewed changes

goderbauer reviewed Apr 8, 2020

View reviewed changes

AyushBherwani1998 added 2 commits April 17, 2020 09:59

Merge branch 'master' of https://github.com/flutter/flutter into deco…

1aacbba

…ration_image

improves test case and how scale is caluculated

b327519

AyushBherwani1998 requested a review from dnfield April 18, 2020 19:54

dnfield approved these changes Apr 18, 2020

View reviewed changes

dnfield added the waiting for tree to go green label Apr 18, 2020

fluttergithubbot removed the waiting for tree to go green label Apr 18, 2020

dnfield added the waiting for tree to go green label Apr 18, 2020

fluttergithubbot merged commit f35b673 into flutter:master Apr 18, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DecorationImage] adds scale property #54258

[DecorationImage] adds scale property #54258

AyushBherwani1998 commented Apr 8, 2020 •

edited

shihaohong Apr 8, 2020

shihaohong Apr 8, 2020

shihaohong Apr 8, 2020

AyushBherwani1998 Apr 8, 2020

shihaohong Apr 8, 2020

shihaohong Apr 8, 2020

shihaohong Apr 8, 2020

goderbauer Apr 8, 2020

dnfield Apr 8, 2020

AyushBherwani1998 Apr 9, 2020 •

edited

dnfield Apr 16, 2020

dnfield Apr 16, 2020

dnfield Apr 16, 2020

AyushBherwani1998 Apr 18, 2020

AyushBherwani1998 commented Apr 18, 2020

dnfield left a comment

fluttergithubbot commented Apr 18, 2020

	fail('DecorationImage didnot throw AssertionError when scale was null');
	fail('DecorationImage did not throw AssertionError when scale was null');

	/// Defines image pixels to be shown per logical pixels
	/// Defines image pixels to be shown per logical pixels.

	/// By default the the value of scale is 1.0
	/// By default, the value of scale is 1.0.

[DecorationImage] adds scale property #54258

[DecorationImage] adds scale property #54258

Conversation

AyushBherwani1998 commented Apr 8, 2020 • edited

Description

Related Issues

Tests

Checklist

Breaking Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AyushBherwani1998 Apr 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AyushBherwani1998 commented Apr 18, 2020

dnfield left a comment

Choose a reason for hiding this comment

fluttergithubbot commented Apr 18, 2020

AyushBherwani1998 commented Apr 8, 2020 •

edited

AyushBherwani1998 Apr 9, 2020 •

edited