[flutter_svg] Update README with an example to scale images#10547
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
bot
added
p: flutter_svg
triage-engine
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the documentation to include an example of how to scale an SVG image. The changes are applied to README.md and the corresponding example file readme_excerpts.dart. My review focuses on the clarity of the new example code. While the added example is functionally correct, its presentation as a sequence of operations where variables are reassigned can be confusing. I've suggested an alternative structure that presents the different image creation methods as clear alternatives, improving readability for developers who will use this as a reference.
third_party/packages/flutter_svg/example/lib/readme_excerpts.dart
Outdated
Show resolved
Hide resolved
domesticmouse
left a comment
domesticmouse
left a comment
There was a problem hiding this comment.
The Gemini Code Assist review concern is valid, please address
Thank you, I've fixed my examples. My understanding is that these changes are exempt from requiring tests since they affect the docs and examples? Do I need to make changes to the pubspec.yaml or changelog? I refrained from making changes there to avoid running into unnecessary conflicts for now. |
Yes. Please see the FAQ in the link in the relevant checklist items. |
| ); | ||
|
|
||
| pictureInfo.picture.dispose(); | ||
| ``` |
There was a problem hiding this comment.
This is adding a lot of code length to the README, and a lot of it is duplicated from above, or is about PictureRecorder rather than flutter_svg. Do we actually need to document anything other than the need to call scale to make it clear how to do this?
There was a problem hiding this comment.
To keep the example code footprint smaller, I could keep the current example and just add two lines like so:
// You can scale the canvas to achieve lossless vector image stretching before drawing it
canvas.scale(1.2, 1.2);
// You can draw the picture to a canvas:
canvas.drawPicture(pictureInfo.picture);Would that be better maybe?
There was a problem hiding this comment.
Sorry, I missed the notification for this question. Yes, just adding one statement to the existing example, with a comment making it very clear that it's optional, would be much better.
There was a problem hiding this comment.
Done, please resolve this if it looks good to you, thanks
|
Based on this, I believe these changes qualify for tests exemption, so I'm going to tick that box, but let me know if I got this wrong, thanks. |
|
This PR is now ready for review again. I'm assuming I don't need to take action on:
Please let me know otherwise. |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM with one small change.
| null, | ||
| ); | ||
|
|
||
| // You can scale the canvas to achieve lossless scaling |
There was a problem hiding this comment.
This needs to have have a period, just like the other comments do.
There was a problem hiding this comment.
The other comments around this one end with a colon, not a period, should I use a colon in that block like so?
// You can scale the canvas to achieve lossless scaling:
canvas.scale(1.2, 1.2);
// You can draw the picture to a canvas:
canvas.drawPicture(pictureInfo.picture);
// Or convert the picture to an image:
final ui.Image image = await pictureInfo.picture.toImage(width, height);
Or should I convert the other comments too so they all end with a period?
There was a problem hiding this comment.
I missed that those were using colons, since it's not typical. Matching them is fine.
There was a problem hiding this comment.
Thanks for clarifying, done.
|
From triage: @domesticmouse This is just waiting for your secondary review. |
stuartmorgan-g
added
the
autosubmit
flutter/packages@fe3de64...c717018 2026-03-09 [email protected] [pigeon] Support javax.annotation.Generated annotation in Kotlin generator (flutter/packages#10961) 2026-03-09 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump gradle-wrapper from 8.13 to 9.4.0 in /packages/path_provider/path_provider/example/android/app (flutter/packages#11206) 2026-03-09 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.android.tools.build:gradle from 8.9.1 to 9.1.0 in /packages/shared_preferences/shared_preferences_android/example/android/app (flutter/packages#11208) 2026-03-09 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump gradle-wrapper from 8.13 to 9.4.0 in /packages/path_provider/path_provider_android/example/android/app (flutter/packages#11207) 2026-03-06 [email protected] [various] Plugin Gradle pre-Kotlin standardization (flutter/packages#11173) 2026-03-06 [email protected] Updated AGP and KGP to align with flutter/flutter templates (flutter/packages#10423) 2026-03-06 [email protected] Roll Flutter from d3dd774 to d182143 (33 revisions) (flutter/packages#11191) 2026-03-06 [email protected] [google_maps_flutter] Add Advanced Markers support (flutter/packages#7882) 2026-03-06 [email protected] [in_app_purchase]Fixes StoreKit 2 purchase flow to send cancelled/pending/unverified results to purchaseStream. (flutter/packages#10736) 2026-03-06 [email protected] [flutter_svg] Update README with an example to scale images (flutter/packages#10547) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…er#183396) flutter/packages@fe3de64...c717018 2026-03-09 [email protected] [pigeon] Support javax.annotation.Generated annotation in Kotlin generator (flutter/packages#10961) 2026-03-09 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump gradle-wrapper from 8.13 to 9.4.0 in /packages/path_provider/path_provider/example/android/app (flutter/packages#11206) 2026-03-09 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.android.tools.build:gradle from 8.9.1 to 9.1.0 in /packages/shared_preferences/shared_preferences_android/example/android/app (flutter/packages#11208) 2026-03-09 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump gradle-wrapper from 8.13 to 9.4.0 in /packages/path_provider/path_provider_android/example/android/app (flutter/packages#11207) 2026-03-06 [email protected] [various] Plugin Gradle pre-Kotlin standardization (flutter/packages#11173) 2026-03-06 [email protected] Updated AGP and KGP to align with flutter/flutter templates (flutter/packages#10423) 2026-03-06 [email protected] Roll Flutter from d3dd774 to d182143 (33 revisions) (flutter/packages#11191) 2026-03-06 [email protected] [google_maps_flutter] Add Advanced Markers support (flutter/packages#7882) 2026-03-06 [email protected] [in_app_purchase]Fixes StoreKit 2 purchase flow to send cancelled/pending/unverified results to purchaseStream. (flutter/packages#10736) 2026-03-06 [email protected] [flutter_svg] Update README with an example to scale images (flutter/packages#10547) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
3 participants
Following up on my original PR dnfield/flutter_svg#991 after the suggestion from @darshankawar here.
This is an update to the flutter_svg documentation to include an example of how to scale an SVG image without losing quality, as this is something currently missing from the documentation and causing some confusion.
I believe it resolves the following issues (there might be more since then, the original PR is from 2023, I haven't searched for more):
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3