Graph authentication improvements #2035

GraniteStateHacker · 2018-04-29T19:24:25Z

Updates logo for graph authentication service
Makes v2 MSAL default authentication provider
Updates Microsoft.Graph to package 1.8.1
Updates Microsoft.Identity.Client to 1.1.2-preview0008
Adds thumbnail & file size where available to OneDrive file items.

Issue: #
Related to requests by Adam Braden & team at Microsoft (by individual request)

PR Type

What kind of change does this PR introduce?
Enhancement

* Makes v2 MSAL default authentication provider * Updates Microsoft.Graph to package 1.8.1 * Updates Microsoft.Identity.Client to 1.1.2-preview0008

dnfclas · 2018-04-29T19:24:36Z

All CLA requirements met.

…matted file size.

We no longer ask the user for an MSA/O365 account provider.

adds section for testing in Sample app.

michael-hawker · 2018-05-04T17:40:48Z

Microsoft.Toolkit.Services/Services/OneDrive/OneDriveStorageFile.cs

+        /// <summary>
+        /// Gets the smallest available thumbnail for the object
+        /// </summary>
+        public string Thumbnail


Should this be a Uri vs. a string?

Given that the XAML binding to Image.Source works as a string, I suggest letting it fly as is, and if folks need it as a URI, let them convert it as needed. Not gonna fall on my sword over it though.

michael-hawker · 2018-05-06T06:57:14Z

Microsoft.Toolkit.Services/Services/OneDrive/OneDriveStorageItem.cs

        {
            get
            {
-                return _path;
+                var size = _fileSize.HasValue ? _fileSize.Value : 0;
+                if (size < 1024)


PR #2055 was doing something similar, could we make this an extension for long?

They started pulling it out here: https://github.com/Microsoft/WindowsCommunityToolkit/pull/2055/files#diff-e62611f48a9fc0cbaed40c2e841f0b25R30, but we should coordinate on needs/formatting.

I think this implementation is fine the way it is here, let's pull it out in a different PR if needed

azchohfi · 2018-05-17T21:45:16Z

Microsoft.Toolkit.Services/Services/OneDrive/OneDriveStorageFile.cs

+                {
+                    _thumbnail = string.Empty;
+                }
+            }).Wait();


I really don't like doing this, but I can't think of anything that will have the same behavior (lazy loaded async property). @nmetulev ?

Good catch. wouldn't this block the UI? Should we just have an asynchronous method to retrieve Thumbnails?

I think that is the best approach. But what if the developer wants to bind an image's Source property to it? I hate every since solution that I can think for this.

I'd expect the developer to add it to their view model that extends INotifyProperty - seems like the cleanest approach

This is lazy load, which is nice, since we don't want every thumbnail to load all the time, but when you'll get it when you ask for it without overhead on the client side. It will prevent the get from completing while the request is in process but other UI thread actions will continue while the request is in process... so you can bind to it without issue. If you run it, the result is a generally pleasant experience.

@GraniteStateHacker If you bind directly to it, it will block the UI Thread for as long as that GetThumbnailSetAsync method takes to do the request. Just remove that and add a Task.Delay(10000), to test it.
It is lazy loading it, but it is still blocking the UI Thread until it does it (that's what the .Wait() is doing).
This class is a Model, not a ViewModel, so it should not be bind directly to. Properties should return immediately (<50ms). This property should be replaced by a method that returns a Task, this way, the developer can wait for it's completion and create their own VM that handles that.

Gotcha, sorry, I was forgetting that Task.Run doesn't keep you on the UI thread, so it doesn't matter that the await is used within it.

👍 Once you remove this property @GraniteStateHacker , I think we should be good to go.

… by the bound property.

GraniteStateHacker · 2018-05-22T11:24:51Z

I’ve got some code I’m about to check in around this… should be better… sorry for the delay, I tried to sneak some work in while I was at NE Give Camp this past weekend, but wasn’t quite able to get the last few tweaks in. Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for Windows 10

________________________________ From: Nikola Metulev <notifications@github.com> Sent: Monday, May 21, 2018 6:30:07 PM To: Microsoft/WindowsCommunityToolkit Cc: Jim Wilcox (The Granite State Hacker); Mention Subject: Re: [Microsoft/WindowsCommunityToolkit] Graph authentication improvements (#2035) @nmetulev commented on this pull request.

________________________________ In Microsoft.Toolkit.Services/Services/OneDrive/OneDriveStorageFile.cs<https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2FWindowsCommunityToolkit%2Fpull%2F2035%23discussion_r189731762&data=02%7C01%7C%7C527dc72f71cf4669047508d5bf6a6852%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636625386089497356&sdata=DCbKcqvS74DX1Ph88YN6IS%2FPIKENA0NEe7JaoACMuMs%3D&reserved=0>:

+ {

+ Task.Run(async () => + { + try + { + var set = await GetThumbnailSetAsync(); + if (set != null) + { + _thumbnail = set.Small ?? set.Medium ?? set.Large; + } + } + catch (Exception) + { + _thumbnail = string.Empty; + } + }).Wait(); 👍 Once you remove this property @GraniteStateHacker<https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FGraniteStateHacker&data=02%7C01%7C%7C527dc72f71cf4669047508d5bf6a6852%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636625386089497356&sdata=e5OJvLcUCM7isKe%2F842ACPhvcvTqUTSlYiSVVmX%2FeOo%3D&reserved=0> , I think we should be good to go. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2FWindowsCommunityToolkit%2Fpull%2F2035%23discussion_r189731762&data=02%7C01%7C%7C527dc72f71cf4669047508d5bf6a6852%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636625386089497356&sdata=DCbKcqvS74DX1Ph88YN6IS%2FPIKENA0NEe7JaoACMuMs%3D&reserved=0>, or mute the thread<https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAFwy4rgArucut62yoLHPyZoW07BAv81-ks5t0z_vgaJpZM4Tr68s&data=02%7C01%7C%7C527dc72f71cf4669047508d5bf6a6852%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636625386089497356&sdata=CUrhXjDY7fUyLReBqmBZyzaeZSTsKuu7jJz9FPNdHuE%3D&reserved=0>.

GraniteStateHacker · 2018-05-22T14:35:35Z

This should be committed and pushed to the dev repo Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for Windows 10

________________________________ From: Nikola Metulev <notifications@github.com> Sent: Monday, May 21, 2018 6:30:07 PM To: Microsoft/WindowsCommunityToolkit Cc: Jim Wilcox (The Granite State Hacker); Mention Subject: Re: [Microsoft/WindowsCommunityToolkit] Graph authentication improvements (#2035) @nmetulev commented on this pull request.

________________________________ In Microsoft.Toolkit.Services/Services/OneDrive/OneDriveStorageFile.cs<https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2FWindowsCommunityToolkit%2Fpull%2F2035%23discussion_r189731762&data=02%7C01%7C%7C527dc72f71cf4669047508d5bf6a6852%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636625386089497356&sdata=DCbKcqvS74DX1Ph88YN6IS%2FPIKENA0NEe7JaoACMuMs%3D&reserved=0>:

+ {

+ Task.Run(async () => + { + try + { + var set = await GetThumbnailSetAsync(); + if (set != null) + { + _thumbnail = set.Small ?? set.Medium ?? set.Large; + } + } + catch (Exception) + { + _thumbnail = string.Empty; + } + }).Wait(); 👍 Once you remove this property @GraniteStateHacker<https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FGraniteStateHacker&data=02%7C01%7C%7C527dc72f71cf4669047508d5bf6a6852%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636625386089497356&sdata=e5OJvLcUCM7isKe%2F842ACPhvcvTqUTSlYiSVVmX%2FeOo%3D&reserved=0> , I think we should be good to go. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2FWindowsCommunityToolkit%2Fpull%2F2035%23discussion_r189731762&data=02%7C01%7C%7C527dc72f71cf4669047508d5bf6a6852%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636625386089497356&sdata=DCbKcqvS74DX1Ph88YN6IS%2FPIKENA0NEe7JaoACMuMs%3D&reserved=0>, or mute the thread<https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAFwy4rgArucut62yoLHPyZoW07BAv81-ks5t0z_vgaJpZM4Tr68s&data=02%7C01%7C%7C527dc72f71cf4669047508d5bf6a6852%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636625386089497356&sdata=CUrhXjDY7fUyLReBqmBZyzaeZSTsKuu7jJz9FPNdHuE%3D&reserved=0>.

nmetulev · 2018-05-22T15:44:15Z

Microsoft.Toolkit.Services/Services/OneDrive/OneDriveStorageItem.cs

@@ -23,98 +26,89 @@ namespace Microsoft.Toolkit.Services.OneDrive
    /// <summary>
    /// GraphOneDriveStorageItem class.
    /// </summary>
-    public class OneDriveStorageItem
+    public class OneDriveStorageItem : INotifyPropertyChanged


It feels weird extending INotifyPropertyChanged for just one property. This is a model level class so I don't think we should be extending the interface at all. How do you feel about removing the Thumbnail property and adding a return value to the GetThumbnailAsync method so the developer can create their own ViewModel?

I wrestled with this, myself, but decided that we're already binding to the class in the UI, now... I felt my choices were to either make the class support the interface, or build out a new viewmodel. The latter felt like overkill, too, but I really wanted to allow the user to continue to bind to the property, as we're already exemplifying, and achieve the expected result. Again, not gonna fall on my sword over it, but that was my rationale... if you still feel it needs to go, lemme know and I'll get it out.

I don't like this, but I still think is the best solution. Only thing I would change is that now you don't need the Method... Lazy loading (fire and forget, so no .Wait()) would be perfect as it would update the UI when needed.

Let me check in that tweak just to see what it looks like, and to make sure I understand you. (will pull it back out if I misunderstood).

You just need to handle multiple requests (don't do them), and make sure you are on the UI thread when you set the variable back.

ok... had to tweak a condition logic bug, but that makes it behave relatively nicely.

The project is not building. Remember that the Services project is using multi-targeting. This means that you can't use the namespace 'Windows' on it. Don't dispatch the call to the UI Thread if you are not on Windows 10. Wrap everything that uses that namespace on a #if WINRT. Also, never use async void. Change that method to "async Task" (still don't await or .Wait() on it, so it still is a fire and forget method) and return the newValue variable. This makes it possible to await that operation if you are not binding to pre property, but you want it's value.

Gotta admit, the thought that any part of this might be targeting anything other than Windows 10 never crossed my mind. Feeling a bit outta touch... sorry.

So this is a good reason to go the route of the separate viewmodel. I'll refactor.

…and found a reasonable place to lazy load it from the calling side.

…acker/UWPCommunityToolkit into GraniteStateHacker

azchohfi · 2018-05-23T17:26:30Z

Microsoft.Toolkit.Services/Services/OneDrive/OneDriveStorageFile.cs

@@ -83,5 +93,30 @@ public OneDriveStorageFile(IBaseClient graphProvider, IBaseRequestBuilder reques
            var renameItem = await base.RenameAsync(desiredName, cancellationToken);
            return InitializeOneDriveStorageFile(renameItem.OneDriveItem);
        }
+
+        /// <summary>
+        /// gets the smallest available thumbnail url as string for the OneDrive file item, asyncrounously, and applies it to the Thumbnail property.


Upper case. I would rename this method. It's a "Get" method that does not return anything(aside from the Task). Either make it return a Task < string > (and then do both, update the property and return the value) or change it to be a Task UpdateThumbnailPropertyAsync();

That's the last comment. I promise.

LOL Gotta say, thumbnails seem wildly disproportionately difficult for the value it feels like they provide.

…acker/UWPCommunityToolkit into GraniteStateHacker

* Updates logo for graph authentication service

1e43af5

* Makes v2 MSAL default authentication provider * Updates Microsoft.Graph to package 1.8.1 * Updates Microsoft.Identity.Client to 1.1.2-preview0008

Implements the OneDrive Storage item tooltip with a thumbnail and for…

bae3521

…matted file size.

nmetulev added the services 👷‍♂️ label Apr 29, 2018

nmetulev added this to the 3.0 milestone Apr 29, 2018

GraniteStateHacker and others added 3 commits April 29, 2018 22:59

Update OneDrive.md

3c9eb02

We no longer ask the user for an MSA/O365 account provider.

Update MicrosoftGraph.md

4c5dc03

adds section for testing in Sample app.

Merge branch 'master' into GraniteStateHacker

3a975ee

michael-hawker reviewed May 4, 2018

View reviewed changes

michael-hawker reviewed May 6, 2018

View reviewed changes

michael-hawker mentioned this pull request May 10, 2018

Added graph controls #2055

Merged

7 tasks

Making sure doc changes get included for Graph & OneDrive services.

0da4a4d

nmetulev approved these changes May 17, 2018

View reviewed changes

Merge branch 'master' into GraniteStateHacker

571b937

azchohfi reviewed May 17, 2018

View reviewed changes

nmetulev and others added 2 commits May 18, 2018 11:30

Merge branch 'master' into GraniteStateHacker

41c4db2

refactored OneDriveStorageFile get thumbnail info to not be triggered…

97c0fbb

… by the bound property.

Cleaned up GetThumbnailAsync, letting exceptions fly to the caller,

a49c2de

nmetulev reviewed May 22, 2018

View reviewed changes

Jim Wilcox and others added 9 commits May 22, 2018 13:39

lazy loading thumbnail info async on get_Thumbnail();

56649eb

Maing sure the property changed event fires on the UI thread.

66de2f2

Fixed logic bug in SetValue

94ddb8f

Bail out sooner if PropertyChanged has no subscribers.

d2ba8c2

Merge branch 'master' into GraniteStateHacker

6e9337b

Merge branch 'master' into GraniteStateHacker

d982821

Pulled thumbnail management as a service out of OneDriveStorageFile, …

de488a2

…and found a reasonable place to lazy load it from the calling side.

Merge branch 'GraniteStateHacker' of https://github.com/GraniteStateH…

451d316

…acker/UWPCommunityToolkit into GraniteStateHacker

Merge branch 'master' into GraniteStateHacker

933723b

nmetulev approved these changes May 23, 2018

View reviewed changes

Merge branch 'master' into GraniteStateHacker

bf1cec5

azchohfi requested changes May 23, 2018

View reviewed changes

Jim Wilcox added 2 commits May 23, 2018 13:46

Updates semantics of a method.

201161f

Merge branch 'GraniteStateHacker' of https://github.com/GraniteStateH…

33e5e0c

…acker/UWPCommunityToolkit into GraniteStateHacker

azchohfi approved these changes May 23, 2018

View reviewed changes

azchohfi merged commit 4381c30 into CommunityToolkit:master May 23, 2018

GraniteStateHacker deleted the GraniteStateHacker branch May 23, 2018 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graph authentication improvements #2035

Graph authentication improvements #2035

GraniteStateHacker commented Apr 29, 2018 •

edited

Loading

dnfclas commented Apr 29, 2018 •

edited

Loading

michael-hawker May 4, 2018

GraniteStateHacker May 4, 2018 •

edited

Loading

michael-hawker May 6, 2018

michael-hawker May 10, 2018

nmetulev May 18, 2018

azchohfi May 17, 2018

nmetulev May 17, 2018

azchohfi May 18, 2018

nmetulev May 18, 2018

GraniteStateHacker May 18, 2018 •

edited

Loading

azchohfi May 18, 2018 •

edited

Loading

GraniteStateHacker May 18, 2018

nmetulev May 21, 2018

GraniteStateHacker commented May 22, 2018 via email

GraniteStateHacker commented May 22, 2018 via email

nmetulev May 22, 2018

GraniteStateHacker May 22, 2018 •

edited

Loading

azchohfi May 22, 2018

GraniteStateHacker May 22, 2018

azchohfi May 22, 2018

GraniteStateHacker May 22, 2018

GraniteStateHacker May 22, 2018

azchohfi May 22, 2018

GraniteStateHacker May 23, 2018

GraniteStateHacker May 23, 2018

azchohfi May 23, 2018 •

edited

Loading

azchohfi May 23, 2018

GraniteStateHacker May 23, 2018

Graph authentication improvements #2035

Graph authentication improvements #2035

Conversation

GraniteStateHacker commented Apr 29, 2018 • edited Loading

PR Type

dnfclas commented Apr 29, 2018 • edited Loading

Choose a reason for hiding this comment

GraniteStateHacker May 4, 2018 • edited Loading

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

GraniteStateHacker May 18, 2018 • edited Loading

Choose a reason for hiding this comment

azchohfi May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GraniteStateHacker commented May 22, 2018 via email

GraniteStateHacker commented May 22, 2018 via email

Choose a reason for hiding this comment

GraniteStateHacker May 22, 2018 • edited Loading

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

Choose a reason for hiding this comment

azchohfi May 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GraniteStateHacker commented Apr 29, 2018 •

edited

Loading

dnfclas commented Apr 29, 2018 •

edited

Loading

GraniteStateHacker May 4, 2018 •

edited

Loading

GraniteStateHacker May 18, 2018 •

edited

Loading

azchohfi May 18, 2018 •

edited

Loading

GraniteStateHacker May 22, 2018 •

edited

Loading

azchohfi May 23, 2018 •

edited

Loading