Skip to content

feat(xdg_icons): replace flutter_svg dependency with jovial_svg#475

Merged
d-loose merged 1 commit intocanonical:mainfrom
PureTryOut:fix/xdg_icons-svg-with-style
Jun 11, 2025
Merged

feat(xdg_icons): replace flutter_svg dependency with jovial_svg#475
d-loose merged 1 commit intocanonical:mainfrom
PureTryOut:fix/xdg_icons-svg-with-style

Conversation

@PureTryOut
Copy link
Contributor

@PureTryOut PureTryOut commented Apr 9, 2025

This allows using SVG icons that have for example the <style> attribute set. An example of an icon theme using this is KDE's Breeze. flutter_svg doesn't support this and without it Breeze's dark icons would render as black even though they're supposed to be white.

See dnfield/flutter_svg#105 for context.

Fixes #472.

CC @jpnurmi

@PureTryOut PureTryOut force-pushed the fix/xdg_icons-svg-with-style branch from d369168 to e897e24 Compare April 10, 2025 16:57
@PureTryOut PureTryOut force-pushed the fix/xdg_icons-svg-with-style branch 2 times, most recently from 2d3dad0 to 48311b0 Compare April 10, 2025 19:22
Copy link
Contributor

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jpnurmi
Copy link
Contributor

jpnurmi commented Apr 11, 2025

@PureTryOut I'm not sure why the CI doesn't clearly state it, but the contributor agreement needs to be signed: https://ubuntu.com/legal/contributors

@d-loose
Copy link
Member

d-loose commented Apr 11, 2025

Looks like the golden test for 'svg file' is failing.
Running it locally, I can see that the expected blue icon doesn't show up correctly:

Expected Got
svg_file_masterImage svg_file_testImage

Could you have a look @PureTryOut ?

Copy link
Contributor

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

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

Sorry for the bikeshedding. 😅 Previously, we would pass the preferred size to SvgPicture constructor, but with ScalableImageWidget, we're not passing any size. Does it end up using the SVG viewbox size, which may not match what the user requested? The mismatch should be noticeable when passing a large size to XdgIcon. If there's no way to pass the preferred size, perhaps we need to wrap it with a SizedBox or something?

@PureTryOut PureTryOut force-pushed the fix/xdg_icons-svg-with-style branch from 48311b0 to 72d3f07 Compare April 12, 2025 08:09
@PureTryOut
Copy link
Contributor Author

Sorry for the bikeshedding.

No problem, that's what PR's are for 😉

Does it end up using the SVG viewbox size

Yes. I've set the size in the example to 480 and it only got as big as the viewbox size. Seems just fine.

@PureTryOut I'm not sure why the CI doesn't clearly state it, but the contributor agreement needs to be signed: https://ubuntu.com/legal/contributors

I saw that in CI but didn't figure out where I could sign it. Thanks for the link, I'll read it through.

@PureTryOut PureTryOut force-pushed the fix/xdg_icons-svg-with-style branch 2 times, most recently from 09a3c34 to 2e920ad Compare April 12, 2025 08:19
@PureTryOut
Copy link
Contributor Author

Not sure why the CLA check is still failing as I've signed it now. I can't sign it again either as it says the email address (the same as on my commits) has already signed it.

@jpnurmi
Copy link
Contributor

jpnurmi commented Apr 14, 2025

Does it end up using the SVG viewbox size

Yes. I've set the size in the example to 480 and it only got as big as the viewbox size. Seems just fine.

The problem is that those arbitrary sizes in the SVG files might significantly differ from the actual requested sizes. For example, requesting a 16px drive-harddisk icon from the Adwaita theme:

Center(
  child: XdgIcon(name: 'drive-harddisk', size: 16, theme: 'Adwaita'),
)

Would show a 128px icon because that specific icon theme happens to ship /usr/share/icons/Adwaita/scalable/devices/drive-harddisk.svg as a scalable icon.

@d-loose
Copy link
Member

d-loose commented Apr 14, 2025

Not sure why the CLA check is still failing as I've signed it now. I can't sign it again either as it says the email address (the same as on my commits) has already signed it.

Could you try rebasing your branch and see if that fixes it? I've updated the CLA workflow in #478.

@PureTryOut PureTryOut force-pushed the fix/xdg_icons-svg-with-style branch from 2e920ad to 033c137 Compare April 30, 2025 12:30
@PureTryOut
Copy link
Contributor Author

@jpnurmi I got your point. I added a SizedBox around them with the given width, should be good now.

@d-loose seems that helped, it went through now 👍

@PureTryOut PureTryOut force-pushed the fix/xdg_icons-svg-with-style branch from 033c137 to a3e92d4 Compare April 30, 2025 13:30
@jpnurmi
Copy link
Contributor

jpnurmi commented Apr 30, 2025

In the latest commit, packages/xdg_icons/test/failures accidentally slipped in. Otherwise LGTM 👍

@PureTryOut PureTryOut force-pushed the fix/xdg_icons-svg-with-style branch from a3e92d4 to 82637f0 Compare April 30, 2025 13:50
@PureTryOut
Copy link
Contributor Author

Woops! That should probably be in a gitignore 😅

@PureTryOut
Copy link
Contributor Author

Is there anything left to do here?

@d-loose
Copy link
Member

d-loose commented Jun 10, 2025

Is there anything left to do here?

Sorry for the late reply - I've been quite busy these past weeks.
The changes look good to me, thanks!

One minor thing, would you mind signing your commits?

This allows using SVG icons that have for example the <style> attribute
set. An example of an icon theme using this is KDE's Breeze.
flutter_svg doesn't support this and without it Breeze's dark icons would
render as black even though they're supposed to be white.

See dnfield/flutter_svg#105 for context.
@PureTryOut PureTryOut force-pushed the fix/xdg_icons-svg-with-style branch from 82637f0 to 505a182 Compare June 10, 2025 18:03
@PureTryOut
Copy link
Contributor Author

Done!

Copy link
Member

@d-loose d-loose left a comment

Choose a reason for hiding this comment

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

Thank you!

@d-loose d-loose merged commit 82dc5cd into canonical:main Jun 11, 2025
10 checks passed
d-loose added a commit that referenced this pull request Jun 11, 2025
Release `xdg_icons` `0.1.1` including the changes from #475.
@PureTryOut PureTryOut deleted the fix/xdg_icons-svg-with-style branch June 11, 2025 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

xdg_icons returns black icons rather than properly colored

3 participants