Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Commit dd0c5ea

Browse files
jcansdalejcansdale
authored
Merge pull request #930 from github/fixes/801-open-pr-file
Only enable Open file when branch checked out. Also, live editable diffs!
2 parents 2c8a718 + 7563620 commit dd0c5ea

File tree

8 files changed

+73
-47
lines changed

8 files changed

+73
-47
lines changed

‎src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ public PullRequestDetailViewModelDesigner()
7070
public IPullRequestModel Model { get; }
7171
public string SourceBranchDisplayName { get; set; }
7272
public string TargetBranchDisplayName { get; set; }
73+
public bool IsCheckedOut { get; }
7374
public bool IsFromFork { get; }
7475
public string Body { get; }
7576
public IReactiveList<IPullRequestChangeNode> ChangedFilesTree { get; }
@@ -84,12 +85,12 @@ public PullRequestDetailViewModelDesigner()
8485
public ReactiveCommand<object> OpenFile { get; }
8586
public ReactiveCommand<object> DiffFile { get; }
8687

87-
public Task<string> ExtractFile(IPullRequestFileNode file)
88+
public Task<Tuple<string, string>> ExtractDiffFiles(IPullRequestFileNode file)
8889
{
8990
return null;
9091
}
9192

92-
public Task<Tuple<string, string>> ExtractDiffFiles(IPullRequestFileNode file)
93+
public string GetLocalFilePath(IPullRequestFileNode file)
9394
{
9495
return null;
9596
}

‎src/GitHub.App/Services/PullRequestService.cs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,8 @@ public IObservable<Tuple<string, string>> ExtractDiffFiles(
287287
IModelService modelService,
288288
IPullRequestModel pullRequest,
289289
string fileName,
290-
string fileSha)
290+
string fileSha,
291+
bool isPullRequestBranchCheckedOut)
291292
{
292293
return Observable.Defer(async () =>
293294
{
@@ -300,7 +301,9 @@ public IObservable<Tuple<string, string>> ExtractDiffFiles(
300301

301302
// The right file - if it comes from a fork - may not be fetched so fall back to
302303
// getting the file contents from the model service.
303-
var right = await GetFileFromRepositoryOrApi(repository, repo, modelService, pullRequest.Head.Sha, fileName, fileSha);
304+
var right = isPullRequestBranchCheckedOut ?
305+
Path.Combine(repository.LocalPath, fileName) :
306+
await GetFileFromRepositoryOrApi(repository, repo, modelService, pullRequest.Head.Sha, fileName, fileSha);
304307

305308
if (left == null)
306309
{

‎src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ public class PullRequestDetailViewModel : BaseViewModel, IPullRequestDetailViewM
3939
IPullRequestCheckoutState checkoutState;
4040
IPullRequestUpdateState updateState;
4141
string operationError;
42+
bool isCheckedOut;
4243
bool isFromFork;
4344
bool isInCheckout;
4445

@@ -103,7 +104,7 @@ public PullRequestDetailViewModel(
103104
SubscribeOperationError(Push);
104105

105106
OpenOnGitHub = ReactiveCommand.Create();
106-
OpenFile = ReactiveCommand.Create();
107+
OpenFile = ReactiveCommand.Create(this.WhenAnyValue(x => x.IsCheckedOut));
107108
DiffFile = ReactiveCommand.Create();
108109
}
109110

@@ -145,6 +146,15 @@ public string TargetBranchDisplayName
145146
private set { this.RaiseAndSetIfChanged(ref targetBranchDisplayName, value); }
146147
}
147148

149+
/// <summary>
150+
/// Gets a value indicating whether the pull request branch is checked out.
151+
/// </summary>
152+
public bool IsCheckedOut
153+
{
154+
get { return isCheckedOut; }
155+
private set { this.RaiseAndSetIfChanged(ref isCheckedOut, value); }
156+
}
157+
148158
/// <summary>
149159
/// Gets a value indicating whether the pull request comes from a fork.
150160
/// </summary>
@@ -269,9 +279,10 @@ public async Task Load(IPullRequestModel pullRequest)
269279
}
270280

271281
var localBranches = await pullRequestsService.GetLocalBranches(repository, pullRequest).ToList();
272-
var isCheckedOut = localBranches.Contains(repository.CurrentBranch);
273282

274-
if (isCheckedOut)
283+
IsCheckedOut = localBranches.Contains(repository.CurrentBranch);
284+
285+
if (IsCheckedOut)
275286
{
276287
var divergence = await pullRequestsService.CalculateHistoryDivergence(repository, Model.Number);
277288
var pullEnabled = divergence.BehindBy > 0;
@@ -340,25 +351,24 @@ public async Task Load(IPullRequestModel pullRequest)
340351
}
341352

342353
/// <summary>
343-
/// Gets the specified file as it appears in the pull request.
354+
/// Gets the before and after files needed for viewing a diff.
344355
/// </summary>
345-
/// <param name="file">The file or directory node.</param>
346-
/// <returns>The path to the extracted file.</returns>
347-
public Task<string> ExtractFile(IPullRequestFileNode file)
356+
/// <param name="file">The changed file.</param>
357+
/// <returns>A tuple containing the full path to the before and after files.</returns>
358+
public Task<Tuple<string, string>> ExtractDiffFiles(IPullRequestFileNode file)
348359
{
349360
var path = Path.Combine(file.DirectoryPath, file.FileName);
350-
return pullRequestsService.ExtractFile(repository, modelService, model.Head.Sha, path, file.Sha).ToTask();
361+
return pullRequestsService.ExtractDiffFiles(repository, modelService, model, path, file.Sha, IsCheckedOut).ToTask();
351362
}
352363

353364
/// <summary>
354-
/// Gets the before and after files needed for viewing a diff.
365+
/// Gets the full path to a file in the working directory.
355366
/// </summary>
356-
/// <param name="file">The changed file.</param>
357-
/// <returns>A tuple containing the full path to the before and after files.</returns>
358-
public Task<Tuple<string, string>> ExtractDiffFiles(IPullRequestFileNode file)
367+
/// <param name="file">The file.</param>
368+
/// <returns>The full path to the file in the working directory.</returns>
369+
public string GetLocalFilePath(IPullRequestFileNode file)
359370
{
360-
var path = Path.Combine(file.DirectoryPath, file.FileName);
361-
return pullRequestsService.ExtractDiffFiles(repository, modelService, model, path, file.Sha).ToTask();
371+
return Path.Combine(repository.LocalPath, file.DirectoryPath, file.FileName);
362372
}
363373

364374
void SubscribeOperationError(ReactiveCommand<Unit> command)

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,13 +120,18 @@ IObservable<string> ExtractFile(
120120
/// <param name="pullRequest">The pull request details.</param>
121121
/// <param name="fileName">The filename relative to the repository root.</param>
122122
/// <param name="fileSha">The SHA of the file in the pull request.</param>
123-
/// <returns>The filenames of the left and right files for the diff.</returns>
123+
/// <param name="isPullRequestBranchCheckedOut">
124+
/// Whether the pull request branch is currently checked out. If so the right file returned
125+
/// will be the path to the file in the working directory.
126+
/// </param>
127+
/// <returns>The paths of the left and right files for the diff.</returns>
124128
IObservable<Tuple<string, string>> ExtractDiffFiles(
125129
ILocalRepositoryModel repository,
126130
IModelService modelService,
127131
IPullRequestModel pullRequest,
128132
string fileName,
129-
string fileSha);
133+
string fileSha,
134+
bool isPullRequestBranchCheckedOut);
130135

131136
/// <summary>
132137
/// Remotes all unused remotes that were created by GitHub for Visual Studio to track PRs

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,11 @@ public interface IPullRequestDetailViewModel : IViewModel, IHasBusy
7979
/// </summary>
8080
string TargetBranchDisplayName { get; }
8181

82+
/// <summary>
83+
/// Gets a value indicating whether the pull request branch is checked out.
84+
/// </summary>
85+
bool IsCheckedOut { get; }
86+
8287
/// <summary>
8388
/// Gets a value indicating whether the pull request comes from a fork.
8489
/// </summary>
@@ -139,18 +144,18 @@ public interface IPullRequestDetailViewModel : IViewModel, IHasBusy
139144
/// </summary>
140145
ReactiveCommand<object> DiffFile { get; }
141146

142-
/// <summary>
143-
/// Gets the specified file as it appears in the pull request.
144-
/// </summary>
145-
/// <param name="file">The file or directory node.</param>
146-
/// <returns>The path to the extracted file.</returns>
147-
Task<string> ExtractFile(IPullRequestFileNode file);
148-
149147
/// <summary>
150148
/// Gets the before and after files needed for viewing a diff.
151149
/// </summary>
152150
/// <param name="file">The changed file.</param>
153151
/// <returns>A tuple containing the full path to the before and after files.</returns>
154152
Task<Tuple<string, string>> ExtractDiffFiles(IPullRequestFileNode file);
153+
154+
/// <summary>
155+
/// Gets the full path to a file in the working directory.
156+
/// </summary>
157+
/// <param name="file">The file.</param>
158+
/// <returns>The full path to the file in the working directory.</returns>
159+
string GetLocalFilePath(IPullRequestFileNode file);
155160
}
156161
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,8 +286,8 @@
286286
<!-- When invoked from the TreeView and the ListView we programmatically bind the
287287
DataContext and update the CommandParameter to the changed file -->
288288
<ContextMenu x:Key="FileContextMenu">
289-
<MenuItem Header="{x:Static prop:Resources.OpenFile}" Command="{Binding OpenFile}"/>
290289
<MenuItem Header="{x:Static prop:Resources.CompareFile}" Command="{Binding DiffFile}"/>
290+
<MenuItem Header="{x:Static prop:Resources.OpenFile}" Command="{Binding OpenFile}"/>
291291
</ContextMenu>
292292
</Grid.Resources>
293293

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

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public PullRequestDetailView()
4141
this.WhenActivated(d =>
4242
{
4343
d(ViewModel.OpenOnGitHub.Subscribe(_ => DoOpenOnGitHub()));
44-
d(ViewModel.OpenFile.Subscribe(x => DoOpenFile((IPullRequestFileNode)x).Forget()));
44+
d(ViewModel.OpenFile.Subscribe(x => DoOpenFile((IPullRequestFileNode)x)));
4545
d(ViewModel.DiffFile.Subscribe(x => DoDiffFile((IPullRequestFileNode)x).Forget()));
4646
});
4747
}
@@ -65,15 +65,12 @@ void DoOpenOnGitHub()
6565
browser.OpenUrl(url);
6666
}
6767

68-
async Task DoOpenFile(IPullRequestFileNode file)
68+
void DoOpenFile(IPullRequestFileNode file)
6969
{
7070
try
7171
{
72-
var fileName = await ViewModel.ExtractFile(file);
73-
var window = Services.Dte.ItemOperations.OpenFile(fileName);
74-
75-
// If the file we extracted isn't the current file on disk, make the window read-only.
76-
window.Document.ReadOnly = fileName != file.DirectoryPath;
72+
var fileName = ViewModel.GetLocalFilePath(file);
73+
Services.Dte.ItemOperations.OpenFile(fileName);
7774
}
7875
catch (Exception e)
7976
{
@@ -90,19 +87,24 @@ async Task DoDiffFile(IPullRequestFileNode file)
9087
var rightLabel = $"{file.FileName};PR {ViewModel.Model.Number}";
9188
var caption = $"Diff - {file.FileName}";
9289
var tooltip = $"{leftLabel}\nvs.\n{rightLabel}";
90+
var options = __VSDIFFSERVICEOPTIONS.VSDIFFOPT_DetectBinaryFiles |
91+
__VSDIFFSERVICEOPTIONS.VSDIFFOPT_LeftFileIsTemporary;
92+
93+
if (!ViewModel.IsCheckedOut)
94+
{
95+
options |= __VSDIFFSERVICEOPTIONS.VSDIFFOPT_RightFileIsTemporary;
96+
}
9397

9498
Services.DifferenceService.OpenComparisonWindow2(
95-
fileNames.Item1,
96-
fileNames.Item2,
97-
caption,
98-
tooltip,
99-
leftLabel,
100-
rightLabel,
101-
string.Empty,
102-
string.Empty,
103-
(int)(__VSDIFFSERVICEOPTIONS.VSDIFFOPT_DetectBinaryFiles |
104-
__VSDIFFSERVICEOPTIONS.VSDIFFOPT_LeftFileIsTemporary |
105-
__VSDIFFSERVICEOPTIONS.VSDIFFOPT_RightFileIsTemporary));
99+
fileNames.Item1,
100+
fileNames.Item2,
101+
caption,
102+
tooltip,
103+
leftLabel,
104+
rightLabel,
105+
string.Empty,
106+
string.Empty,
107+
(uint)options);
106108
}
107109
catch (Exception e)
108110
{

‎submodules/GitHubVSAutomationIDs‎

0 commit comments

Comments
 (0)