miloush
commented
10 months ago So are these not for discussion or what is the status of the theming API proposals?
Open pchaurasia14 opened 10 months ago
miloush
commented
10 months ago So are these not for discussion or what is the status of the theming API proposals?
pchaurasia14
commented
10 months ago @miloush - These were generated via automated build differences. I'm making the edits to make it more easier to review. Once done, we can discuss them. Same with the other API proposals. Should be ready in a day or two.
lindexi
commented
10 months ago @pchaurasia14 Will the field of FontIcon.TextBlock break the dotnet design standard?
❌ DO NOT provide instance fields that are public or protected.
See https://github.com/dotnet/docs/blob/main/docs/standard/design-guidelines/field.md
pchaurasia14
commented
10 months ago @lindexi - Yup. I've corrected it.
miloush
commented
10 months ago SymbolRegular is missing?
miloush
commented
10 months ago Is this modeled after other frameworks? UWP/WinUI/MAUI?
Icon="{FontIcon Glyph="'🌈'" FontFamily="Cascadia" FontSize=10 }" is probably invalid syntax with the quotes
Why IIconControl rather than [IconProperty("Icon")]?
Why protected IconElement.OnForegroundPropertyChanged? Derived classes can override the DP metadata.
Why the explicit control properties like FontIcon.TextBlock, ImageIcon.Image? Isn't the idea of WPF that the controls are UI-less and the UI is provided with templates?
It is a bit weird to call Glyph a property that actually means character, not a font glyph (shouldn't we support glyphs too btw.?). But if other API already uses it, I guess it's ok.
Shouldn't boolean properties like SymbolIcon.Filled be IsFilled?
Why is IconSourceElement needed? Does not seem to be used by any of the classes.
Don't the icon sources need not to be Animatable? Why are they not image sources (or drawings where appropriate, there is already a GlyphRunDrawing)?
Finally, why does IconElement even need to exist? Its only purpose seems to have a Foreground property. What does the ImageIcon need a foreground for? Moreover, it would confusingly not apply to drawings image sources. FontIcon seems to be better class to have a Foreground property.
pchaurasia14
commented
10 months ago @miloush - Thanks for the questions.
SymbolRegular is missing?
I've added the link to the file.
Is this modeled after other frameworks? UWP/WinUI/MAUI?
I don't think it is. We will take a look at them and see how we can bring uniformity to APIs.
For the other changes, I think we'll need to first decide on the high level API changes and then trim down the supporting abstractions.
dipeshmsft
commented
9 months ago @miloush , I have just updated the proposal. Here are the answer to few of your questions. ( Deferring SymbolIcon related APIs and questions for second iteration )
Why is IconSourceElement needed? Does not seem to be used by any of the classes?
IconSourceElement is supposed to work in combination with the IconSource classes. IconSource objects are DependencyObject and hence can be shared and used for binding. Here is how they are used : https://github.com/microsoft/terminal/blob/e3ff44bb82699178c00dd46db70030ea4a777353/src/cascadia/TerminalSettingsEditor/Launch.xaml#L64-L67
It is a bit weird to call Glyph a property that actually means character, not a font glyph (shouldn't we support glyphs too btw.?). But if other API already uses it, I guess it's ok.
WinUI uses the same property name, hence we kept it this way in the proposal.
Why IIconControl rather than [IconProperty("Icon")]? Why protected IconElement.OnForegroundPropertyChanged? Derived classes can override the DP metadata.
Removed these. They were not needed as you mentioned.
dipeshmsft
commented
9 months ago @dotnet/dotnet-wpf-triage @MikeHillberg The proposal has been updated with the suggestions. Can you take a look before we take this for API review meeting ?
MikeHillberg
commented
9 months ago @dotnet/dotnet-wpf-triage @MikeHillberg The proposal has been updated with the suggestions. Can you take a look before we take this for API review meeting ?
General process note, would you be OK with creating API specs for these APIs in general? The benefit of the API spec is that it can be merged and then easily searched for later, and it's much easier for commenting. This is the spec template we use for Windows specs, which is designed to align with public documentation. In the markdown comments are instructions, including samples such as RadialGradientBrush.
batzen
commented
9 months ago I might be a bit late, but what's the purpose of all these new classes? We already have Image and ImageSource. What's the difference between those and ImageIcon/IconSource? Icons are just images, aren't they?
Instead of FontIcon and FontIconSource I could just use a TextBlock, can't i?
dipeshmsft
commented
9 months ago Sorry @batzen , I missed this earlier.
Icons are just images, aren't they?
In case of FontIcon, it is a glyph. And apart from ImageIcon, there are other scenarios, where we can have PathIcon, AnimatedIcon, etc.
Instead of FontIcon and FontIconSource I could just use a TextBlock, can't i?
Yes, we can use a TextBlock, however FontIcon and other classes will allow us to make templated controls with icons. IconSource classes allow us to share icons and make it possible to have non UI objects that can be bound with the application UI.
batzen
commented
9 months ago @dipeshmsft I still don't understand what the advantage of all those new classes would be and how they differ from an regular image.
In case of FontIcon, it is a glyph. And apart from ImageIcon, there are other scenarios, where we can have PathIcon, AnimatedIcon, etc.
Glyph = char = string? So i don't see the nee for a dedicated class here when i can just use a TextBlock. Designing icons with paths is also already possible with DrawingImage. So i still don't understand what the benefit of having new classes, which sounds like just being an alternative to Image/ImageSource, would be. Not sure about AnimatedIcon though. Are you going to add support for gif images for that, or what would an AnimatedIcon look like?
Yes, we can use a TextBlock, however FontIcon and other classes will allow us to make templated controls with icons.
Templating controls with icons, or placeholders for such, is already possible now. Forcing those templates to something like FontIcon or anything else, instead of object, would be a major step back in regards to flexibility of WPF. We already have templated controls with icons in WPF. For example MenuItem. And the Icon property there is of type object by purpose.
What i would love to see instead is native support for SVG images. But not even WinUI does have proper SVG support. But having proper native SVG support in WPF would be greatly appreciated.
terrajobst
commented
9 months ago For easier review I've removed all overrides (as we their shape and naming isn't up for debate) and collapsed all types into a single block using C# syntax instead of diff.
Two questions: are all these types going into namespace System.Windows?
namespace System.Windows;
[TypeConverter(typeof(IconElementConverter))]
public abstract class IconElement : FrameworkElement
{
protected abstract UIElement InitializeChildren();
protected IconElement();
public static readonly DependencyProperty ForegroundProperty;
[Bindable(true)]
[Category("Appearance")]
public Brush Foreground { get; set; }
}
public class FontIcon : IconElement
{
public FontIcon();
public static readonly DependencyProperty FontFamilyProperty;
[Bindable(true)]
[Category("Appearance")]
[Localizability(LocalizationCategory.Font)]
public FontFamily FontFamily { get; set; }
public static readonly DependencyProperty FontSizeProperty;
[Bindable(true)]
[Category("Appearance")]
[TypeConverter(typeof(FontSizeConverter))]
[Localizability(LocalizationCategory.None)]
public double FontSize { get; set; }
public static readonly DependencyProperty FontStyleProperty;
[Bindable(true)]
[Category("Appearance")]
public FontStyle FontStyle { get; set; }
public static readonly DependencyProperty FontWeightProperty;
[Bindable(true)]
[Category("Appearance")]
public FontWeight FontWeight { get; set; }
public static readonly DependencyProperty GlyphProperty;
public string Glyph { get; set; }
}
[ContentProperty("IconSource")]
public class IconSourceElement : IconElement
{
public IconSourceElement();
public static readonly DependencyProperty IconSourceProperty;
public IconSource IconSource { get; set; }
}
[ContentProperty("IconSource")]
public class ImageIcon : IconElement
{
public ImageIcon();
public static readonly DependencyProperty SourceProperty;
public ImageSource Source { get; set; }
}
public abstract class IconSource : DependencyObject
{
protected IconSource();
public static readonly DependencyProperty ForegroundProperty;
public Brush Foreground { get; set; }
public abstract IconElement CreateIconElement();
}
public class FontIconSource : IconSource
{
public FontIconSource();
public static readonly DependencyProperty FontFamilyProperty;
public FontFamily FontFamily { get; set; }
public static readonly DependencyProperty FontSizeProperty;
public double FontSize { get; set; }
public static readonly DependencyProperty FontStyleProperty;
public FontStyle FontStyle { get; set; }
public static readonly DependencyProperty FontWeightProperty;
public FontWeight FontWeight { get; set; }
public static readonly DependencyProperty GlyphProperty;
public string Glyph { get; set; }
}
public partial class IconSourceElementConverter : IValueConverter
{
public IconSourceElementConverter();
public object Convert(object value, Type targetType, object parameter, CultureInfo culture);
public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture);
}
[ContentProperty(nameof(Source))]
[MarkupExtensionReturnType(typeof(ImageIcon))]
public class ImageIconExtension : MarkupExtension
{
public ImageIconExtension(ImageSource? source);
[ConstructorArgument("source")]
public ImageSource? Source { get; set; }
public double Width { get; set; } = 16D;
public double Height { get; set; } = 16D;
}
[ContentProperty(nameof(Glyph))]
[MarkupExtensionReturnType(typeof(FontIcon))]
public class FontIconExtension : MarkupExtension
{
public FontIconExtension(string glyph);
public FontIconExtension(string glyph, FontFamily fontFamily);
[ConstructorArgument("glyph")]
public string Glyph { get; set; }
[ConstructorArgument("fontFamily")]
public FontFamily FontFamily { get; set; }
public double FontSize { get; set; }
}
ThomasGoulet73
commented
9 months ago I had some comments about the proposal.
1:
Small nit: The name InitializeChildren looks wrong to me, I think CreateChild would be better. "Child" instead of "Children" because it returns only one item and "Create" instead of "Initialize" because I think "Initialize" is more used when the method modifies the instance while "Create" is more used when it creates a new instance (This last point might be personal preference only).
2:
Two questions: are all these types going into namespace System.Windows?
I think all these classes would fit better in System.Windows.Controls.
3:
I share the same view (I think 😄) as @batzen about the extensibility of these new classes. I think the idea of having a base class used to say "this is an icon" is positive and having implementations for common uses is also good but we shouldn't lose the ability of setting an object as an icon. To allow that, we could either make IconElement not abstract and allow to set random objects as its content or add a new implementation of IconElement where we can set random objects as its content.
bartonjs
commented
9 months ago internal class is probably more consistent with the rest of WPF, but if it's needed as public then it's fine.namespace System.Windows.Controls;
[TypeConverter(typeof(IconElementConverter))]
public abstract class IconElement : FrameworkElement
{
protected abstract UIElement InitializeChildren();
protected IconElement();
public static readonly DependencyProperty ForegroundProperty;
[Bindable(true)]
[Category("Appearance")]
public Brush Foreground { get; set; }
}
public class FontIcon : IconElement
{
public FontIcon();
public static readonly DependencyProperty FontFamilyProperty;
[Bindable(true)]
[Category("Appearance")]
[Localizability(LocalizationCategory.Font)]
public FontFamily FontFamily { get; set; }
public static readonly DependencyProperty FontSizeProperty;
[Bindable(true)]
[Category("Appearance")]
[TypeConverter(typeof(FontSizeConverter))]
[Localizability(LocalizationCategory.None)]
public double FontSize { get; set; }
public static readonly DependencyProperty FontStyleProperty;
[Bindable(true)]
[Category("Appearance")]
public FontStyle FontStyle { get; set; }
public static readonly DependencyProperty FontWeightProperty;
[Bindable(true)]
[Category("Appearance")]
public FontWeight FontWeight { get; set; }
public static readonly DependencyProperty GlyphProperty;
public string Glyph { get; set; }
}
[ContentProperty("IconSource")]
public class IconSourceElement : IconElement
{
public IconSourceElement();
public static readonly DependencyProperty IconSourceProperty;
public IconSource IconSource { get; set; }
}
[ContentProperty("IconSource")]
public class ImageIcon : IconElement
{
public ImageIcon();
public static readonly DependencyProperty SourceProperty;
public ImageSource Source { get; set; }
}
public abstract class IconSource : DependencyObject
{
protected IconSource();
public static readonly DependencyProperty ForegroundProperty;
public Brush Foreground { get; set; }
public abstract IconElement CreateIconElement();
}
public class FontIconSource : IconSource
{
public FontIconSource();
public static readonly DependencyProperty FontFamilyProperty;
public FontFamily FontFamily { get; set; }
public static readonly DependencyProperty FontSizeProperty;
public double FontSize { get; set; }
public static readonly DependencyProperty FontStyleProperty;
public FontStyle FontStyle { get; set; }
public static readonly DependencyProperty FontWeightProperty;
public FontWeight FontWeight { get; set; }
public static readonly DependencyProperty GlyphProperty;
public string Glyph { get; set; }
}
public partial class IconSourceElementConverter : IValueConverter
{
public IconSourceElementConverter();
public object Convert(object value, Type targetType, object parameter, CultureInfo culture);
public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture);
}
[ContentProperty(nameof(Source))]
[MarkupExtensionReturnType(typeof(ImageIcon))]
public class ImageIconExtension : MarkupExtension
{
public ImageIconExtension(ImageSource? source);
[ConstructorArgument("source")]
public ImageSource? Source { get; set; }
public double Width { get; set; } = 16D;
public double Height { get; set; } = 16D;
}
[ContentProperty(nameof(Glyph))]
[MarkupExtensionReturnType(typeof(FontIcon))]
public class FontIconExtension : MarkupExtension
{
public FontIconExtension(string glyph);
public FontIconExtension(string glyph, FontFamily fontFamily);
[ConstructorArgument("glyph")]
public string Glyph { get; set; }
[ConstructorArgument("fontFamily")]
public FontFamily FontFamily { get; set; }
public double FontSize { get; set; }
} public partial class IconElement : System.Windows.FrameworkElement
{
public IconElement() { }
public static readonly System.Windows.DependencyProperty IconSourceProperty;
public System.Windows.Controls.IconSource IconSource { get { throw null; } set { } }
protected virtual System.Windows.UIElement InitializeChildren() { throw null; }
protected override int VisualChildrenCount { get { throw null; } }
protected override System.Windows.Size ArrangeOverride(System.Windows.Size finalSize) { throw null; }
protected override System.Windows.Media.Visual GetVisualChild(int index) { throw null; }
protected override System.Windows.Size MeasureOverride(System.Windows.Size availableSize) { throw null; }
}
public abstract partial class IconSource : System.Windows.DependencyObject
{
protected IconSource() { }
public static readonly System.Windows.DependencyProperty ForegroundProperty;
public System.Windows.Media.Brush Foreground { get { throw null; } set { } }
public abstract System.Windows.UIElement CreateIconContent();
}
public partial class FontIconSource : System.Windows.Controls.IconSource
{
public FontIconSource() { }
public static readonly System.Windows.DependencyProperty FontFamilyProperty;
public static readonly System.Windows.DependencyProperty FontSizeProperty;
public static readonly System.Windows.DependencyProperty FontStyleProperty;
public static readonly System.Windows.DependencyProperty FontWeightProperty;
public static readonly System.Windows.DependencyProperty GlyphProperty;
public System.Windows.Media.FontFamily FontFamily { get { throw null; } set { } }
public double FontSize { get { throw null; } set { } }
public System.Windows.FontStyle FontStyle { get { throw null; } set { } }
public System.Windows.FontWeight FontWeight { get { throw null; } set { } }
public string Glyph { get { throw null; } set { } }
public override System.Windows.UIElement CreateIconContent() { throw null; }
}
[System.Windows.Markup.ContentPropertyAttribute("Glyph")]
[System.Windows.Markup.MarkupExtensionReturnTypeAttribute(typeof(System.Windows.Controls.FontIconSource))]
public partial class FontIconSourceExtension : System.Windows.Markup.MarkupExtension
{
public FontIconSourceExtension(string glyph) { }
public FontIconSourceExtension(string glyph, System.Windows.Media.FontFamily fontFamily) { }
[System.Windows.Markup.ConstructorArgumentAttribute("fontFamily")]
public System.Windows.Media.FontFamily FontFamily { get { throw null; } set { } }
public double FontSize { get { throw null; } set { } }
[System.Windows.Markup.ConstructorArgumentAttribute("glyph")]
public string Glyph { get { throw null; } set { } }
public override object ProvideValue(System.IServiceProvider serviceProvider) { throw null; }
}I still don't understand the need for all those new classes. It would be nice to get an explanation from the team. As far as I can see all of this, aside from some missing markup extensions, can be achieved with existing classes.
KirillOsenkov
commented
9 months ago @terrajobst @bartonjs I really feel that this is rushed and it's not clear that this is a beneficial change at all.
I strongly agree with @batzen and @miloush and share his and others' concerns. This is a significant addition to the public API that in my view isn't warranted.
Can we make sure these are absolutely necessary? I'd like at the very least to see a small real WPF app that is using this API and an equivalent that just uses the existing API surface. I'd like it compared side-by-side.
It's strange to ship an API without a clear and compelling usage example that is real and not contrived.
miloush
commented
9 months ago For those who have not seen the recording, this was approved so that it can become part of public preview, with the anticipation that it will need changes and re-approval following feedback from the community. I would therefore encourage people to not give up and keep voicing their concerns.
It should also be said the proposed design seems to mirror WinUI. Note however, that WinUI does not have any vector Drawing support. I suspect much of the design there comes from this gap.
I agree that I am not convinced at all that the proposed infrastructure is needed. Based on the Alternative 2, I see two desires:
ContentControl having a Content, or, much more alike, Header for HeaderedContentControl and HeaderedItemsControl, and wasn't sure why are these two not satisfactory for the same purpose as IconElement. Now I am realizing that the elements are probably not supposed to render anything else other than the icon, in which case the question is how are they different from Images? In what way is
<IconSourceElement>
<IconSourceElement.IconSource>
...
</IconSourceElement.IconSource>
</IconSourceElement>better than
<Image>
<Image.Source>
...
</Image.Source>
</Image>?
TextBlock could be used for this purpose, but there is also the GlyphRun and corresponding Glyphs element. Most notably, there already is a GlyphRunDrawing that can be used as an image source. These options are not available in the WinUI platform. I do admit, however, that they are quite complicated to use. There could indeed be a markup extension that produces a DrawingImage with GlyphRunDrawing, or some kind of GlyphImage or GlyphRunImage : DrawingImage that would simplify this usage. I do not however see a point of IconSource being distinct from and not an ImageSource (it is also confusing with the term "Icon" as in IconBitmapDecoder).(I also reiterate my reservations against Glyph property when it doesn't mean a glyph in the font terminology.)
The API proposals tend to be out of context and do not show how they are planned to be used in wider context (not even in the Win11 templates) or implemented. As @KirillOsenkov suggested, some demonstration would be very useful.
singhashish-wpf
commented
9 months ago @batzen @miloush @KirillOsenkov Thanks for bringing these gaps to our knowledge. This proposal was created with the intent of bringing first class support for Icons in WPF and to bring WPF to parity with WinUI in terms of Icon support. From the above discussions there are concerns with the current state of the proposal and we will not be implementing this in its current state. We are anticipating more changes to this as per the feedback received, and they will be implemented only once there is an agreement with the proposal (as was stated in the API Proposal discussion meeting). We will provide more usage examples and side-by-side comparisons of approaches as requested. We are also open to receiving alternative designs to provide first-class support for Icons in WPF.
miloush
commented
9 months ago @singhashish-wpf I think part of the issue is that as far as I can tell from the issue tracker, no one really asked for "first class support for Icons", it is not clear what an "Icon" means and why is such support needed. It would be good to have these discussions before an API surface is put for approval.
It looked like "Icons" are needed for the Win11 theme effort, but that is perhaps not really the case and should stand their ground on their own.
KirillOsenkov
commented
9 months ago Also the best API normally emerges from usage, after refactoring and making sure it solves the actual problem well. Perhaps even test-driven, where you write usage first and then define the API to make the tests pass.
An experienced WPF developer should look at an API and immediately understand what it is for and how to use it. The roles should be clearly defined, like what is a Control, what is a Visual, what is a Panel. This is not the case here.
I do appreciate the work that was put in though!
Part of effort - https://github.com/dotnet/wpf/discussions/8655
Background and motivation
Icons are an important part of Win11 design system. With the release of Windows 11, the Segoe Fluent Icons font is the recommended symbol icon font. In this proposal, we have the API changes for supporting symbol icon font with ease in WPF applications.
User Scenarios
Code Example
API Proposals Iteration 1
IconElement
FontIcon
IconElement. Represents an icon that uses a glyph from the specified font.IconSourceElement
IconSourceobject as its content.ImageIcon
System.Windows.Controls.Imageas its content.IconSource
IconSourceElementto add icons to WPF applications. This is similar toIconElement, however since this is not aFrameworkElementit can be shared.FontIconSource
IconSourceElementConverter
IconSourceElementtoIconElementImageIconExtension
ImageIconUsage -
FontIconExtension
FontIconUsage -
Alternative Designs
In an another approach for the above, we can eliminate derived classes of
IconElement, have anIconSourceproperty inIconElement, and we keepIconSourceand derived classes (FontIconSource,ImageIconSource, ...) alongwith corresponding icon source extensions (FontIconSourceExtension,ImageIconSourceExtension, ... ).If we follow this approach, the usage of APIs will be as follows:
IconElement
As mentioned above, here is the IconElement API for the above approach:
IconSource, FontIconSource, FontIconSourceExtension, ...
Risks
Unknown
cc: @dotnet/dotnet-wpf-triage @pomianowski