Min viable FontImageSource #4747

kingces95 · 2018-12-15T03:02:55Z

Description of Change

FontImageSource derives from ImageSource and accepts a glyph (character) from a font plus a size and color which it'll use to create a .png which can then be displayed as an image. Unfortunately, many places where you'd like to use this currently accept a FileImageSource but that'll be a separate work item.

Issues Resolved

fixes Xamarin.Forms.FontIconSource Spec #3189 tho that spec has aspirations of unifying the metadata needed to identify the font such that the XAML is simplified to a single word. That's a great idea but is beyond what's necessary for a minimum viable feature. And anyway, abstracting resources across platforms is a larger issue.

API Changes

Added:
FontImageSource
see https://github.com/xamarin/Xamarin.Forms/compare/font-image-source?expand=1#diff-6e54598e403bbd75200d983f2182df8fR564

Before/After Screenshots

Testing Procedure

Run Font ImageSource - Legacy

charlesroddie · 2018-12-15T10:06:20Z

Why the intermediate png?

StephaneDelcroix · 2018-12-18T10:41:58Z

Xamarin.Forms.Core/FontImageSource.cs

+{
+	public partial class FontImageSource : ImageSource
+	{
+		private static BindableProperty CreateBindableProperty<T>(


I'd avoid trying to be smart here. it makes the code harder to understand in the big picture. this code is basically boilerplate. And I agree we could make this boilerplate thinner, but not here, ideally...

StephaneDelcroix · 2018-12-18T10:43:45Z

Xamarin.Forms.Core/FontImageSource.cs

+
+namespace Xamarin.Forms
+{
+	public partial class FontImageSource : ImageSource


ImageSourceConverter needs to be extended with a new format to create a FontImageSource easily from XAML. please propose something

StephaneDelcroix

weren't we discussing an UWP approach too ?

paymicro · 2018-12-18T15:32:19Z

I have an implementation for UWP. Do we need to add this?

Xamarin.Forms.Controls/CoreGallery.cs

Xamarin.Forms.Controls/GalleryPages/FontImageSourceGallery.cs

Xamarin.Forms.Platform.Android/Renderers/FontExtensions.cs

Xamarin.Forms.Core/FontImageSource.cs

* [UWP] font image source * address comment * [UWP] performance optimization of the rendering of icons font - the minimum Dpi of the icon is 300

kingces95 · 2018-12-19T21:39:01Z

Why the intermediate png?

To allow emoji to be used everywhere an ImageSource is allowed as opposed to just where a label is allowed. At least, that's the dream. We still need to change FileImageSource to ImageSource all over the place to realize the dream. And yes, the dream does come at the cost of drawing a png...

mattleibow · 2018-12-19T23:38:45Z

If we cache the image somehow, then this really is an optimization - in a sense - because we don't have to redraw the emoji each time.

mattleibow · 2018-12-19T23:42:11Z

Xamarin.Forms.Platform.UAP/FontImageSourceHandler.cs

+				return null;
+
+			var device = CanvasDevice.GetSharedDevice();
+			var dpi = Math.Max(_minimumDpi, Windows.Graphics.Display.DisplayInformation.GetForCurrentView().LogicalDpi);


Just a note about DPI on UWP: it can change during the lifetime of an app and even the control. The density can be changed via settings, but also by just dragging the app to a new monitor. I have a 200% surface and a 150% externbal display.

This might also change if accessibility is used - possibly on iOS and Android as well.

Truth! But should fixing that use case block the PR? Kinda sounds like a corner case on a corner platform, no? Otherwise, we'll have to detect that the DPI changed and then run around and update glyphs? Sounds icky (or is it simple)? Do we think this is the only place we have that issue? Which is to say, if there are other places we cache DPI specific resources then how about we open an enhancement, subject it to prioritization, and fix 'em all at once.

I'm outta chaff.

we'll have to detect that the DPI changed and then run around and update glyphs?

it simple.

I agree this is not a blocking issue at this time, but just wanted to make sure we all knew about this possibility. And, as you mention, we possibly have the same issue in other areas - maybe we need another issue...

Nice. So we'll open a separate "Fix multimon DPI change on UAP" and fixing FontImageSource can be task one?

Yeah. We probably can actually make this a public concept - the app may want to adjust to things like this. I was doing this for macOS for Xamarin.Essentials - a way to track when the resolution/scale changes. If the OS does not support it, then the event just never fires.

macOS

NSNotificationCenter.DefaultCenter.AddObserver(NSApplication.DidChangeScreenParametersNotification, OnDidChangeScreenParameters);

UWP:

var di = DisplayInformation.GetForCurrentView(); di.DpiChanged += OnDisplayInformationChanged;

mattleibow · 2018-12-19T23:43:05Z

Xamarin.Forms.Platform.UAP/ImageRenderer.cs

+			else
+			{
+				throw new InvalidCastException($"\"{Control.Source.GetType().FullName}\" is not supported.");
+			}


I would move this into an extension method because it probably needs to be used on other image-bearing controls - such as image buttons or page icons.

More Truth! But if we're gonna go on a de-duplication jihad, how about we start with BP definition boilerplate (see above...).

kingces95 · 2018-12-20T08:29:24Z

Caching the images would be a win, but how about we wait for a customer to complain first. Not trying to be facetious, but caches also have their problems... They take up memory, they need to be flushed, they need to be invalidated (if we ever do the DPI stuff). If anything, I'd expose the png so the user can cache and own the policy.

Or do we already have an image cache?

samhouts · 2019-01-03T17:46:10Z

Failing tests are unrelated to this PR

* min viable fontImageSource * [UWP] Min viable FontImageSource (#4817) * [UWP] font image source * address comment * [UWP] performance optimization of the rendering of icons font - the minimum Dpi of the icon is 300 * char -> string; CR feedback

* min viable fontImageSource * [UWP] Min viable FontImageSource (xamarin#4817) * [UWP] font image source * address comment * [UWP] performance optimization of the rendering of icons font - the minimum Dpi of the icon is 300 * char -> string; CR feedback

minaairsupport · 2019-02-11T23:05:54Z

Nice feature but I think it will be better if you have a helper class like that as no one will memorize all this Glyph

https://github.com/neilkennedy/FontAwesome.Xamarin/blob/master/FontAwesome.Xamarin/FontAwesome.cs

andreinitescu · 2019-05-22T05:18:40Z

Is it really necessary to keep an array in memory, even if it's just 4 items, having to iterate over it using a loop with temporary variables?
https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Core/FontImageSource.cs#L47
Why not just use a switch like how all the other code in the framework uses

Also having CreateBindableProperty just in that class seems odd to me, at least why not put it in BindableObject or an utility class...

min viable fontImageSource

26d36ae

kingces95 requested a review from StephaneDelcroix December 15, 2018 03:02

samhouts added this to In Review in v3.6.0 Dec 15, 2018

samhouts added this to Ready for Review (PRs) in Sprint 145 Dec 15, 2018

samhouts added the t/enhancement ➕ label Dec 15, 2018

samhouts added this to Ready for Review (PRs) in Sprint 146 Dec 17, 2018

samhouts added the e/6 🕕 6 label Dec 17, 2018

samhouts assigned paymicro Dec 17, 2018

samhouts requested a review from paymicro December 17, 2018 18:31

samhouts assigned StephaneDelcroix Dec 17, 2018

StephaneDelcroix reviewed Dec 18, 2018

View reviewed changes

StephaneDelcroix approved these changes Dec 18, 2018

View reviewed changes

paymicro approved these changes Dec 18, 2018

View reviewed changes

paymicro mentioned this pull request Dec 18, 2018

[UWP] Min viable FontImageSource #4817

Merged

3 tasks

samhouts added the approved Has two approvals, no pending reviews, and no changes requested label Dec 18, 2018

mattleibow self-requested a review December 18, 2018 19:30

mattleibow reviewed Dec 18, 2018

View reviewed changes

Pavel Yakovlev and others added 2 commits December 19, 2018 10:36

[UWP] Min viable FontImageSource (#4817)

7ca231e

* [UWP] font image source * address comment * [UWP] performance optimization of the rendering of icons font - the minimum Dpi of the icon is 300

char -> string; CR feedback

98a2fcb

Merge branch 'master' into font-image-source

1a5df25

mattleibow reviewed Dec 19, 2018

View reviewed changes

jsmarcus mentioned this pull request Dec 27, 2018

Iconize to set the Icon of Master/MasterDetailPage jsmarcus/Iconize#117

Open

samhouts mentioned this pull request Jan 3, 2019

Replace all occurrences of FileImageSource with ImageSource #3207

Closed

samhouts merged commit 5716f8d into master Jan 3, 2019

v3.6.0 automation moved this from In Review to Done Jan 3, 2019

Sprint 146 automation moved this from Ready for Review (PRs) to Done Jan 3, 2019

samhouts deleted the font-image-source branch January 3, 2019 17:46

samhouts added this to Done in v4.0.0 Feb 2, 2019

samhouts removed this from Done in v3.6.0 Feb 2, 2019

samhouts modified the milestones: 4.0.0, 3.6.0 Feb 6, 2019

samhouts removed this from Done in v4.0.0 Feb 7, 2019

samhouts added this to Done in v3.6.0 Feb 7, 2019

samhouts added p/iOS 🍎 p/Android labels Jul 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Min viable FontImageSource #4747

Min viable FontImageSource #4747

kingces95 commented Dec 15, 2018 •

edited

Loading

charlesroddie commented Dec 15, 2018

StephaneDelcroix Dec 18, 2018 •

edited

Loading

StephaneDelcroix Dec 18, 2018

StephaneDelcroix left a comment

paymicro commented Dec 18, 2018

kingces95 commented Dec 19, 2018

mattleibow commented Dec 19, 2018

mattleibow Dec 19, 2018

kingces95 Dec 20, 2018

paymicro Dec 20, 2018

mattleibow Dec 20, 2018

kingces95 Dec 21, 2018

mattleibow Dec 21, 2018

mattleibow Dec 19, 2018

kingces95 Dec 20, 2018

kingces95 commented Dec 20, 2018

samhouts commented Jan 3, 2019

minaairsupport commented Feb 11, 2019

andreinitescu commented May 22, 2019

Min viable FontImageSource #4747

Min viable FontImageSource #4747

Conversation

kingces95 commented Dec 15, 2018 • edited Loading

Description of Change

Issues Resolved

API Changes

Before/After Screenshots

Testing Procedure

charlesroddie commented Dec 15, 2018

StephaneDelcroix Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephaneDelcroix left a comment

Choose a reason for hiding this comment

paymicro commented Dec 18, 2018

kingces95 commented Dec 19, 2018

mattleibow commented Dec 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingces95 commented Dec 20, 2018

samhouts commented Jan 3, 2019

minaairsupport commented Feb 11, 2019

andreinitescu commented May 22, 2019

kingces95 commented Dec 15, 2018 •

edited

Loading

StephaneDelcroix Dec 18, 2018 •

edited

Loading