jesusrp98 / adwaita_icons

GNU General Public License v3.0
6 stars 6 forks source link

Initial refactor using icon_font_generator #3

Closed pablojimpas closed 2 years ago

pablojimpas commented 2 years ago
jesusrp98 commented 2 years ago

Thanks for taking the time to upgrade this package to use font icons instead of SVGs.

The problem is that the adwaita icons are not really suitable for font rendering:

  1. Some icons feature transparency parts, that cannot be rendered because Flutter lacks the ability to render semi-transparent fonts.
  2. As you said, there are some icons that look a bit broken. This is because the GNOME team didn't build the icon pack to be font compatible.

Because all those issue, I decided to peruse the SVG rendering route: you can render semi-transparent icons, and all icons look great.

In my opinion, I think we should stick to flutter_svg to render the icons, instead of using font rendering. We'd need to optimize the compression process and tweak a couple other stuff, but I think it's the right path.

Please let me know what you think about what I've just said, and thank you very much for taking the time to improve this package :)

pablojimpas commented 2 years ago

@jesusrp98 thanks for clarifying what's causing the issue! I had no idea that GNOME uses semi-transparent icons and that Flutter can't render them.

In any case, I think that we should strive to implement a solution similar to this attempt instead of relaying on rendering bloated SVGs with an additional non-trivial dependency.

Flutter's built-in Icon and IconData widgets have great properties that we will benefit from if we can create a proper font.

This package's purpose shouldn't be trying to mimic the built-in Icon with a similar implementation, it will only add complexity, bugs and further problems maintaining this package in the future. Instead, I think that this package's purpose should be being a drop-in replacement (providing the same functionality) to the Icons class from the Material package.

With that being said...Ideally the issue that we are facing with rendering semi-transparent fonts would be fix with upstream in mind, either modifying the problematic SVGs in GNOME repo (not very likely that they want) or getting Flutter to render semi-transparent fonts (almost impossible to get their attention with the amount of work they are facing right now and definitely non suitable short-term).

So assuming that those approaches aren't viable I think we have two options:

  1. Identify which SVGs are having issues and manually fix them before font generation.
  2. Implement a feature in the font generator used (or find another one) that will allow us to generate non-transparent fonts regardless of the SVGs properties.

Let me know what you think of this or if I'm missing something but I really think this approach will be better long-term.

jesusrp98 commented 2 years ago

I don't think that the issue of rendering font with semi-transparency is going to be fixed any time soon. In fact, I asked about it years ago (time flies) and a member of the Flutter team answered me.

I see two possible paths, and correct me if I'm wrong:

  1. We continue rendering the icons using a SVG render. This way we can render the icons with transparency and no issues. It has the inconvenience of not being compatible with the Icon widget. This way, the package weights quite a bit.
  2. We switch to a font-render. We cannot render any transparency, and some icons are broken (we could fix them). We have full compatibility with the Icon widget and the package weights less.

For me, not being able to render transparency is a dealbreaker. This is an obstacle that I don't think the Flutter team is going to fix in the near future, unfortunately.

We can improve this package in many ways, like deprecating the AdwaitaIcon and start using the ImageIcon widget, which is basically the same thing.

The flutter_svg_provider can also improve this pacakge, in conjunction with ImageIcon.

To sum up, at this point and from my point of view, font rendering is not the way going forward. It would degrade the quality of the package substantially. We can improve the quality of the package in other areas.

Thank you very much for taking the time to trying to improve this package for all the community. Let me know what you think about all I've said :)

pablojimpas commented 2 years ago

2. We switch to a font-render. We cannot render any transparency, and some icons are broken (we could fix them). We have full compatibility with the Icon widget and the package weights less.

For now I'll try to pursuit this route with this PR.

Actually, transparency isn't the blocking issue here. Sure, icons that have two tones won't look identical but they are rendered fine. The weird issue that I was experiencing was caused only by 12 icons (the ones named preferences-desktop-*-symbolic.svg), I've compared their actual SVG xml code and it looks way different than all the rest. I've just temporary deleted those 12 icons and now you can check out the example app just fine (without semi-transparency though, that's a different story...)

jesusrp98 commented 2 years ago

Ok, I'll take a look at the rest of the icons to see how they look. I'll let you know :)

Thanks! :)

pablojimpas commented 2 years ago

Ok, I'll take a look at the rest of the icons to see how they look.

A bunch of icons will need editing to look similar. It's a shame that GNOME uses so much two-tone icons for "symbolic" icons. In my opinion symbolic icons should be monotone and convey all the meaning just with their form. For more expressive icons it's okey to use SVGs with different colors and transparency but symbolic ones should stick to one solid color to avoid this kinds of issues using them as fonts...

Anyway, minor tweaks would be needed for example to volume icons:

Original rendered with flutter_svg:

image

Generated font without tweaking:

image

Generated font with some quick dirty tweaks (not ideal, specially the muted icon 😅, just to illustrate my point):

image

On the bright side there are other icons where transparency doesn't really mean anything and may not need any changes:

Original rendered with flutter_svg:

image

Generated font without tweaking:

image

Honestly...for me in this case it just looks better without transparency, it won't be 100% pixel perfect with GNOME apps but I think it's reasonable.

jesusrp98 commented 2 years ago

After thinking it hard for the last couple days, I don't think using a font to render adwaita icons is the right path going forward. There are a noticeable number of icons that would be broken. Sure we could fix them in a dirty way like you've shown, but that solution is far from good.

The GNOME team has been updating the icon library (as shown here) and that would mean that we'd need to update a bunch of icons after the update: that wouldn't be scalable nor maintainable going forward.

Fixing a technical inconvenience, like not being able to use the Icon widget, would create an usability and quality problem that would decrees the user experience. I do think that this package could be greatly improved as I said in previous comments, but this would only decrease the quality of the final product, in my humble opinion.

If you're not content with my final decision, I encourage you to create a new adwaita icon package that render the icons using a font.

I really, really thank you for the effort you've done trying to improve the experience of adwaita apps in Linux using Flutter.

pablojimpas commented 2 years ago

I totally understand your decision of sticking to SVG rendering, it's the reasonable one to make right now given the poor state of upstream to support font rendering. I just wanted to explore this approach to see if it was doable short term, now we know that it isn't viable as long as GNOME uses more than one tone for their symbolic icons and/or Flutter doesn't support semi-transparency in font rendering (likely never).

Fixing a technical inconvenience, like not being able to use the Icon widget, would create an usability and quality problem that would decrees the user experience. I do think that this package could be greatly improved as I said in previous comments, but this would only decrease the quality of the final product, in my humble opinion.

Regarding this problem, mimicking the Icon widget may need some improvements to make it behave as close as possible to the original one, that will make it more developer-friendly and hopefully a drop-in replacement, but that's for another day, today I'm closing this PR with an eye put on upstream for future changes, especially this issue https://github.com/flutter/flutter/issues/1831 which might be useful some day.

See also: https://flutter.dev/go/vector-graphics

Looking forward to keep improving multi-platform development targeting Linux with you. Thanks for your insight in this topic!