Merge pull request #930 from github/fixes/801-open-pr-file

Only enable Open file when branch checked out.

Also, live editable diffs!

src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs

@@ -70,6 +70,7 @@ This requires that errors be propagated from the viewmodel to the view and from
         public IPullRequestModel Model { get; }
         public string SourceBranchDisplayName { get; set; }
         public string TargetBranchDisplayName { get; set; }
+        public bool IsCheckedOut { get; }
         public bool IsFromFork { get; }
         public string Body { get; }
         public IReactiveList<IPullRequestChangeNode> ChangedFilesTree { get; }
@@ -84,12 +85,12 @@ This requires that errors be propagated from the viewmodel to the view and from
         public ReactiveCommand<object> OpenFile { get; }
         public ReactiveCommand<object> DiffFile { get; }
 
-        public Task<string> ExtractFile(IPullRequestFileNode file)
+        public Task<Tuple<string, string>> ExtractDiffFiles(IPullRequestFileNode file)
         {
             return null;
         }
 
-        public Task<Tuple<string, string>> ExtractDiffFiles(IPullRequestFileNode file)
+        public string GetLocalFilePath(IPullRequestFileNode file)
         {
             return null;
         }

src/GitHub.App/Services/PullRequestService.cs

@@ -287,7 +287,8 @@ namespace GitHub.Services
             IModelService modelService,
             IPullRequestModel pullRequest,
             string fileName,
-            string fileSha)
+            string fileSha,
+            bool isPullRequestBranchCheckedOut)
         {
             return Observable.Defer(async () =>
             {
@@ -300,7 +301,9 @@ namespace GitHub.Services
 
                 // The right file - if it comes from a fork - may not be fetched so fall back to
                 // getting the file contents from the model service.
-                var right = await GetFileFromRepositoryOrApi(repository, repo, modelService, pullRequest.Head.Sha, fileName, fileSha);
+                var right = isPullRequestBranchCheckedOut ?
+                    Path.Combine(repository.LocalPath, fileName) :
+                    await GetFileFromRepositoryOrApi(repository, repo, modelService, pullRequest.Head.Sha, fileName, fileSha);
 
                 if (left == null)
                 {

src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs

@@ -39,6 +39,7 @@ namespace GitHub.ViewModels
         IPullRequestCheckoutState checkoutState;
         IPullRequestUpdateState updateState;
         string operationError;
+        bool isCheckedOut;
         bool isFromFork;
         bool isInCheckout;
 
@@ -103,7 +104,7 @@ namespace GitHub.ViewModels
             SubscribeOperationError(Push);
 
             OpenOnGitHub = ReactiveCommand.Create();
-            OpenFile = ReactiveCommand.Create();
+            OpenFile = ReactiveCommand.Create(this.WhenAnyValue(x => x.IsCheckedOut));
             DiffFile = ReactiveCommand.Create();
         }
 
@@ -145,6 +146,15 @@ namespace GitHub.ViewModels
             private set { this.RaiseAndSetIfChanged(ref targetBranchDisplayName, value); }
         }
 
+        /// <summary>
+        /// Gets a value indicating whether the pull request branch is checked out.
+        /// </summary>
+        public bool IsCheckedOut
+        {
+            get { return isCheckedOut; }
+            private set { this.RaiseAndSetIfChanged(ref isCheckedOut, value); }
+        }
+
         /// <summary>
         /// Gets a value indicating whether the pull request comes from a fork.
         /// </summary>
@@ -269,9 +279,10 @@ namespace GitHub.ViewModels
             }
 
             var localBranches = await pullRequestsService.GetLocalBranches(repository, pullRequest).ToList();
-            var isCheckedOut = localBranches.Contains(repository.CurrentBranch);
 
-            if (isCheckedOut)
+            IsCheckedOut = localBranches.Contains(repository.CurrentBranch);
+
+            if (IsCheckedOut)
             {
                 var divergence = await pullRequestsService.CalculateHistoryDivergence(repository, Model.Number);
                 var pullEnabled = divergence.BehindBy > 0;
@@ -339,17 +350,6 @@ namespace GitHub.ViewModels
             }
         }
 
-        /// <summary>
-        /// Gets the specified file as it appears in the pull request.
-        /// </summary>
-        /// <param name="file">The file or directory node.</param>
-        /// <returns>The path to the extracted file.</returns>
-        public Task<string> ExtractFile(IPullRequestFileNode file)
-        {
-            var path = Path.Combine(file.DirectoryPath, file.FileName);
-            return pullRequestsService.ExtractFile(repository, modelService, model.Head.Sha, path, file.Sha).ToTask();
-        }
-
         /// <summary>
         /// Gets the before and after files needed for viewing a diff.
         /// </summary>
@@ -358,7 +358,17 @@ namespace GitHub.ViewModels
         public Task<Tuple<string, string>> ExtractDiffFiles(IPullRequestFileNode file)
         {
             var path = Path.Combine(file.DirectoryPath, file.FileName);
-            return pullRequestsService.ExtractDiffFiles(repository, modelService, model, path, file.Sha).ToTask();
+            return pullRequestsService.ExtractDiffFiles(repository, modelService, model, path, file.Sha, IsCheckedOut).ToTask();
+        }
+
+        /// <summary>
+        /// Gets the full path to a file in the working directory.
+        /// </summary>
+        /// <param name="file">The file.</param>
+        /// <returns>The full path to the file in the working directory.</returns>
+        public string GetLocalFilePath(IPullRequestFileNode file)
+        {
+            return Path.Combine(repository.LocalPath, file.DirectoryPath, file.FileName);
         }
 
         void SubscribeOperationError(ReactiveCommand<Unit> command)

src/GitHub.Exports.Reactive/Services/IPullRequestService.cs

@@ -120,13 +120,18 @@ namespace GitHub.Services
         /// <param name="pullRequest">The pull request details.</param>
         /// <param name="fileName">The filename relative to the repository root.</param>
         /// <param name="fileSha">The SHA of the file in the pull request.</param>
-        /// <returns>The filenames of the left and right files for the diff.</returns>
+        /// <param name="isPullRequestBranchCheckedOut">
+        /// Whether the pull request branch is currently checked out. If so the right file returned
+        /// will be the path to the file in the working directory.
+        /// </param>
+        /// <returns>The paths of the left and right files for the diff.</returns>
         IObservable<Tuple<string, string>> ExtractDiffFiles(
             ILocalRepositoryModel repository,
             IModelService modelService,
             IPullRequestModel pullRequest,
             string fileName,
-            string fileSha);
+            string fileSha,
+            bool isPullRequestBranchCheckedOut);
 
         /// <summary>
         /// Remotes all unused remotes that were created by GitHub for Visual Studio to track PRs

src/GitHub.Exports.Reactive/ViewModels/IPullRequestDetailViewModel.cs

@@ -79,6 +79,11 @@ namespace GitHub.ViewModels
         /// </summary>
         string TargetBranchDisplayName { get; }
 
+        /// <summary>
+        /// Gets a value indicating whether the pull request branch is checked out.
+        /// </summary>
+        bool IsCheckedOut { get; }
+
         /// <summary>
         /// Gets a value indicating whether the pull request comes from a fork.
         /// </summary>
@@ -139,18 +144,18 @@ namespace GitHub.ViewModels
         /// </summary>
         ReactiveCommand<object> DiffFile { get; }
 
-        /// <summary>
-        /// Gets the specified file as it appears in the pull request.
-        /// </summary>
-        /// <param name="file">The file or directory node.</param>
-        /// <returns>The path to the extracted file.</returns>
-        Task<string> ExtractFile(IPullRequestFileNode file);
-
         /// <summary>
         /// Gets the before and after files needed for viewing a diff.
         /// </summary>
         /// <param name="file">The changed file.</param>
         /// <returns>A tuple containing the full path to the before and after files.</returns>
         Task<Tuple<string, string>> ExtractDiffFiles(IPullRequestFileNode file);
+
+        /// <summary>
+        /// Gets the full path to a file in the working directory.
+        /// </summary>
+        /// <param name="file">The file.</param>
+        /// <returns>The full path to the file in the working directory.</returns>
+        string GetLocalFilePath(IPullRequestFileNode file);
     }
 }

src/GitHub.VisualStudio/UI/Views/PullRequestDetailView.xaml

@@ -286,8 +286,8 @@
                                 <!-- When invoked from the TreeView and the ListView we programmatically bind the 
                                      DataContext and update the CommandParameter to the changed file -->
                                 <ContextMenu x:Key="FileContextMenu">
-                                    <MenuItem Header="{x:Static prop:Resources.OpenFile}" Command="{Binding OpenFile}"/>
                                     <MenuItem Header="{x:Static prop:Resources.CompareFile}" Command="{Binding DiffFile}"/>
+                                    <MenuItem Header="{x:Static prop:Resources.OpenFile}" Command="{Binding OpenFile}"/>
                                 </ContextMenu>
                             </Grid.Resources>
                             

src/GitHub.VisualStudio/UI/Views/PullRequestDetailView.xaml.cs

@@ -41,7 +41,7 @@ namespace GitHub.VisualStudio.UI.Views
             this.WhenActivated(d =>
             {
                 d(ViewModel.OpenOnGitHub.Subscribe(_ => DoOpenOnGitHub()));
-                d(ViewModel.OpenFile.Subscribe(x => DoOpenFile((IPullRequestFileNode)x).Forget()));
+                d(ViewModel.OpenFile.Subscribe(x => DoOpenFile((IPullRequestFileNode)x)));
                 d(ViewModel.DiffFile.Subscribe(x => DoDiffFile((IPullRequestFileNode)x).Forget()));
             });
         }
@@ -65,15 +65,12 @@ namespace GitHub.VisualStudio.UI.Views
             browser.OpenUrl(url);
         }
 
-        async Task DoOpenFile(IPullRequestFileNode file)
+        void DoOpenFile(IPullRequestFileNode file)
         {
             try
             {
-                var fileName = await ViewModel.ExtractFile(file);
+                var fileName = ViewModel.GetLocalFilePath(file);
-                var window = Services.Dte.ItemOperations.OpenFile(fileName);
+                Services.Dte.ItemOperations.OpenFile(fileName);
-
-                // If the file we extracted isn't the current file on disk, make the window read-only.
-                window.Document.ReadOnly = fileName != file.DirectoryPath;
             }
             catch (Exception e)
             {
@@ -90,19 +87,24 @@ namespace GitHub.VisualStudio.UI.Views
                 var rightLabel = $"{file.FileName};PR {ViewModel.Model.Number}";
                 var caption = $"Diff - {file.FileName}";
                 var tooltip = $"{leftLabel}\nvs.\n{rightLabel}";
+                var options = __VSDIFFSERVICEOPTIONS.VSDIFFOPT_DetectBinaryFiles |
+                    __VSDIFFSERVICEOPTIONS.VSDIFFOPT_LeftFileIsTemporary;
+
+                if (!ViewModel.IsCheckedOut)
+                {
+                    options |= __VSDIFFSERVICEOPTIONS.VSDIFFOPT_RightFileIsTemporary;
+                }
 
                 Services.DifferenceService.OpenComparisonWindow2(
-                fileNames.Item1,
+                    fileNames.Item1,
-                fileNames.Item2,
+                    fileNames.Item2,
-                caption,
+                    caption,
-                tooltip,
+                    tooltip,
-                leftLabel,
+                    leftLabel,
-                rightLabel,
+                    rightLabel,
-                string.Empty,
+                    string.Empty,
-                string.Empty,
+                    string.Empty,
-                (int)(__VSDIFFSERVICEOPTIONS.VSDIFFOPT_DetectBinaryFiles |
+                    (uint)options);
-                    __VSDIFFSERVICEOPTIONS.VSDIFFOPT_LeftFileIsTemporary |
-                    __VSDIFFSERVICEOPTIONS.VSDIFFOPT_RightFileIsTemporary));
             }
             catch (Exception e)
             {

submodules/GitHubVSAutomationIDs

@@ -1 +1 @@
-Subproject commit e3a4c2ed78980e4a2379716062e9e4ae04c44e8a
+Subproject commit 3105a88caaef9b7059f3779843b8b2c8feb5390a