-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Needs API review] Feature/icons #5867
Conversation
Should this be added to #5893 to help prioritize? This one (especially FontIcon) is pretty important in my opinion. |
@robloo personally I would like to have these icons build in the Avalonia core. With the very one reason - to keep relevant controls' API stricter. On other hand, we already have FontIcon control that is questionable for me, and to port more UWP controls we most likely will need these Icons anyway. So, to justify these breaking changes we need pretty good reasons, and Avalonia core team does not really see any other advantages, but compatibility with UWP. |
Well, that's what i was afraid of. The Icon story is pretty poor in Avalonia and it should really be generalized more as in UWP. There is no 1.0 release yet so in my opinion better late than never. All these controls should be taking an IconSource and not bitmaps. Avalonia should at least have an Icon interface that is more specific than a bitmap. To export to a native menu there should be a mechanism to render the entire font icon to a bitmap (like in UWP where you can render a control to a bitmap) this is needed for drag/drop graphics anyway. |
@robloo I think icons can be easily implemented in Avalonia using SVG paths, and you could actually animate them (with some work). Practically all modern icons libraries expose their icons in SVG format, since Avalonia can run on SkiaSkarp, it is easy to render SVG icons. |
@beto-rodriguez Thats an interesting idea. However, I would say there should actually be an SvgIcon to go along with the rest. And the reason fonts were used instead of other vector graphics at the time was simply performance. You can't beat the performance of rendering a glyph from a font as its already so heavily optimized all the way through systems. Now supporting Svg animations is nice too but the XAML UI frameworks seem to be moving to Lottie animations instead. These would go towards an animated icon. Everyone standardizing on lottie would help sharing design resources. I'll think about all this more. I'm not saying UWP was perfect: IconSource vs IconElement, etc. Wasnt the best. However, having all the icon types readily available saved a lot of time rather than creating your own bitmaps. For Avalonia, I firmly think it needs to be aware of 'icons' separately from 'bitmaps' in some way. Icons are a subset of bitmaps but for very specific usages: like menus and interface elements. This would make the API a lot cleaner and easier to use. |
@beto-rodriguez yep, unlikely UWP implementation, with this PR it is possible to create custom third-party IconElement with overriding IconElementTemplate. It might include various font implementations (Fluent icons, Segoe icons, FontAwesome icons etc.) and SVG as well. Let's say, https://github.com/wieslawsoltes/Svg.Skia/ repo can provide SvgIcon and SvgIconSource in Avalonia nuget package, and implementation will be simple. |
Writing FontIcons to a Bitmap is a no brainer. We can switch to using a GlyphRun directly instead of a TextBlock. By using a GlyphRun it will be possible to export everything to a path as well. |
I'm not sure where this PR is at -- hopefully it can still get past the core team and accepted into mainline.
@maxkatz6 As you are aware, I have most of the tooling necessary now to generate various symbol fonts including those you mentioned. If ever you want to add them in some fashion -- even as another 3rd party Nuget -- just let me know. We should be able to automatically generate most of the code defining symbol icons and the font .ttf files as well. https://github.com/robloo/SymbolIconManager |
I had another thought about this. Can't these two icon API's switch to using IBitmap instead of Bitmap? Then supported Icon types can also implement IBitmap and do their own rasterization. It wouldn't be a breaking change to anyone using Bitmaps already as well since they are already IBitmap. That might solve most of the concerns here? |
I think we should at least remove IconSourceElement support from this PR and just propose to use <Window.Resources>
<Template x:Key="IconTemplate">
<PathIcon Data="..." />
</Template>
</Window.Resources>
<StackPanel>
<Button Content="{StaticResource IconTemplate}" />
<Button Content="{StaticResource IconTemplate}" />
</StackPanel> Just double checked, and it works with Styles as a glue: <Window.Resources>
<Template x:Key="ImageTemplate"> <!-- Or use Setter.Value directly without StaticResource -->
<Image Source="..." />
</Template>
</Window.Resources>
<Window.Styles>
<Style Selector="Button">
<Setter Property="Content" Value="{StaticResource ImageTemplate}"/>
</Style>
</Window.Styles>
<StackPanel>
<Button />
<Button />
</StackPanel> As alternative, UWP (and this PR's) approach would be like this: <Window.Resources>
<PathIconSource x:Key="IconSource" Data="..." />
</Window.Resources>
<StackPanel>
<Button>
<IconSourceElement IconSource="{StaticResource IconSource}" />
</Button>
<Button>
<IconSourceElement IconSource="{StaticResource IconSource}" />
</Button>
</StackPanel> |
Not a huge difference, since WindowIcon API uses Bitmap only because it can get memory handle from there, it doesn't need to render anything in that way. We still will have a problem with question "when we should render icon?" Although, to be honest, I forgot what's a problem with that, because we can render controls outside of renderer thread with immediate renderer (just like now we can 'screenshot' control), can't we @kekekeks? But there definitely will be performance problems with Lottie based icons, but we should just disallow them from using as a WindowIcon. |
"I think we should at least remove IconSourceElement support from this PR and just propose to use where developer need to reuse same icon in multiple places." Thats pretty cool you can do that in Avalonia. I always get confused the first time I have to use an IconSource after a while. Cleaning this up would be a bonus and I think everyone is more familiar with styling. I really am just looking forward to FontIcon and PathIcon. |
The memory handle can be accessed from either IBitmap or Bitmap. Both have the necessary properties for this.
A FontIcon, for example, should be responsible for it's own rendering to a bitmap and expose the IBitmap interface. When the FontIcon itself detects a Glyph, FontFamily, FontSize, etc. property change it should re-render itself and update the IBitmap properties. That takes care of 'When we should render" it. I don't see that the IBitmap interface supports any notification of updates though so I'm not sure how to notify a native menu it needs to re-draw the bitmaps. Perhaps, some bitmap invalidated events are needed to the interface itself. |
What if user has animation that should update any of these values with 60fps rendering? I know it's not common case, but it can be possible. I believe there is a difference in performance between "render single control" on UI thread and usual deferred rendering on renderer thread. |
That's also really easy I think, don't re-render on each property change. Set a flag, batch property changes together and then render to a bitmap internally only every N milliseconds (likely a very high number to filter out animations entirely, 500ms or so). Put in the docs that animation of icons and their properties used within NativeMenus isn't supported. If a dev tries this it will be a very poor animation and shouldn't affect performance. They'll read the docs or ask here why it isnt working and we can answer. Animations are not bitmaps. NativeMenu requires bitmaps. I'm also assuming image animation would be done in a future AnimatedIcon which obviously wouldn't implement IBitmap and therefore couldn't be passed to the NativeMenu APIs. |
A couple of additional comments:
|
An alternative, less-ideal, idea:
Edit: Point 3 could actually be generalized into a control that rendered its child content into a bitmap. |
Closing it for now. Will come back after tasks with higher priority that I am planning to work during February |
What does the pull request do?
What is the current behavior?
What is the updated/expected behavior with this PR?
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues
See: #4087