[google_sign_in] Use a transparent image as a placeholder for the `GoogleUserCircleAvatar` #4391

ValentinVignal · 2021-09-28T15:29:39Z

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

Use a valid transparent 1 x 1 image for the placeholder of the GoogleUserCircleAvatar (Uint8List((size.round() * size.round())), is not a valid image).

List which issues are fixed by this PR. You must list at least one issue.

flutter/flutter#88329
flutter/flutter#55639

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

google-cla · 2021-09-28T15:29:51Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

…ilds

ValentinVignal · 2021-09-30T14:06:05Z

I'm not proud of the test I have written at all. It works though, this is a failing test without the changes I have made:

00:02 +0: It should build th GoogleUserCircleAvatar successfully                                                                                       
photoUrl photoUrl
══╡ EXCEPTION CAUGHT BY IMAGE RESOURCE SERVICE ╞════════════════════════════════════════════════════
The following _Exception was thrown resolving an image codec:
Exception: Invalid image data

When the exception was thrown, this was the stack:
#0      _futurize (dart:ui/painting.dart:5502:5)
#1      ImageDescriptor.encoded (dart:ui/painting.dart:5370:12)
#2      instantiateImageCodec (dart:ui/painting.dart:2053:60)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)
════════════════════════════════════════════════════════════════════════════════════════════════════
00:02 +0 -1: It should build th GoogleUserCircleAvatar successfully [E]                                                                                
  Test failed. See exception logs above.
  The test description was: It should not raise an exception

I had to override the HttpClient and it takes 500 lines... I took the code from https://github.com/flutter/flutter/blob/master/packages/flutter_test/lib/src/_binding_io.dart

I set _transparentImage as a private variable because I guess we don't want to export it.
But because of that, I had to create this variable twice: one time in lib/widgets.dart and one time in test/widgets_test.dart.

How can I improve what I have done?

stuartmorgan · 2021-10-25T14:46:53Z

packages/google_sign_in/google_sign_in/lib/widgets.dart

+    68,
+    1,
+    0,
+    59,


Nit: data bytes are generally written as fixed-width hex (e.g., 0x00 rather than 0, 0xFF rather than 255)

I've changed it in a54232d

stuartmorgan · 2021-10-25T14:48:39Z

packages/google_sign_in/google_sign_in/test/widgets_test.dart

+  _TestGoogleIdentity({
+    required this.id,
+    required this.email,
+    this.displayName,


Why is there a constructor argument that is never used?

Sorry for that, I removed it in 9a9d6d4

stuartmorgan · 2021-10-25T14:53:45Z

packages/google_sign_in/google_sign_in/test/widgets_test.dart

+}
+
+/// A mocked [HttpClient] which always returns a [_MockHttpRequest].
+class _MockHttpClient implements HttpClient {


class _MockHttpClient extends Fake implements HttpClient would eliminate quite a bit of code here.

I used Fake in b18a8e2

stuartmorgan · 2021-10-25T14:55:33Z

packages/google_sign_in/google_sign_in/test/widgets_test.dart

+
+/// A mocked [HttpClientResponse] which is empty and has a [statusCode] of 200
+/// and returns a 1x1 transparent image.
+class _MockHttpResponse implements HttpClientResponse {


Fake seems very likely to help here as well.

I used Fake in b18a8e2

packages/google_sign_in/google_sign_in/test/widgets_test.dart

…mage # Conflicts: # packages/google_sign_in/google_sign_in/CHANGELOG.md # packages/google_sign_in/google_sign_in/pubspec.yaml

…serverAuthCode in test

stuartmorgan

Minor nits, but otherwise looks good now. Thanks!

stuartmorgan · 2021-10-27T19:31:34Z

packages/google_sign_in/google_sign_in/lib/widgets.dart

+/// This is an transparent 1x1 gif image
+final Uint8List _transparentImage = Uint8List.fromList(
+  [
+    0x47,


You can improve these data blobs by adding a // somewhere and then removing most of the newlines.

E.g.:

final Uint8List _transparentImage = Uint8List.fromList( [ 0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x01, 0x00, 0x01, 0x00, 0x80, 0x00, // 0x00, 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00, 0x21, 0xF9, 0x04, 0x01, 0x00, 0x00, 0x00, 0x00, 0x2C, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x01, 0x00, ...

I've formatter the data blobs in 166822e

stuartmorgan · 2021-10-27T19:31:55Z

packages/google_sign_in/google_sign_in/test/widgets_test.dart

+/// This is an transparent 1x1 gif image
+final Uint8List _transparentImage = Uint8List.fromList(
+  [
+    0x47,


Same formatting note here.

I've formatter the data blobs in 166822e

stuartmorgan · 2021-10-27T19:32:13Z

packages/google_sign_in/google_sign_in/lib/widgets.dart

              image: sizedPhotoUrl,
            )
          ]),
        ));
  }
 }
+
+/// This is an transparent 1x1 gif image


Nit: should have a "." at the end of the comment.

I added the "." in 03c48da

stuartmorgan · 2021-10-27T19:32:58Z

packages/google_sign_in/google_sign_in/test/widgets_test.dart

+  }
+}
+
+/// This is an transparent 1x1 gif image


Please comment that this is just an arbitrary valid image, and that it's not important that it match the placeholder, since that confused me.

I've tried to add a better comment in 03c48da

stuartmorgan · 2021-10-27T19:33:53Z

packages/google_sign_in/google_sign_in/test/widgets_test.dart

+);
+
+/// A mocked [HttpClientResponse] which is empty and has a [statusCode] of 200
+/// and returns a 1x1 transparent image.


s/1x1 transparent/valid/ since that's what matters.

I changed it in 03c48da

stuartmorgan · 2021-10-27T19:34:16Z

packages/google_sign_in/google_sign_in/test/widgets_test.dart

+}
+
+void main() {
+  testWidgets('It should build th GoogleUserCircleAvatar successfully',


I have fixed the typo in bf0f524

stuartmorgan · 2021-10-27T19:35:21Z

Adding @gaaclarke for secondary review.

gaaclarke · 2021-10-27T21:40:22Z

packages/google_sign_in/google_sign_in/lib/widgets.dart

              image: sizedPhotoUrl,
            )
          ]),
        ));
  }
 }
+
+/// This is an transparent 1x1 gif image
+final Uint8List _transparentImage = Uint8List.fromList(


You should check in a script that generates this code and add a comment here pointing to it.

I'm sorry, I'm a bit confused and I don't understand what you want me to do?

You should check in a script that generates this code

Why? Under what circumstance would the bytes of a static image need to be re-generated?

Presumably a script was already written to generate it. Who knows, maybe there is a bug where the pixel is #000000FF but we need #FFFFFFFF, depending on alpha blending that can make a difference. Plus it means people don't need to execute the code to know it is correct. I don't know the gif header format.

Alternately, we could compare it byte by byte to the source, which would be significantly less work than reviewing (and then maintaining) a script.

@ValentinVignal Can you add a comment with the URL of the source gif you used?

Good idea. So let's land the gif (maybe in a new package-level directory with a README explaining why it's there), and reference it in a comment in the code.

Hello, I'm sorry again, I'm still not clear on what I should do exactly. Let me try to reformulate and you tell me if I'm wrong?

I should:

Create a new package in plugin/packages/<new package>

Add the gif image as an asset and this variable transparentImage (not private)

Import this package in google_sign_in to reuse the variable transparentImage

Write a test in the new repo to read the bytes of the gif image and compare them to the ones in the variable

Did I miss something?

No, you just need to add the source GIF to this PR. My suggestion is that it be in a subdirectory (e.g., resources/) with a README, rather than being at the top level of the plugin.

@stuartmorgan @gaaclarke I couldn't remember where I took the image in the first place.

In 4d17090, I used this one:

from this wikimedia page.

I added the image in the repo as resources/transparentImage.gif and updated the bytes of _transparentImage to match the gif file.

I got the bytes by doing:

final bytes = await File('resources/transparentImage.gif').readAsBytes();

Is it what you were requesting?

Yep, that sounds good to me, thanks!

stuartmorgan

LGTM

@gaaclarke can do the second review

gaaclarke

LGTM modulo the code to generate the data blob. Without demonstrating the generation of the bytes I don't know if it really is 1x1 pixel or what the color of that pixel is.

stuartmorgan

Changes LGTM

… the `GoogleUserCircleAvatar` (flutter/plugins#4391)

* master: [webview_flutter] Deprecate evaluateJavascript in favour of runJavaScript and runJavaScriptForResult. (flutter#4403) [webview_flutter] Add interface methods to load local files and HTML strings. (flutter#4446) [ci] add pedantic dep to new in_app_purchase pkgs (flutter#4471) [ci] Remove unused dep from file_selector. (flutter#4468) [ci] update build_runner dep in android_intent and file_selector example (flutter#4467) [webview_flutter] Add platform interface method `loadRequest`. (flutter#4450) Remove -Werror from deprecated plugin Android builds (flutter#4466) [webview_flutter] Update webview packages for Android and iOS to implement `runJavascript` and `runJavascriptReturningResult`. (flutter#4402) [camera] Fix CamcorderProfile Usages (flutter#4423) [google_sign_in] Use a transparent image as a placeholder for the `GoogleUserCircleAvatar` (flutter#4391) [in_app_purchase]IAP/platform interface add cancel status (flutter#4093) [image_picker] doc:readme image picker misprints (flutter#4421) Support Hybrid Composition through the GoogleMaps Widget (flutter#4082) [path_provider] started supporting background platform channels (flutter#4443) [device_info] started using Background Platform Channels (flutter#4456) # Conflicts: # packages/webview_flutter/webview_flutter/CHANGELOG.md # packages/webview_flutter/webview_flutter/pubspec.yaml

…ogleUserCircleAvatar` (flutter#4391)

google-cla bot added the cla: no label Sep 28, 2021

github-actions bot added the p: google_sign_in label Sep 28, 2021

🐛 Use a transparent image as a placeholder

dd83135

ValentinVignal force-pushed the use-transparent-image branch from 1b15fe4 to dd83135 Compare September 28, 2021 15:30

google-cla bot added cla: yes and removed cla: no labels Sep 28, 2021

ValentinVignal added 2 commits September 30, 2021 20:23

🔖 Modify version in pubspec and CHANGELOG

4ffd27f

✅ Write a test to make sur the GoogleUserCircleAvatar successfully bu…

8008688

…ilds

ValentinVignal marked this pull request as ready for review September 30, 2021 13:55

🎨 _kTransparentImage -> _transparentImage as it is not a const

8048451

📄 Add license block to test/widgets_test.dart

75725ef

stuartmorgan self-requested a review September 30, 2021 20:07

stuartmorgan suggested changes Oct 25, 2021

View reviewed changes

ValentinVignal added 3 commits October 26, 2021 22:42

🏷️ Use fixed-width hex

a54232d

🔥 Remove unused constructor parameter

9a9d6d4

♻️ Use Fake

b18a8e2

ValentinVignal requested a review from stuartmorgan October 26, 2021 15:07

ValentinVignal added 2 commits October 26, 2021 23:10

Merge remote-tracking branch 'upstream/master' into use-transparent-i…

d25857f

…mage # Conflicts: # packages/google_sign_in/google_sign_in/CHANGELOG.md # packages/google_sign_in/google_sign_in/pubspec.yaml

🔀 Fix merge conflict with master. Update version number and override …

316afbf

…serverAuthCode in test

stuartmorgan suggested changes Oct 27, 2021

View reviewed changes

stuartmorgan requested a review from gaaclarke October 27, 2021 19:35

gaaclarke reviewed Oct 27, 2021

View reviewed changes

ValentinVignal added 4 commits October 28, 2021 20:28

🎨 Use comment to format data blobs

166822e

💡 Improve comment of _transparentImage

03c48da

✏️ Fix typo in test description

bf0f524

💡 Add another forgotten "."

1d2638a

stuartmorgan approved these changes Oct 28, 2021

View reviewed changes

stuartmorgan added the last mile label Oct 28, 2021

gaaclarke approved these changes Oct 28, 2021

View reviewed changes

ValentinVignal and others added 3 commits October 30, 2021 00:46

🍱 Add resources/transparentImage.gif

4d17090

Fix comment

6c69b63

Fix comment

3291d1b

stuartmorgan approved these changes Oct 29, 2021

View reviewed changes

stuartmorgan added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Oct 29, 2021

fluttergithubbot merged commit 7588dd9 into flutter:master Oct 29, 2021

engine-flutter-autoroll mentioned this pull request Oct 29, 2021

Roll Plugins from e51cc1df7b75 to 7588dd9cfe0b (1 revision) flutter/flutter#92726

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 29, 2021

7588dd9 [google_sign_in] Use a transparent image as a placeholder for…

40b3bdb

… the `GoogleUserCircleAvatar` (flutter/plugins#4391)

engine-flutter-autoroll mentioned this pull request Nov 3, 2021

Roll Plugins from e51cc1df7b75 to db098ed8408d (6 revisions) flutter/flutter#92949

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2021

7588dd9 [google_sign_in] Use a transparent image as a placeholder for…

2091382

… the `GoogleUserCircleAvatar` (flutter/plugins#4391)

engine-flutter-autoroll mentioned this pull request Nov 3, 2021

Roll Plugins from e51cc1df7b75 to 53fff22979d2 (7 revisions) flutter/flutter#92954

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2021

7588dd9 [google_sign_in] Use a transparent image as a placeholder for…

840da41

… the `GoogleUserCircleAvatar` (flutter/plugins#4391)

amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021

[google_sign_in] Use a transparent image as a placeholder for the `Go…

894d16a

…ogleUserCircleAvatar` (flutter#4391)

KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021

[google_sign_in] Use a transparent image as a placeholder for the `Go…

cb8f42b

…ogleUserCircleAvatar` (flutter#4391)

+,
+,
+,
+,

[google_sign_in] Use a transparent image as a placeholder for the GoogleUserCircleAvatar #4391

[google_sign_in] Use a transparent image as a placeholder for the GoogleUserCircleAvatar #4391

Conversation

ValentinVignal commented Sep 28, 2021 • edited Loading

Pre-launch Checklist

google-cla bot commented Sep 28, 2021

ValentinVignal commented Sep 30, 2021 • edited Loading

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

stuartmorgan left a 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

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

stuartmorgan commented Oct 27, 2021

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

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

[google_sign_in] Use a transparent image as a placeholder for the `GoogleUserCircleAvatar` #4391

[google_sign_in] Use a transparent image as a placeholder for the `GoogleUserCircleAvatar` #4391

ValentinVignal commented Sep 28, 2021 •

edited

Loading

ValentinVignal commented Sep 30, 2021 •

edited

Loading