Maui
Maui copied to clipboard
CommunityToolkit
110 gravatar image source
Description of Change
Added GravatarImageSource, along with GravatarImageSourceService for platform handlers.
Linked Issues
- Implements #110
PR Checklist
- [x] Has a linked Issue, and the Issue has been
approved(bug) orChampioned(feature/proposal) - [x] Has tests (if omitted, state reason in description)
- [x] Has samples (if omitted, state reason in description)
- [ ] Rebased on top of
mainat time of PR - [x] Changes adhere to coding standard
- [x] Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pull/127
Additional information
Platforms Tested
- [x] Android
- [ ] iOS
- [ ] MacCatalyst
- [x] Windows
Still to do:
- [x] Add samples for all controls that use ImageSource.
- [x] Add unit tests for all controls that use ImageSource.
- [x] Identify why AvatarView doesn't provide Parent in order to get image size. (Issue with AvatarView or GravatarImageSource?)
- [x] Documentation (in progress)
Commenter does not have sufficient privileges for PR 531 in repo CommunityToolkit/Maui
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}
ALL of the unit tests need to be re-worked! I'll undertake this ASAP.
This was great work @GeorgeLeithead thank you
"We" did great work. Community effort.
Thank you @GeorgeLeithead. I left some minor suggestions and improvements
It looks good to me, except for the cancellationTokenSourceTimeout. Maybe I don't like the number.
My recommendation is to add a Tuple<Uri, Stream, DateTime> for caching the result. Or even remove it at all. If it's only for the initialization, what can be worse for the customer - multiple requests or possible exceptions on a failed task?
It looks good to me, except for the
cancellationTokenSourceTimeout. Maybe I don't like the number. My recommendation is to add aTuple<Uri, Stream, DateTime>for caching the result. Or even remove it at all. If it's only for the initialization, what can be worse for the customer - multiple requests or possible exceptions on a failed task?
The number 737 is a personal number and probably should be a more standardised 1000ms. In my testing on the Windows Machine and Android emulators, less than 600ms didn't catch multiple property changes (from the samples), whereas 1000ms was a noticeable (IMO) delay to the end user. On average, the samples which set all properties (including parent sizes), from initial to last property took on average 688ms. (Other than Button which issues multiple size property changes).
It is worth noting that as we are inheriting from UriImageSource via 'StreamImageSource`, that caching is handled entirely by the .NET MAUI implementations.
It looks good to me, except for the
cancellationTokenSourceTimeout. Maybe I don't like the number. My recommendation is to add aTuple<Uri, Stream, DateTime>for caching the result. Or even remove it at all. If it's only for the initialization, what can be worse for the customer - multiple requests or possible exceptions on a failed task?The number 737 is a personal number and probably should be a more standardised 1000ms. In my testing on the Windows Machine and Android emulators, less than 600ms didn't catch multiple property changes (from the samples), whereas 1000ms was a noticeable (IMO) delay to the end user. On average, the samples which set all properties (including parent sizes), from initial to last property took on average 688ms. (Other than
Buttonwhich issues multiple size property changes).It is worth noting that as we are inheriting from
UriImageSourcevia 'StreamImageSource`, that caching is handled entirely by the .NET MAUI implementations.
please pay attention, that you tested it only on your local devices. and if we are talking about milliseconds 688 and 737 are really close. I believe we shouldn't rely on it
It looks good to me, except for the
cancellationTokenSourceTimeout. Maybe I don't like the number. My recommendation is to add aTuple<Uri, Stream, DateTime>for caching the result. Or even remove it at all. If it's only for the initialization, what can be worse for the customer - multiple requests or possible exceptions on a failed task?The number 737 is a personal number and probably should be a more standardised 1000ms. In my testing on the Windows Machine and Android emulators, less than 600ms didn't catch multiple property changes (from the samples), whereas 1000ms was a noticeable (IMO) delay to the end user. On average, the samples which set all properties (including parent sizes), from initial to last property took on average 688ms. (Other than
Buttonwhich issues multiple size property changes). It is worth noting that as we are inheriting fromUriImageSourcevia 'StreamImageSource`, that caching is handled entirely by the .NET MAUI implementations.please pay attention, that you tested it only on your local devices. and if we are talking about milliseconds 688 and 737 are really close. I believe we shouldn't rely on it
Here are some debug tests on a Windows i3 with 8gb RAM, running an Android emulator Nexus One with 1GB RAM. IMO this is a good debouce solution, however we can add in additional protection to validate that we dont dispatch a request for the LAST dispatch. The only case where a HTTP request was dispatched and then a later size change caused another dispatch is with Button, which raises three distinct size changes!
(FYI: format for time is ss.ffffff)
Image - with Width&Height and all Gravatar properties
=====================================================
31.261207: SizeChanged: -1
31.265947: SizeChanged: -1
31.304169: EmailChanged: [email protected]
32.228177: SizeChanged: 128
32.231513: SizeChanged: 128
32.236930: UriChanged: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=128&d=mp
33.006736: DISPATCHED: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=128&d=mp
then Email binding property changed
-----------------------------------
14.665828: EmailChanged: [email protected]
14.668010: UriChanged: www.gravatar.com/avatar/1bcc5826ac8088344367ba42364c287b?s=128&d=mp
15.420067: DISPATCHED: www.gravatar.com/avatar/1bcc5826ac8088344367ba42364c287b?s=128&d=mp
Image - NO Width or Height, all Gravatar properties
===================================================
50.899193: SizeChanged: -1
50.901930: SizeChanged: -1
50.923320: EmailChanged: [email protected]
51.511152: SizeChanged: 1403
51.513440: SizeChanged: 0
51.516996: UriChanged: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=1&d=mp
51.716750: SizeChanged: 1403
51.720186: UriChanged: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=1&d=mp
51.723737: CANCELED any previous Uri change request
52.471818: DISPATCHED: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=1&d=mp
then Email binding property changed
-----------------------------------
52.697250: EmailChanged: [email protected]
52.699449: UriChanged: www.gravatar.com/avatar/1bcc5826ac8088344367ba42364c287b?s=1&d=mp
53.452331: DISPATCHED: www.gravatar.com/avatar/1bcc5826ac8088344367ba42364c287b?s=1&d=mp
Button - NO width or height, all Gravatar properties
======================================================
(Note that a .NET MAUI button will automatically size, and throw additional resizes)
35.702429: SizeChanged: -1
35.706014: SizeChanged: -1
35.728156: EmailChanged: [email protected]
36.624258: SizeChanged: 1403
36.626533: SizeChanged: 39
36.630400: UriChanged: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
36.929835: SizeChanged: 62
36.935294: UriChanged: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
36.940141: CANCELED any previous Uri change request
37.677692: DISPATCHED: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
37.695511: SizeChanged: 42
37.697407: UriChanged: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
38.446581: DISPATCHED: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
then Email binding property changed
-----------------------------------
18.063451: EmailChanged: [email protected]
18.065691: UriChanged: www.gravatar.com/avatar/1bcc5826ac8088344367ba42364c287b?s=39&d=mp
18.816583: DISPATCHED: www.gravatar.com/avatar/1bcc5826ac8088344367ba42364c287b?s=39&d=mp
ImageButton - with Width&Height and all Gravatar properties
=====================================================
38.022284: SizeChanged: -1
38.025810: SizeChanged: -1
38.044222: EmailChanged: [email protected]
38.619604: SizeChanged: 72
38.621559: SizeChanged: 73
38.625540: UriChanged: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=72&d=mp
39.384202: DISPATCHED: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=72&d=mp
then Email binding property changed
-----------------------------------
09.466453: EmailChanged: [email protected]
09.468157: UriChanged: www.gravatar.com/avatar/1bcc5826ac8088344367ba42364c287b?s=72&d=mp
10.222458: DISPATCHED: www.gravatar.com/avatar/1bcc5826ac8088344367ba42364c287b?s=72&d=mp
I've added in an additional check so that only if the HTTP request does not match the previous, do we dispatch a new HTTP request:
Button - NO width or height, all Gravatar properties
======================================================
(Note that a .NET MAUI button will automatically size, and throw additional resizes)
35.702429: SizeChanged: -1
35.706014: SizeChanged: -1
35.728156: EmailChanged: [email protected]
36.624258: SizeChanged: 1403
36.626533: SizeChanged: 39
36.630400: UriChanged: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
36.929835: SizeChanged: 62
36.935294: UriChanged: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
36.940141: CANCELED any previous Uri change request
37.677692: DISPATCHED: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
37.695511: SizeChanged: 42
37.697407: UriChanged: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
38.446581: DISPATCHED: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
Button + new last dispatch check - NO width or height, all Gravatar properties
======================================================
(Note that a .NET MAUI button will automatically size, and throw additional resizes)
14.056019: SizeChanged: -1
14.060296: SizeChanged: -1
14.078675: EmailChanged: [email protected]
14.804419: SizeChanged: 1403
14.806311: SizeChanged: 39
14.809717: UriChanged: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
14.968115: SizeChanged: 62
14.970190: UriChanged: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
14.972147: CANCELED any previous Uri change request
15.721908: DISPATCHED: www.gravatar.com/avatar/b65a519785f69fbe7236dd0fd6396094?s=39&d=mp
15.749049: SizeChanged: 42
15.751199: SAME as PREVIOUS, no dispatch
Hey Gang! Just checking in - how close is the PR to completion?
I'd love to include it in the next release if this PR will be ready to merge by the end of next week!
@brminnick It is very close. There are 2 unresolved questions: https://github.com/CommunityToolkit/Maui/pull/531/files#r959566281 and https://github.com/CommunityToolkit/Maui/pull/531/files#r956334081
Hey Gang! Just checking in - how close is the PR to completion?
I'd love to include it in the next release if this PR will be ready to merge by the end of next week!
@brminnick , @VladislavAntonyuk and @bijington have been a great help on this. Vlad and I have a small difference of opinion in regards to the debounce implementation, perhaps it needs additional eyes looking ait it. Other than that, I think it's good.
Unit test code coverage is good.
I think it still needs device verification on iOS and MacCatalyst, for completeness.
@GeorgeLeithead do you believe this PR is good to merge?
@bijington I believe it is good to merge, once it has been verified on both iOS and MacCatalyst.
Hey Gang! Just checking in - are we good to merge this PR today or tomorrow (Wednesday or Thursday)?
We plan on pushing a new release to NuGet on Friday and would love to include this PR!
If it needs more time, no worries! We can add it in the next release 💯
I have finally been able to sort out my dev environment thanks to @brminnick and @VladislavAntonyuk.
The majority of the sample app looks fine. Currently though the Button sample doesn't appear to render the GravatarImageSource on iOS, Android or macOS (I haven't tested Windows as I am on a Mac right now).
I will try and take a look at why now
I haven't been able to confirm this yet but it looks like the Button implementation doesn't result in calling the Stream func once the Uri has actually been set. ImageButton does appear to do this
Windows
Android
I'm investigating, but initial thoughts are that this is a change within .NET MAUI in that it no longer appears to raise Size changed events like previous.
NOTE, that I specifically did not include WidthRequest or HeighrRequest in this sample because it did raise multiple size events.
NOTE: The Button control has changed somewhat in that previously the Image was aligned next to the text.
@GeorgeLeithead I found that the code to set the Uri based on size changed did happen and the image source code even called OnSourceChanged but the Download func wasn't called again. This smells like a .NET MAUI issue but it would be great to prove it
@GeorgeLeithead I found that the code to set the Uri based on size changed did happen and the image source code even called OnSourceChanged but the Download func wasn't called again. This smells like a .NET MAUI issue but it would be great to prove it
@bijington I know for absolutely certain it WAS working, and no code has changed since then. I agree, it smells like a .NET MAUI issue, but need to prove.
I am still happy to go ahead and merge this. We can always open a bug detailing the issue with Button if we feel we need to