Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

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

Merged
merged 17 commits into from
Oct 29, 2021
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
dd83135
:bug: Use a transparent image as a placeholder
ValentinVignal Sep 28, 2021
4ffd27f
:bookmark: Modify version in pubspec and CHANGELOG
ValentinVignal Sep 30, 2021
8008688
:white_check_mark: Write a test to make sur the GoogleUserCircleAvata…
ValentinVignal Sep 30, 2021
8048451
:art: _kTransparentImage -> _transparentImage as it is not a const
ValentinVignal Sep 30, 2021
75725ef
:page_facing_up: Add license block to test/widgets_test.dart
ValentinVignal Sep 30, 2021
a54232d
🏷️ Use fixed-width hex
ValentinVignal Oct 26, 2021
9a9d6d4
:fire: Remove unused constructor parameter
ValentinVignal Oct 26, 2021
b18a8e2
:recycle: Use Fake
ValentinVignal Oct 26, 2021
d25857f
Merge remote-tracking branch 'upstream/master' into use-transparent-i…
ValentinVignal Oct 26, 2021
316afbf
🔀 Fix merge conflict with master. Update version number and override…
ValentinVignal Oct 26, 2021
166822e
:art: Use comment to format data blobs
ValentinVignal Oct 28, 2021
03c48da
:bulb: Improve comment of _transparentImage
ValentinVignal Oct 28, 2021
bf0f524
:pencil2: Fix typo in test description
ValentinVignal Oct 28, 2021
1d2638a
:bulb: Add another forgotten "."
ValentinVignal Oct 28, 2021
4d17090
:bento: Add resources/transparentImage.gif
ValentinVignal Oct 29, 2021
6c69b63
Fix comment
stuartmorgan Oct 29, 2021
3291d1b
Fix comment
stuartmorgan Oct 29, 2021
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
4 changes: 4 additions & 0 deletions packages/google_sign_in/google_sign_in/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 5.1.2

Change the placeholder of the GoogleUserCircleAvatar to a transparent image.

## 5.1.1

* Update minimum Flutter SDK to 2.5 and iOS deployment target to 9.0.
Expand Down
51 changes: 50 additions & 1 deletion packages/google_sign_in/google_sign_in/lib/widgets.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,59 @@ class GoogleUserCircleAvatar extends StatelessWidget {
FadeInImage.memoryNetwork(
// This creates a transparent placeholder image, so that
// [placeholder] shows through.
placeholder: Uint8List((size.round() * size.round())),
placeholder: _transparentImage,
image: sizedPhotoUrl,
)
]),
));
}
}

/// This is an transparent 1x1 gif image
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the "." in 03c48da

final Uint8List _transparentImage = Uint8List.fromList(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. Create a new package in plugin/packages/<new package>
  2. Add the gif image as an asset and this variable transparentImage (not private)
  3. Import this package in google_sign_in to reuse the variable transparentImage
  4. 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that sounds good to me, thanks!

[
71,
73,
70,
56,
57,
97,
1,
0,
1,
0,
128,
0,
0,
255,
255,
255,
0,
0,
0,
33,
249,
4,
1,
0,
0,
0,
0,
44,
0,
0,
0,
0,
1,
0,
1,
0,
0,
2,
2,
68,
1,
0,
59,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it in a54232d

],
);
2 changes: 1 addition & 1 deletion packages/google_sign_in/google_sign_in/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ description: Flutter plugin for Google Sign-In, a secure authentication system
for signing in with a Google account on Android and iOS.
repository: https://github.com/flutter/plugins/tree/master/packages/google_sign_in/google_sign_in
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+google_sign_in%22
version: 5.1.1
version: 5.1.2

environment:
sdk: ">=2.14.0 <3.0.0"
Expand Down
Loading