search

davidxuang / FluentIcons

A multi-framework wrapper of https://github.com/microsoft/fluentui-system-icons
MIT License
72 stars 6 forks source link

fix markup extension for avalonia #11

Closed StefanKoell closed 2 months ago

StefanKoell commented 2 months ago
davidxuang commented 2 months ago

No, this UseSegoeMetricsDefaultValue implementation is wrong. It needs to supplied by a callback. like in https://github.com/davidxuang/FluentIcons/blob/52a57f6516796a0900eb8b2468322e1eed12be17/FluentIcons.WinUI/SymbolIcon.cs#L24, which Avalonia did not implement. It may be logical to allow global value only for this property.

Maybe make the property Lazy<> will also do, but I'm not sure, plus this solution has a racing issue.

StefanKoell commented 2 months ago

I did test this and for me it worked when I used the AppBuilderExtension to set the internal static UseSegoeMetricsDefaultValue to true. I double checked the results. Where do you see a potential racing condition?

davidxuang commented 2 months ago

I did test this and for me it worked when I used the AppBuilderExtension to set the internal static UseSegoeMetricsDefaultValue to true. I double checked the results. Where do you see a potential racing condition?

Weird. I suppose that all static fields has to be initialized before it can be changed, so the property would have been created before UseSegoeMetricsDefaultValue could be altered.

StefanKoell commented 2 months ago

I think it works because the AppBuilder extension is currently the only way to alter the default and this is executed before the whole Avalonia infrastructure is built. So at the time the styled properties are set, the static default value is already set. That's my guess at least. If it should be possible to alter the value afterwards, it has to be implemented differently but in that case, the ability to use style selectors to "override" the value will be gone.

A bit off topic but this setting is a mystery to me. Is there any documentation what value this should be? Is it encouraged to set it to true?

davidxuang commented 2 months ago

A bit off topic but this setting is a mystery to me. Is there any documentation what value this should be? Is it encouraged to set it to true?

It enables a modified version of fluentui-system-icons, so that it looks more similar to Segoe fonts. Check https://github.com/davidxuang/FluentIcons/blob/master/seagull-icons/README.md.

The most significant change is that Segoe uses a stroke width of 1/16 (of size of bounding square) while fluentui-system-icons uses 1/20 and has a large padding. This is done by removing padding and re-drawing some symbols which are out of bound. Re-drawing are done by hand (https://github.com/davidxuang/FluentIcons/tree/master/seagull-icons/override) or semi-automated composition (https://github.com/davidxuang/FluentIcons/blob/master/seagull-icons/transform.ts).

StefanKoell commented 2 months ago

It enables a modified version of fluentui-system-icons, so that it looks more similar to Segoe fonts. Check https://github.com/davidxuang/FluentIcons/blob/master/seagull-icons/README.md.

The most significant change is that Segoe uses a stroke width of 1/16 (of size of bounding square) while fluentui-system-icons uses 1/20 and has a large padding. This is done by removing padding and re-drawing some symbols which are out of bound. Re-drawing are done by hand (https://github.com/davidxuang/FluentIcons/tree/master/seagull-icons/override) or semi-automated composition (https://github.com/davidxuang/FluentIcons/blob/master/seagull-icons/transform.ts).

Thanks for the clarifications. This explains a couple of things. I have other icon sources as well and they are designed according to the fluent system guidelines (smaller appearance, padding around, etc.). So in this case I need to keep the UseSegoeMetricsDefaultValue set to false and compensate with the FontSize to align the icon sizes. The icons from my other source doesn't allow me to remove the padding/change the stroke width.

Regarding the PR: Do you think the implementation as it is in the PR is OK or should set the value in the ctor again. It will not have any impact for me as I'm not using the UseSegoeMetricsDefaultValue = true

davidxuang commented 2 months ago

I didn't see it in the changelog, but is there any issue caused by inheriting MarkupExtension?

Regarding the PR: Do you think the implementation as it is in the PR is OK or should set the value in the ctor again. It will not have any impact for me as I'm not using the UseSegoeMetricsDefaultValue = true

I need more tests on this.

StefanKoell commented 2 months ago

I think the requirement to inherit from MarkupExtension is deprecated IIRC. The official docs show MarkupExtension as a standalone class: https://docs.avaloniaui.net/docs/concepts/markupextensions

I also briefly looked at the Avalonia source code and most MarkupExtensions are not inheriting from MarkupExtension: https://github.com/AvaloniaUI/Avalonia/tree/f140033e420ee39b16dcc0621d5580f94d5bcb2c/src/Markup/Avalonia.Markup.Xaml/MarkupExtensions

davidxuang commented 2 months ago

Issues addressed.