IImageSourceHandler impl #70

ysmoradi · 2020-08-04T09:36:46Z

This closes #69
To test it simply add followings to MainPage.xaml

NavigationPage.TitleIconImageSource="https://www.google.com/images/branding/googlelogo/1x/googlelogo_color_272x92dp.png"

This uses Glidex.forms image handler's method which accepts ImageSource only for UriImageSources

We need another issue in XF, because it calls our handler's method for UriImageSources but it doesn't call the same method for FileImageSources and StreamImageSources )-:

ysmoradi · 2020-08-04T10:13:34Z

Please accept this PR because of followings:

1- I know this works for UriImageSources and it's not working for other image source types, but UriImageSource is more important because it needs authentication in some scenarios and if we pass custom IGlideHandler which adds required request headers, at least everything works (Image and ImageView and UriImageSource will gets handled by Glide and StreamImageSource and FileImageSource will gets handled by XF which is not high performant, but at least works!)

2- After XF fixes issues described here, our ImageSourceHandler will handle all ImageSource types without releasing new version of glidex.forms which improves xf apps performance even more!

jonathanpeppers · 2020-08-04T12:57:53Z

@ysmoradi Why can't Xamarin.Forms just always call IImageViewHandler.LoadImageAsync? If it can, let's change Xamarin.Forms instead?

ysmoradi · 2020-08-04T13:32:04Z

@ysmoradi Why can't Xamarin.Forms just always call IImageViewHandler.LoadImageAsync? If it can, let's change Xamarin.Forms instead?

From this perspective, IImageSourceHandler interface is useless.
They've introduced two interfaces for two different reasons

IImageViewHandler => For Image & ImageButtons
IImageSourceHandlers => For ImageSources of background, icons, toolbar item etc.

But, as I'm not completly in and have no idea for all historical descicions, I'm happy to ask them about this.
I just took a look at FFImageLoading handler which is implementing IImageSourceHandler only and I think it's fine to implement IImageSourceHandler.

I'll open an issue in xamarin forms tomorrow

Thanks for paying attention

ysmoradi · 2020-08-06T16:27:08Z

xamarin/Xamarin.Forms#11676

ysmoradi · 2020-08-07T18:47:23Z

@roubachof Do you have any idea about this?
It seems you've implemented IImageSourceHandler only
Thanks in advance

ysmoradi · 2020-08-10T12:50:52Z

@daniel-luberda Do you have any idea about this?
It seems you've implemented both IImageSourceHandler and IImageViewHandler
Thanks in advance

ysmoradi · 2020-08-12T06:57:55Z

@jonathanpeppers I've submitted an issue to XF
I've also tried to contact with FFImageLoading & Nuke maintainers
No luck!
Could you please merge this PR, so we can make UriImageSources work?
As I said, whenever XF team supports IImageSourceHandler completely for non Image/ImageButton images (such as Page's icon etc), our code will remain unchanged.
I'd rather the idea to only have IImageViewHandler, but this needs efforts from XF team, and it seems they've other priorities )":
Thanks in advance

jonathanpeppers · 2020-08-12T13:35:57Z

@ysmoradi did you send a PR to Xamarin.Forms to fix it?

ysmoradi · 2020-08-12T14:45:54Z

@jonathanpeppers I've submitted an issue. I'm fine with PR too, but first of all, someone should give me basic understanding on XF image handling structure!

It's totally unclear! Look at GlideX.Forms | Nuke | FFImageLoading:
One has implemented IImageViewHandler only!
Another one has implemented IImageSourceHandler only!
Another one has implemented both!

I'm an active person on GitHub as you can see on my profile, and I've submitted lots of PRs to several repositories. But I can't reverse engineering entire codes to submit a PR, and at the end someone says "hey! your assumptions were wrong!"

A good PR comes with an good issue first!

ysmoradi · 2020-08-12T14:48:56Z

I see a few details are provided in my issue, I try to submit a PR, but can't give my word

Context: #70 Multiple things are cleaned up in #70, I'm pulling over the parts that make sense. * Use `MemoryStream.CopyToAsync()` Co-authored-by: Yaser Moradi <ysmoradi@outlook.com>

Useful improvements from PR #70

jonathanpeppers · 2020-09-25T19:59:04Z

glidex.forms/GlideExtensions.cs

+				if (builder is null) {
+					return null;
+				} else {
+					var result = await builder.Submit ().GetAsync ();


This isn't the proper way to await an IFuture with cancellation. This should be something like:

Suggested change

var result = await builder.Submit ().GetAsync ();

var future = builder.Submit ();

var result = await Task.Run (() => future.Get (), token);

jonathanpeppers · 2020-09-25T20:00:01Z

glidex.forms/GlideExtensions.cs

+					return null;
+				} else {
+					var result = await builder.Submit ().GetAsync ();
+					return (Bitmap) result;


This code throws InvalidCastException because result is BitmapDrawable!

Suggested change

return (Bitmap) result;

if (result is BitmapDrawable drawable)

return drawable.Bitmap;

jonathanpeppers · 2020-09-25T20:00:41Z

glidex.forms/GlideExtensions.cs

+				switch (source) {
+					case FileImageSource fileSource:
+						var fileName = fileSource.File;
+						var drawable = ResourceManager.GetDrawableByName (fileName);
+						if (drawable != 0) {
+							Forms.Debug ("Loading `{0}` as an Android resource", fileName);
+							builder = request.AsBitmap ().Load (drawable);
+						} else {
+							Forms.Debug ("Loading `{0}` from disk", fileName);
+							builder = request.AsBitmap ().Load (fileName);
+						}
+						break;
+
+					case UriImageSource uriSource:
+						var url = uriSource.Uri.OriginalString;
+						Forms.Debug ("Loading `{0}` as a web URL", url);
+						builder = request.AsBitmap ().Load (url);
+						break;
+
+					case StreamImageSource streamSource:
+						Forms.Debug ("Loading `{0}` as a byte[]. Consider using `AndroidResource` instead, as it would be more performant", nameof (StreamImageSource));
+						using (var memoryStream = new MemoryStream ())
+						using (var stream = await streamSource.Stream (token)) {
+							if (token.IsCancellationRequested || stream == null)
+								return null;
+							if (!IsActivityAlive (context, source)) {
+								return null;
+							}
+							await stream.CopyToAsync (memoryStream, token);
+							builder = request.AsBitmap ().Load (memoryStream.ToArray ());
+						}
+						break;
+				}


This long switch statement is a lot of copy-paste. This should be refactored out into smaller methods that the other LoadViaGlide() method can share.

jonathanpeppers · 2020-09-25T20:01:51Z

glidex.forms/IGlideHandler.cs

@@ -19,5 +19,6 @@ public interface IGlideHandler
 		/// <param name="token">The CancellationToken if you need it</param>
 		/// <returns>True if the image was handled. Return false if you need the image to be cleared for you.</returns>
 		bool Build (ImageView imageView, ImageSource source, RequestBuilder builder, CancellationToken token);
+		void Build (ImageSource source, RequestBuilder builder, CancellationToken token);


This should be a C# 8 default interface method, so that existing IGlideHandler implementations won't get a compilation error.

This should also use ref RequestBuilder builder so that IGlideHandler's can set the builder to null if desired.

jonathanpeppers · 2020-09-25T20:02:08Z

glidex.forms/ImageViewHandler.cs

+
+		public async Task<Bitmap> LoadImageAsync (ImageSource source, Context context, CancellationToken token = default)
+		{
+			Forms.Debug ("IImageViewHandler of type `{0}`, `{1}` called.", GetType (), nameof (LoadImageAsync));


Suggested change

Forms.Debug ("IImageViewHandler of type `{0}`, `{1}` called.", GetType (), nameof (LoadImageAsync));

Forms.Debug ("IImageSourceHandler of type `{0}`, `{1}` called.", GetType (), nameof (LoadImageAsync));

jonathanpeppers · 2020-09-25T20:26:41Z

Closing in favor of #77, let me know if those changes will work for you.

ysmoradi · 2020-09-26T07:27:52Z

Thanks so much
We've switched from TabbedPage's icon to custom views, but I can test it for sure
Is there any pre-release for this or I've to build it manually? (I can do that anyway)

jonathanpeppers · 2020-09-28T16:06:45Z

There are .nupkg files on CI here: https://jopepper.visualstudio.com/Jon%20Peppers%20OSS/_build/results?buildId=502&view=artifacts&type=publishedArtifacts

I think I want to have a fix for #71 and VisualStateManager before doing another release.

ysmoradi added 2 commits August 4, 2020 13:54

IImageSourceHandler impl

df7fe26

Fixed small issue

0f3b974

jonathanpeppers mentioned this pull request Sep 25, 2020

Useful improvements from PR #70 #76

Merged

jonathanpeppers added a commit that referenced this pull request Sep 25, 2020

Merge pull request #76 from jonathanpeppers/pr-70-improvements

a3b4c19

Useful improvements from PR #70

jonathanpeppers reviewed Sep 25, 2020

View reviewed changes

jonathanpeppers closed this Sep 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IImageSourceHandler impl #70

IImageSourceHandler impl #70

ysmoradi commented Aug 4, 2020

ysmoradi commented Aug 4, 2020 •

edited

Loading

jonathanpeppers commented Aug 4, 2020

ysmoradi commented Aug 4, 2020

ysmoradi commented Aug 6, 2020

ysmoradi commented Aug 7, 2020 •

edited

Loading

ysmoradi commented Aug 10, 2020 •

edited

Loading

ysmoradi commented Aug 12, 2020

jonathanpeppers commented Aug 12, 2020

ysmoradi commented Aug 12, 2020 •

edited

Loading

ysmoradi commented Aug 12, 2020

jonathanpeppers Sep 25, 2020

jonathanpeppers Sep 25, 2020

jonathanpeppers Sep 25, 2020

jonathanpeppers Sep 25, 2020

jonathanpeppers Sep 25, 2020

jonathanpeppers commented Sep 25, 2020

ysmoradi commented Sep 26, 2020

jonathanpeppers commented Sep 28, 2020

	var result = await builder.Submit ().GetAsync ();
	var future = builder.Submit ();
	var result = await Task.Run (() => future.Get (), token);

	return (Bitmap) result;
	if (result is BitmapDrawable drawable)
	return drawable.Bitmap;

	Forms.Debug ("IImageViewHandler of type `{0}`, `{1}` called.", GetType (), nameof (LoadImageAsync));
	Forms.Debug ("IImageSourceHandler of type `{0}`, `{1}` called.", GetType (), nameof (LoadImageAsync));

IImageSourceHandler impl #70

IImageSourceHandler impl #70

Conversation

ysmoradi commented Aug 4, 2020

ysmoradi commented Aug 4, 2020 • edited Loading

jonathanpeppers commented Aug 4, 2020

ysmoradi commented Aug 4, 2020

ysmoradi commented Aug 6, 2020

ysmoradi commented Aug 7, 2020 • edited Loading

ysmoradi commented Aug 10, 2020 • edited Loading

ysmoradi commented Aug 12, 2020

jonathanpeppers commented Aug 12, 2020

ysmoradi commented Aug 12, 2020 • edited Loading

ysmoradi commented Aug 12, 2020

jonathanpeppers Sep 25, 2020

Choose a reason for hiding this comment

jonathanpeppers Sep 25, 2020

Choose a reason for hiding this comment

jonathanpeppers Sep 25, 2020

Choose a reason for hiding this comment

jonathanpeppers Sep 25, 2020

Choose a reason for hiding this comment

jonathanpeppers Sep 25, 2020

Choose a reason for hiding this comment

jonathanpeppers commented Sep 25, 2020

ysmoradi commented Sep 26, 2020

jonathanpeppers commented Sep 28, 2020

ysmoradi commented Aug 4, 2020 •

edited

Loading

ysmoradi commented Aug 7, 2020 •

edited

Loading

ysmoradi commented Aug 10, 2020 •

edited

Loading

ysmoradi commented Aug 12, 2020 •

edited

Loading