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

Commit aa6dbc1

Browse files
jcansdalejcansdale
authored
Merge pull request #1585 from github/fixes/reuse-existing-diff-tab
Reuse existing tabs when opening files from changes tree
2 parents a96fd8f + 41850a9 commit aa6dbc1

File tree

7 files changed

+174
-73
lines changed

7 files changed

+174
-73
lines changed

‎src/GitHub.App/Api/ApiClient.cs‎

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -149,30 +149,10 @@ public IObservable<LicenseMetadata> GetLicenses()
149149

150150
public HostAddress HostAddress { get; }
151151

152-
static string GetSha256Hash(string input)
153-
{
154-
Guard.ArgumentNotEmptyString(input, nameof(input));
155-
156-
try
157-
{
158-
using (var sha256 = SHA256.Create())
159-
{
160-
var bytes = Encoding.UTF8.GetBytes(input);
161-
var hash = sha256.ComputeHash(bytes);
162-
163-
return string.Join("", hash.Select(b => b.ToString("x2", CultureInfo.InvariantCulture)));
164-
}
165-
}
166-
catch (Exception e)
167-
{
168-
log.Error(e, "IMPOSSIBLE! Generating Sha256 hash caused an exception");
169-
return null;
170-
}
171-
}
172-
173152
static string GetFingerprint()
174153
{
175-
return GetSha256Hash(ProductName + ":" + GetMachineIdentifier());
154+
var fingerprint = ProductName + ":" + GetMachineIdentifier();
155+
return fingerprint.GetSha256Hash();
176156
}
177157

178158
static string GetMachineNameSafe()

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

Lines changed: 62 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.Reactive.Linq;
77
using System.Reactive.Threading.Tasks;
88
using System.Threading.Tasks;
9+
using EnvDTE;
910
using GitHub.Commands;
1011
using GitHub.Extensions;
1112
using GitHub.Models;
@@ -16,6 +17,7 @@
1617
using Microsoft.VisualStudio.Shell;
1718
using Microsoft.VisualStudio.Shell.Interop;
1819
using Microsoft.VisualStudio.Text;
20+
using Microsoft.VisualStudio.Text.Differencing;
1921
using Microsoft.VisualStudio.Text.Editor;
2022
using Microsoft.VisualStudio.Text.Projection;
2123
using Microsoft.VisualStudio.TextManager.Interop;
@@ -132,8 +134,6 @@ public async Task OpenDiff(IPullRequestSession session, string relativePath, str
132134
var file = await session.GetFile(relativePath, headSha ?? "HEAD");
133135
var mergeBase = await pullRequestService.GetMergeBase(session.LocalRepository, session.PullRequest);
134136
var encoding = pullRequestService.GetEncoding(session.LocalRepository, file.RelativePath);
135-
var rightPath = file.RelativePath;
136-
var leftPath = await GetBaseFileName(session, file);
137137
var rightFile = workingDirectory ?
138138
Path.Combine(session.LocalRepository.LocalPath, relativePath) :
139139
await pullRequestService.ExtractToTempFile(
@@ -142,12 +142,20 @@ await pullRequestService.ExtractToTempFile(
142142
relativePath,
143143
file.CommitSha,
144144
encoding);
145+
146+
if (FocusExistingDiffViewer(session, mergeBase, rightFile))
147+
{
148+
return;
149+
}
150+
145151
var leftFile = await pullRequestService.ExtractToTempFile(
146152
session.LocalRepository,
147153
session.PullRequest,
148154
relativePath,
149155
mergeBase,
150156
encoding);
157+
var leftPath = await GetBaseFileName(session, file);
158+
var rightPath = file.RelativePath;
151159
var leftLabel = $"{leftPath};{session.GetBaseBranchDisplay()}";
152160
var rightLabel = workingDirectory ? rightPath : $"{rightPath};PR {session.PullRequest.Number}";
153161
var caption = $"Diff - {Path.GetFileName(file.RelativePath)}";
@@ -177,9 +185,7 @@ await pullRequestService.ExtractToTempFile(
177185
(uint)options);
178186
}
179187

180-
object docView;
181-
frame.GetProperty((int)__VSFPROPID.VSFPROPID_DocView, out docView);
182-
var diffViewer = ((IVsDifferenceCodeWindow)docView).DifferenceViewer;
188+
var diffViewer = GetDiffViewer(frame);
183189

184190
AddBufferTag(diffViewer.LeftView.TextBuffer, session, leftPath, mergeBase, DiffSide.Left);
185191

@@ -388,6 +394,44 @@ IVsTextView OpenDocument(string fullPath)
388394
return view;
389395
}
390396

397+
bool FocusExistingDiffViewer(
398+
IPullRequestSession session,
399+
string mergeBase,
400+
string rightPath)
401+
{
402+
IVsUIHierarchy uiHierarchy;
403+
uint itemID;
404+
IVsWindowFrame windowFrame;
405+
406+
// Diff documents are indexed by the path on the right hand side of the comparison.
407+
if (VsShellUtilities.IsDocumentOpen(
408+
serviceProvider,
409+
rightPath,
410+
Guid.Empty,
411+
out uiHierarchy,
412+
out itemID,
413+
out windowFrame))
414+
{
415+
var diffViewer = GetDiffViewer(windowFrame);
416+
417+
if (diffViewer != null)
418+
{
419+
PullRequestTextBufferInfo leftBufferInfo;
420+
421+
if (diffViewer.LeftView.TextBuffer.Properties.TryGetProperty(
422+
typeof(PullRequestTextBufferInfo),
423+
out leftBufferInfo) &&
424+
leftBufferInfo.Session.PullRequest.Number == session.PullRequest.Number &&
425+
leftBufferInfo.CommitSha == mergeBase)
426+
{
427+
return ErrorHandler.Succeeded(windowFrame.Show());
428+
}
429+
}
430+
}
431+
432+
return false;
433+
}
434+
391435
void ShowErrorInStatusBar(string message)
392436
{
393437
statusBar.ShowMessage(message);
@@ -420,7 +464,7 @@ void AddBufferTag(
420464
}
421465
}
422466

423-
void EnableNavigateToEditor(IWpfTextView textView, IPullRequestSession session, IPullRequestSessionFile file)
467+
void EnableNavigateToEditor(ITextView textView, IPullRequestSession session, IPullRequestSessionFile file)
424468
{
425469
var view = vsEditorAdaptersFactory.GetViewAdapter(textView);
426470
EnableNavigateToEditor(view, session, file);
@@ -521,6 +565,18 @@ static string GetAbsolutePath(IPullRequestSession session, IPullRequestSessionFi
521565
return Path.Combine(localPath, relativePath);
522566
}
523567

568+
static IDifferenceViewer GetDiffViewer(IVsWindowFrame frame)
569+
{
570+
object docView;
571+
572+
if (ErrorHandler.Succeeded(frame.GetProperty((int)__VSFPROPID.VSFPROPID_DocView, out docView)))
573+
{
574+
return (docView as IVsDifferenceCodeWindow)?.DifferenceViewer;
575+
}
576+
577+
return null;
578+
}
579+
524580
static IDisposable OpenInProvisionalTab()
525581
{
526582
return new NewDocumentStateScope(

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

Lines changed: 32 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
using System.Collections.Generic;
1717
using LibGit2Sharp;
1818
using GitHub.Logging;
19+
using GitHub.Extensions;
20+
using static System.FormattableString;
1921

2022
namespace GitHub.Services
2123
{
@@ -467,11 +469,18 @@ public async Task<string> ExtractToTempFile(
467469
string commitSha,
468470
Encoding encoding)
469471
{
470-
using (var repo = gitService.GetRepository(repository.LocalPath))
472+
var tempFilePath = CalculateTempFileName(relativePath, commitSha, encoding);
473+
474+
if (!File.Exists(tempFilePath))
471475
{
472-
var remote = await gitClient.GetHttpRemote(repo, "origin");
473-
return await ExtractToTempFile(repo, pullRequest.Number, commitSha, relativePath, encoding);
476+
using (var repo = gitService.GetRepository(repository.LocalPath))
477+
{
478+
var remote = await gitClient.GetHttpRemote(repo, "origin");
479+
await ExtractToTempFile(repo, pullRequest.Number, commitSha, relativePath, encoding, tempFilePath);
480+
}
474481
}
482+
483+
return tempFilePath;
475484
}
476485

477486
public Encoding GetEncoding(ILocalRepositoryModel repository, string relativePath)
@@ -562,39 +571,30 @@ string CreateUniqueRemoteName(IRepository repo, string name)
562571
return uniqueName;
563572
}
564573

565-
async Task<string> ExtractToTempFile(
574+
async Task ExtractToTempFile(
566575
IRepository repo,
567576
int pullRequestNumber,
568577
string commitSha,
569-
string fileName,
570-
Encoding encoding)
578+
string relativePath,
579+
Encoding encoding,
580+
string tempFilePath)
571581
{
572582
string contents;
573-
583+
574584
try
575585
{
576-
contents = await gitClient.ExtractFile(repo, commitSha, fileName) ?? string.Empty;
586+
contents = await gitClient.ExtractFile(repo, commitSha, relativePath) ?? string.Empty;
577587
}
578588
catch (FileNotFoundException)
579589
{
580590
var pullHeadRef = $"refs/pull/{pullRequestNumber}/head";
581591
var remote = await gitClient.GetHttpRemote(repo, "origin");
582592
await gitClient.Fetch(repo, remote.Name, commitSha, pullHeadRef);
583-
contents = await gitClient.ExtractFile(repo, commitSha, fileName) ?? string.Empty;
593+
contents = await gitClient.ExtractFile(repo, commitSha, relativePath) ?? string.Empty;
584594
}
585595

586-
return CreateTempFile(fileName, commitSha, contents, encoding);
587-
}
588-
589-
static string CreateTempFile(string fileName, string commitSha, string contents, Encoding encoding)
590-
{
591-
var tempDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
592-
var tempFileName = $"{Path.GetFileNameWithoutExtension(fileName)}@{commitSha}{Path.GetExtension(fileName)}";
593-
var tempFile = Path.Combine(tempDir, tempFileName);
594-
595-
Directory.CreateDirectory(tempDir);
596-
File.WriteAllText(tempFile, contents, encoding);
597-
return tempFile;
596+
Directory.CreateDirectory(Path.GetDirectoryName(tempFilePath));
597+
File.WriteAllText(tempFilePath, contents, encoding);
598598
}
599599

600600
IEnumerable<string> GetLocalBranchesInternal(
@@ -674,6 +674,17 @@ static string GetSafeBranchName(string name)
674674
}
675675
}
676676

677+
static string CalculateTempFileName(string relativePath, string commitSha, Encoding encoding)
678+
{
679+
// The combination of relative path, commit SHA and encoding should be sufficient to uniquely identify a file.
680+
var relativeDir = Path.GetDirectoryName(relativePath) ?? string.Empty;
681+
var key = relativeDir + '|' + encoding.WebName;
682+
var relativePathHash = key.GetSha256Hash();
683+
var tempDir = Path.Combine(Path.GetTempPath(), "GitHubVisualStudio", "FileContents", relativePathHash);
684+
var tempFileName = Invariant($"{Path.GetFileNameWithoutExtension(relativePath)}@{commitSha}{Path.GetExtension(relativePath)}");
685+
return Path.Combine(tempDir, tempFileName);
686+
}
687+
677688
static string BuildGHfVSConfigKeyValue(IPullRequestModel pullRequest)
678689
{
679690
return pullRequest.Base.RepositoryCloneUrl.Owner + '#' +

‎src/GitHub.App/ViewModels/GitHubPane/PullRequestReviewAuthoringViewModel.cs‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,15 +57,18 @@ public PullRequestReviewAuthoringViewModel(
5757
.ToProperty(this, x => x.CanApproveRequestChanges);
5858

5959
Files = files;
60+
61+
var hasBodyOrComments = this.WhenAnyValue(
62+
x => x.Body,
63+
x => x.FileComments.Count,
64+
(body, comments) => !string.IsNullOrWhiteSpace(body) || comments > 0);
65+
6066
Approve = ReactiveCommand.CreateAsyncTask(_ => DoSubmit(Octokit.PullRequestReviewEvent.Approve));
6167
Comment = ReactiveCommand.CreateAsyncTask(
62-
this.WhenAnyValue(
63-
x => x.Body,
64-
x => x.FileComments.Count,
65-
(body, comments) => !string.IsNullOrWhiteSpace(body) || comments > 0),
68+
hasBodyOrComments,
6669
_ => DoSubmit(Octokit.PullRequestReviewEvent.Comment));
6770
RequestChanges = ReactiveCommand.CreateAsyncTask(
68-
this.WhenAnyValue(x => x.Body, body => !string.IsNullOrWhiteSpace(body)),
71+
hasBodyOrComments,
6972
_ => DoSubmit(Octokit.PullRequestReviewEvent.RequestChanges));
7073
Cancel = ReactiveCommand.CreateAsyncTask(DoCancel);
7174
}

‎src/GitHub.Extensions/StringExtensions.cs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,11 @@
44
using System.Globalization;
55
using System.IO;
66
using System.Linq;
7+
using System.Security.Cryptography;
78
using System.Text;
89
using System.Text.RegularExpressions;
10+
using GitHub.Logging;
11+
using Splat;
912

1013
namespace GitHub.Extensions
1114
{
@@ -222,5 +225,23 @@ public static string Humanize(this string s)
222225
var combined = String.Join(" ", result);
223226
return Char.ToUpper(combined[0], CultureInfo.InvariantCulture) + combined.Substring(1);
224227
}
228+
229+
/// <summary>
230+
/// Generates a SHA256 hash for a string.
231+
/// </summary>
232+
/// <param name="input">The input string.</param>
233+
/// <returns>The SHA256 hash.</returns>
234+
public static string GetSha256Hash(this string input)
235+
{
236+
Guard.ArgumentNotNull(input, nameof(input));
237+
238+
using (var sha256 = SHA256.Create())
239+
{
240+
var bytes = Encoding.UTF8.GetBytes(input);
241+
var hash = sha256.ComputeHash(bytes);
242+
243+
return string.Join("", hash.Select(b => b.ToString("x2", CultureInfo.InvariantCulture)));
244+
}
245+
}
225246
}
226247
}

‎test/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -593,7 +593,14 @@ public async Task ExtractsExistingFile()
593593
gitClient.ExtractFile(Arg.Any<IRepository>(), "123", "filename").Returns(GetFileTask(fileContent));
594594
var file = await target.ExtractToTempFile(repository, pr, "filename", "123", Encoding.UTF8);
595595

596-
Assert.That(File.ReadAllText(file), Is.EqualTo(fileContent));
596+
try
597+
{
598+
Assert.That(File.ReadAllText(file), Is.EqualTo(fileContent));
599+
}
600+
finally
601+
{
602+
File.Delete(file);
603+
}
597604
}
598605

599606
[Test]
@@ -607,7 +614,14 @@ public async Task CreatesEmptyFileForNonExistentFile()
607614
gitClient.ExtractFile(Arg.Any<IRepository>(), "123", "filename").Returns(GetFileTask(null));
608615
var file = await target.ExtractToTempFile(repository, pr, "filename", "123", Encoding.UTF8);
609616

610-
Assert.That(File.ReadAllText(file), Is.EqualTo(string.Empty));
617+
try
618+
{
619+
Assert.That(File.ReadAllText(file), Is.EqualTo(string.Empty));
620+
}
621+
finally
622+
{
623+
File.Delete(file);
624+
}
611625
}
612626

613627
// https://github.com/github/VisualStudio/issues/1010
@@ -631,8 +645,15 @@ public async Task CanChangeEncoding(string encodingName)
631645
gitClient.ExtractFile(Arg.Any<IRepository>(), "123", "filename").Returns(GetFileTask(fileContent));
632646
var file = await target.ExtractToTempFile(repository, pr, "filename", "123", encoding);
633647

634-
Assert.That(File.ReadAllText(expectedPath), Is.EqualTo(File.ReadAllText(file)));
635-
Assert.That(File.ReadAllBytes(expectedPath), Is.EqualTo(File.ReadAllBytes(file)));
648+
try
649+
{
650+
Assert.That(File.ReadAllText(expectedPath), Is.EqualTo(File.ReadAllText(file)));
651+
Assert.That(File.ReadAllBytes(expectedPath), Is.EqualTo(File.ReadAllBytes(file)));
652+
}
653+
finally
654+
{
655+
File.Delete(file);
656+
}
636657
}
637658

638659
static IPullRequestModel CreatePullRequest()

0 commit comments

Comments
 (0)