Implement Add Repositories UI in the File Explorer Dev Home Page #3488

ssparach · 2024-07-23T23:03:11Z

Summary of the pull request

This PR removes the temporary UI and implements the design based UI for adding and removing repositories (for source control integration) in the File Explorer Settings page.

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

Built msix release pckg
Validated UI functionality

PR checklist

Closes #xxx
Tests added/passed
Documentation updated

tools/Customization/DevHome.FileExplorerSourceControlIntegration/Services/RepositoryTracking.cs

tools/Customization/DevHome.Customization/ViewModels/FileExplorerViewModel.cs

tools/Customization/DevHome.Customization/Views/AddRepositoriesView.xaml

tools/Customization/DevHome.Customization/Strings/en-us/Resources.resw

tools/Customization/DevHome.Customization/ViewModels/FileExplorerViewModel.cs

tools/Customization/DevHome.Customization/Views/AddRepositoriesView.xaml

tools/Customization/DevHome.Customization/Views/AddRepositoriesView.xaml.cs

tools/Customization/DevHome.Customization/Views/FileExplorerPage.xaml

tools/Customization/DevHome.Customization/Views/AddRepositoriesView.xaml

DefaultRyan · 2024-07-30T20:48:48Z

tools/Customization/DevHome.Customization/Views/AddRepositoriesView.xaml

+                                <DropDownButton.Flyout>
+                                    <MenuFlyout 
+                                        Placement="Bottom">
+                                        <!-- WinUI does not currently allow populating MenuFlyoutItems dynamically within a DataTemplate. -->


Please file an issue/bug to track this. We only have the one provider at the moment, so this shouldn't block some testing/selfhosting. But let's not forget to come back to this and fix it.

Adding this as a potential fix. Using an MSBuild task to iterate through all the extensions and populate XAML before it is built. It would be difficult though. Just an idea.

This reverts commit f30055f.

**Summary of the pull request** This PR contains implementation changes for the File Explorer Source Control Integration experimental feature. This feature will allow File Explorer to obtain property information from source control technologies for display (image attached below): **Detailed description of the pull request / Additional comments** This PR contains the following changes: - Reference official Dev Home SDK version (that defines APIs for File Explorer Source Control Integration) - Declare FileExplorerSourceControlIntegration as an experimental feature - The FileExplorerSourceControlIntegration project which creates a COM Server used to communicate information with File Explorer - The FileExplorerGitIntegration project which allows Dev Home to come with an inbox extension that understands git - Basic Unit Tests **Validation steps performed** SDK and MSIX local builds Tested Git Integration File Explorer behavior inside VM Related work items: #48431506

* declare appextension for git, find all source control extensions, UI changes, SDK changes, get extension information to use for mapping * changes to map extension to registered root paths, add validation to git implementation * changes after testing * use serilog in validation code * reorder using * use published SDK version * address PR feedback * address PR feedback * address PR feedback * Minor cleanup of RepositoryTracking.cs --------- Co-authored-by: Ryan Shepherd <ryansh@microsoft.com>

extensions/GitExtension/FileExplorerGitIntegration/GitLocalRepositoryProviderFactory`1.cs

dhoehna · 2024-08-01T20:44:00Z

extensions/GitExtension/FileExplorerGitIntegration/Models/GitLocalRepository.cs

+                        latestCommit = FindLatestCommit(relativePath, repository);
+                        if (latestCommit != null)
+                        {
+                            result.Add("System.VersionControl.LastChangeMessage", latestCommit.MessageShort);


PropName can be used here instead of re-typing the string.

dhoehna · 2024-08-01T20:50:46Z

extensions/GitExtension/FileExplorerGitIntegration/Models/GitLocalRepository.cs

+            return result;
+        }
+
+        foreach (var propName in properties)


I believe the null checks are used because of iterating through the properties and this needs the latest commit to be not null.

I don't like the duplication of the check.

If LatestCommit returns null for a property, would a second call to LatestCommit return a non-null value? If this is the case the call to LatestCommit can be moved outside the foreach loop.

The null check for LatestCommit still needs to be in each case "If not null, add the property"

dhoehna · 2024-08-01T20:52:10Z

extensions/GitExtension/FileExplorerGitIntegration/Models/GitLocalRepository.cs

+        {
+            switch (propName)
+            {
+                case "System.VersionControl.LastChangeMessage":


Another way to do this is,

Put LatestCommit outside of the for loop.

For every property use result.Add(propName, latestCommit?.[PropertyName] ?? string.empty)

Before returning result remove all entries that contain string.empty.

dhoehna · 2024-08-01T20:54:17Z

extensions/GitExtension/FileExplorerGitIntegration/Models/GitLocalRepository.cs

+        var commits = repository.Commits;
+
+        // Check the most recent commit and bail if the file is not present
+        {


Why is this in its own scope?

dhoehna · 2024-08-01T20:57:38Z

extensions/GitExtension/FileExplorerGitIntegration/Models/GitLocalRepository.cs

+        // Check the most recent commit and bail if the file is not present
+        {
+            var firstCommit = commits.FirstOrDefault();
+            if (firstCommit != null && firstCommit.Tree[relativePath] == null)


I believe this can be simplified to commits.FirstOrDefault()?.Tree?[relativePath] == null

dhoehna · 2024-08-01T20:58:59Z

extensions/GitExtension/FileExplorerGitIntegration/Models/GitLocalRepository.cs

+            }
+        }
+
+        foreach (var currentCommit in commits)


NIT: commit.Select(x => x.Tree[relativePath]).Where(x +> x is not null)

dhoehna · 2024-08-01T20:59:54Z

extensions/GitExtension/FileExplorerGitIntegration/Models/GitLocalRepository.cs

+
+            var parents = currentCommit.Parents;
+            var count = parents.Count();
+            if (count == 0)


NIT: currentCommit.Parents.Count() == 0

dhoehna · 2024-08-01T21:02:33Z

extensions/GitExtension/FileExplorerGitIntegration/Models/RepositoryCache.cs

+        catch (Exception ex)
+        {
+            log.Error("RepositoryCache", "Failed to create Repository", ex);
+            if (repo != null)


NIT: repo?.Dispose()

dhoehna · 2024-08-01T21:06:50Z

extensions/GitExtension/FileExplorerGitIntegration/Program.cs

+    public static void Main([System.Runtime.InteropServices.WindowsRuntime.ReadOnlyArray] string[] args)
+    {
+        // Set up Logging
+        Environment.SetEnvironmentVariable("DEVHOME_LOGS_ROOT", Path.Join(DevHome.Common.Logging.LogFolderRoot, "FileExplorerGitIntegration"));


NIT: You can use Microsoft.Windows.System.EnvironmentManager and the EV will be removed when the app is uninstalled.

dhoehna · 2024-08-01T21:11:10Z

tools/Customization/DevHome.Customization/Models/SourceControlIntegration.cs

+                return new SourceControlValidationResult(ResultType.Failure, ErrorType.RepositoryProvderCreationFailed, null, null, null);
+            }
+
+            ILocalRepositoryProvider provider = MarshalInterface<ILocalRepositoryProvider>.FromAbi(providerPtr);


Why use PInvoke to get the ILocalRepositoryProvider? I believe DevHome has interfaces to get the providers already.

dhoehna · 2024-08-01T21:17:36Z

tools/Customization/DevHome.FileExplorerSourceControlIntegration/Services/RepositoryTracking.cs

+    {
+        lock (trackRepoLock)
+        {
+            var caseSensitiveDictionary = fileService.Read<Dictionary<string, string>>(RepoStoreOptions.RepoStoreFolderPath, RepoStoreOptions.RepoStoreFileName);


old name? Can the name be changed to something more descriptive?

Considering on line 72 the case is ignored.

dhoehna · 2024-08-01T21:20:06Z

tools/Customization/DevHome.FileExplorerSourceControlIntegration/Services/RepositoryTracking.cs

+        {
+            if (!TrackedRepositories.ContainsKey(rootPath))
+            {
+                TrackedRepositories[rootPath] = extensionCLSID!;


I understand. Tracked repositories is a map from a root path to the extension that is tracking the repository. I understand now. Okay.

NIT: change the name to TrackedRepositoriesToExtension, or something similar.

That is why so many other files use pUnk to determing the extension type.

dhoehna · 2024-08-01T21:23:24Z

tools/Customization/DevHome.FileExplorerSourceControlIntegration/Services/RepositoryTracking.cs

+            if (TrackedRepositories.TryGetValue(rootPath, out var value))
+            {
+                log.Information("Source Control Provider returned for root path");
+                return TrackedRepositories[rootPath];


return value.

dhoehna · 2024-08-01T21:26:34Z

tools/Customization/DevHome.FileExplorerSourceControlIntegration/Services/RepositoryTracking.cs

@@ -0,0 +1,180 @@
+// Copyright (c) Microsoft Corporation.


NIT for the file: Make CLSID the second argument. That is what it is. All other methods need either

nothing

RootPath

RootPath and CLSID.

This keeps the arguments in a consistent order.

ssparach · 2024-08-01T21:27:34Z

@dhoehna Just a heads up. This PR contains alot of changes which have already been merged to the feature branch. Since the feature branch was rebased to latest in main, the changes are appearing here. I am currently (locally) fixing the state of the branch. As such, the PR will only contain the UI changes. However, we do plan to create a PR for review to commit to main soon. Thanks!

dhoehna · 2024-08-01T21:27:42Z

tools/Customization/DevHome.FileExplorerSourceControlIntegration/Services/RepositoryTracking.cs

+        {
+            if (TrackedRepositories.TryGetValue(rootPath, out var existingExtensionCLSID))
+            {
+                TrackedRepositories[rootPath] = extensionCLSID;


you already retrieved TrackedRepositories[rootPath] in TryGetValue. existingExtensionCLSID can be used in this scope.

NVM. This is used to check the existence of the key. In the case. can you use .ContainsKey instead?

dhoehna · 2024-08-01T21:31:39Z

tools/Customization/DevHome.FileExplorerSourceControlIntegration/Services/RepositoryTracking.cs

+                TrackedRepositories = caseSensitiveDictionary.ToDictionary(entry => entry.Key, entry => entry.Value, StringComparer.OrdinalIgnoreCase);
+            }
+
+            LastRestore = DateTime.Now;


NIT: use UTcNow. An edge case can occur if using .Now. For example, Daylight savings time occurred. Or the user moved between times zones.

dhoehna · 2024-08-01T21:32:13Z

tools/Customization/DevHome.FileExplorerSourceControlIntegration/SourceControlProvider.cs

+
+    internal ILocalRepositoryProvider GetLocalProvider(string rootPath)
+    {
+        // TODO: Iterate extensions to find the correct one for this rootPath.


Is this TODO still relevant?

dhoehna · 2024-08-01T21:35:56Z

tools/Customization/DevHome.Customization/ViewModels/FileExplorerViewModel.cs

+        {
+            TrackedRepositories.Clear();
+            var repoCollection = RepoTracker.GetAllTrackedRepositories();
+            foreach (KeyValuePair<string, string> data in repoCollection)


NIT: repoCollection.Select(x => new RepositoryInformation(x.Key, x.Value))

dhoehna · 2024-08-01T21:37:25Z

tools/Customization/DevHome.FileExplorerSourceControlIntegration/SourceControlProvider.cs

+
+    IDictionary<string, object> IExtraFolderPropertiesHandler.GetProperties(string[] propertyStrings, string rootFolderPath, string relativePath)
+    {
+        var localProvider = GetLocalProvider(rootFolderPath);


NIT: GetLocalProvider(rootFolderPath).GetRepository(rootFolderPath)

dhoehna · 2024-08-01T21:38:41Z

...Customization/DevHome.FileExplorerSourceControlIntegration/SourceControlProviderFactory`1.cs

+        }
+
+        // TODO: How to detect between WinRT/IInspectable and COM/IUnknown interfaces
+        ppvObject = MarshalInspectable<T>.CreateMarshaler2(_createSourceControlProvider(), riid, true).Detach();


Is this TODO still relevant?

ssparach requested review from DefaultRyan, krschau, jamespik and ethanfangg July 23, 2024 23:13

ssparach marked this pull request as ready for review July 23, 2024 23:16

ssparach removed the request for review from ethanfangg July 23, 2024 23:16

dhoehna reviewed Jul 24, 2024

View reviewed changes

tools/Customization/DevHome.FileExplorerSourceControlIntegration/Services/RepositoryTracking.cs Outdated Show resolved Hide resolved

dhoehna reviewed Jul 24, 2024

View reviewed changes

tools/Customization/DevHome.FileExplorerSourceControlIntegration/Services/RepositoryTracking.cs Show resolved Hide resolved

dhoehna reviewed Jul 24, 2024

View reviewed changes

tools/Customization/DevHome.Customization/ViewModels/FileExplorerViewModel.cs Outdated Show resolved Hide resolved

krschau reviewed Jul 24, 2024

View reviewed changes

tools/Customization/DevHome.Customization/Views/AddRepositoriesView.xaml Outdated Show resolved Hide resolved

krschau reviewed Jul 26, 2024

View reviewed changes

tools/Customization/DevHome.Customization/Views/AddRepositoriesView.xaml Outdated Show resolved Hide resolved

DefaultRyan reviewed Jul 30, 2024

View reviewed changes

DefaultRyan approved these changes Jul 30, 2024

View reviewed changes

ssparach force-pushed the feature/fileexplorer-sourcecontrol-integration branch 3 times, most recently from 16b9823 to e7bc4e1 Compare July 31, 2024 23:27

ssparach and others added 13 commits August 1, 2024 11:39

reset branch to 548d868

0838dba

Revert "reset branch to 548d868"

0753b47

This reverts commit f30055f.

save state

f731106

save state

bda31f0

save state

e432815

save state

a0bfb4f

save state

170c7cf

implement button actions

c2f5a8d

changes

504df3a

address PR feedback

f2f95de

address PR feedback

41c9650

dhoehna reviewed Aug 1, 2024

View reviewed changes

extensions/GitExtension/FileExplorerGitIntegration/GitLocalRepositoryProviderFactory`1.cs Outdated Show resolved Hide resolved

dhoehna reviewed Aug 1, 2024

View reviewed changes

rebased user branch, made fixes, validated functionality on VM

3ae6b69

ssparach force-pushed the user/ssparach/AddRepositoriesUIForFEVersionControlSettings branch from 2ea72d7 to 3ae6b69 Compare August 1, 2024 22:56

ssparach added 2 commits August 2, 2024 09:53

cleanup UI changes, tested on VM

c340366

fix proj references to match dev home feed and unblock PR pipeline

64b7609

ssparach merged commit 136ea6e into feature/fileexplorer-sourcecontrol-integration Aug 2, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Add Repositories UI in the File Explorer Dev Home Page #3488

Implement Add Repositories UI in the File Explorer Dev Home Page #3488

ssparach commented Jul 23, 2024 •

edited

Loading

DefaultRyan Jul 30, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024 •

edited

Loading

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024 •

edited

Loading

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

ssparach commented Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024 •

edited

Loading

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

dhoehna Aug 1, 2024

Implement Add Repositories UI in the File Explorer Dev Home Page #3488

Implement Add Repositories UI in the File Explorer Dev Home Page #3488

Conversation

ssparach commented Jul 23, 2024 • edited Loading

Summary of the pull request

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

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

dhoehna Aug 1, 2024 • 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

dhoehna Aug 1, 2024 • 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

ssparach commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoehna Aug 1, 2024 • 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

ssparach commented Jul 23, 2024 •

edited

Loading

dhoehna Aug 1, 2024 •

edited

Loading

dhoehna Aug 1, 2024 •

edited

Loading

dhoehna Aug 1, 2024 •

edited

Loading