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

Commit fa8aab8

Browse files
grokysgrokys
committed
Display PR review comment inline.
When the user clicks a review comment represented by a `PullRequestReviewFileCommentViewModel`, open the comment inline in a diff view.
1 parent be015f4 commit fa8aab8

26 files changed

+498
-350
lines changed

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

Lines changed: 51 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,26 @@ public async Task OpenFile(
7171

7272
try
7373
{
74-
var file = await session.GetFile(relativePath);
75-
var fullPath = GetAbsolutePath(session, file);
76-
var fileName = workingDirectory ? fullPath : await ExtractFile(session, file, true);
74+
var fullPath = Path.Combine(session.LocalRepository.LocalPath, relativePath);
75+
string fileName;
76+
string commitSha;
77+
78+
if (workingDirectory)
79+
{
80+
fileName = fullPath;
81+
commitSha = null;
82+
}
83+
else
84+
{
85+
var file = await session.GetFile(relativePath);
86+
fileName = await pullRequestService.ExtractToTempFile(
87+
session.LocalRepository,
88+
session.PullRequest,
89+
file.RelativePath,
90+
file.CommitSha,
91+
pullRequestService.GetEncoding(session.LocalRepository, file.RelativePath));
92+
commitSha = file.CommitSha;
93+
}
7794

7895
using (workingDirectory ? null : OpenInProvisionalTab())
7996
{
@@ -84,7 +101,7 @@ public async Task OpenFile(
84101

85102
if (!workingDirectory)
86103
{
87-
AddBufferTag(buffer, session, fullPath, null);
104+
AddBufferTag(buffer, session, fullPath, commitSha, null);
88105
}
89106
}
90107

@@ -100,21 +117,33 @@ public async Task OpenFile(
100117
}
101118

102119
/// <inheritdoc/>
103-
public async Task OpenDiff(
104-
IPullRequestSession session,
105-
string relativePath,
106-
bool workingDirectory)
120+
public async Task OpenDiff(IPullRequestSession session, string relativePath, string headSha)
107121
{
108122
Guard.ArgumentNotNull(session, nameof(session));
109123
Guard.ArgumentNotEmptyString(relativePath, nameof(relativePath));
110124

111125
try
112126
{
113-
var file = await session.GetFile(relativePath);
127+
var workingDirectory = headSha == null;
128+
var file = await session.GetFile(relativePath, headSha ?? "HEAD");
129+
var mergeBase = await pullRequestService.GetMergeBase(session.LocalRepository, session.PullRequest);
130+
var encoding = pullRequestService.GetEncoding(session.LocalRepository, file.RelativePath);
114131
var rightPath = file.RelativePath;
115132
var leftPath = await GetBaseFileName(session, file);
116-
var rightFile = workingDirectory ? GetAbsolutePath(session, file) : await ExtractFile(session, file, true);
117-
var leftFile = await ExtractFile(session, file, false);
133+
var rightFile = workingDirectory ?
134+
Path.Combine(session.LocalRepository.LocalPath, relativePath) :
135+
await pullRequestService.ExtractToTempFile(
136+
session.LocalRepository,
137+
session.PullRequest,
138+
relativePath,
139+
file.CommitSha,
140+
encoding);
141+
var leftFile = await pullRequestService.ExtractToTempFile(
142+
session.LocalRepository,
143+
session.PullRequest,
144+
relativePath,
145+
mergeBase,
146+
encoding);
118147
var leftLabel = $"{leftPath};{session.GetBaseBranchDisplay()}";
119148
var rightLabel = workingDirectory ? rightPath : $"{rightPath};PR {session.PullRequest.Number}";
120149
var caption = $"Diff - {Path.GetFileName(file.RelativePath)}";
@@ -148,14 +177,11 @@ public async Task OpenDiff(
148177
frame.GetProperty((int)__VSFPROPID.VSFPROPID_DocView, out docView);
149178
var diffViewer = ((IVsDifferenceCodeWindow)docView).DifferenceViewer;
150179

151-
AddBufferTag(diffViewer.LeftView.TextBuffer, session, leftPath, DiffSide.Left);
180+
AddBufferTag(diffViewer.LeftView.TextBuffer, session, leftPath, mergeBase, DiffSide.Left);
152181

153182
if (!workingDirectory)
154183
{
155-
AddBufferTag(diffViewer.RightView.TextBuffer, session, rightPath, DiffSide.Right);
156-
EnableNavigateToEditor(diffViewer.LeftView, session, file);
157-
EnableNavigateToEditor(diffViewer.RightView, session, file);
158-
EnableNavigateToEditor(diffViewer.InlineView, session, file);
184+
AddBufferTag(diffViewer.RightView.TextBuffer, session, rightPath, file.CommitSha, DiffSide.Right);
159185
}
160186

161187
if (workingDirectory)
@@ -179,7 +205,7 @@ public async Task OpenDiff(
179205
Guard.ArgumentNotEmptyString(relativePath, nameof(relativePath));
180206
Guard.ArgumentNotNull(thread, nameof(thread));
181207

182-
await OpenDiff(session, relativePath, false);
208+
await OpenDiff(session, relativePath, thread.CommitSha);
183209

184210
// HACK: We need to wait here for the diff view to set itself up and move its cursor
185211
// to the first changed line. There must be a better way of doing this.
@@ -365,19 +391,24 @@ void ShowErrorInStatusBar(string message, Exception e)
365391
statusBar.ShowMessage(message + ": " + e.Message);
366392
}
367393

368-
void AddBufferTag(ITextBuffer buffer, IPullRequestSession session, string path, DiffSide? side)
394+
void AddBufferTag(
395+
ITextBuffer buffer,
396+
IPullRequestSession session,
397+
string path,
398+
string commitSha,
399+
DiffSide? side)
369400
{
370401
buffer.Properties.GetOrCreateSingletonProperty(
371402
typeof(PullRequestTextBufferInfo),
372-
() => new PullRequestTextBufferInfo(session, path, side));
403+
() => new PullRequestTextBufferInfo(session, path, commitSha, side));
373404

374405
var projection = buffer as IProjectionBuffer;
375406

376407
if (projection != null)
377408
{
378409
foreach (var source in projection.SourceBuffers)
379410
{
380-
AddBufferTag(source, session, path, side);
411+
AddBufferTag(source, session, path, commitSha, side);
381412
}
382413
}
383414
}
@@ -430,19 +461,6 @@ async Task DoNavigateToEditor(IPullRequestSession session, IPullRequestSessionFi
430461
}
431462
}
432463

433-
async Task<string> ExtractFile(IPullRequestSession session, IPullRequestSessionFile file, bool head)
434-
{
435-
var encoding = pullRequestService.GetEncoding(session.LocalRepository, file.RelativePath);
436-
var relativePath = head ? file.RelativePath : await GetBaseFileName(session, file);
437-
438-
return await pullRequestService.ExtractFile(
439-
session.LocalRepository,
440-
session.PullRequest,
441-
relativePath,
442-
head,
443-
encoding).ToTask();
444-
}
445-
446464
ITextBuffer GetBufferAt(string filePath)
447465
{
448466
IVsUIHierarchy uiHierarchy;

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

Lines changed: 21 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,20 @@ public IObservable<BranchTrackingDetails> CalculateHistoryDivergence(ILocalRepos
326326
});
327327
}
328328

329+
public async Task<string> GetMergeBase(ILocalRepositoryModel repository, IPullRequestModel pullRequest)
330+
{
331+
using (var repo = gitService.GetRepository(repository.LocalPath))
332+
{
333+
return await gitClient.GetPullRequestMergeBase(
334+
repo,
335+
pullRequest.Base.RepositoryCloneUrl,
336+
pullRequest.Base.Sha,
337+
pullRequest.Head.Sha,
338+
pullRequest.Base.Ref,
339+
pullRequest.Number);
340+
}
341+
}
342+
329343
public IObservable<TreeChanges> GetTreeChanges(ILocalRepositoryModel repository, IPullRequestModel pullRequest)
330344
{
331345
return Observable.Defer(async () =>
@@ -446,46 +460,18 @@ public IObservable<Tuple<string, int>> GetPullRequestForCurrentBranch(ILocalRepo
446460
});
447461
}
448462

449-
public IObservable<string> ExtractFile(
463+
public async Task<string> ExtractToTempFile(
450464
ILocalRepositoryModel repository,
451465
IPullRequestModel pullRequest,
452-
string fileName,
453-
bool head,
466+
string relativePath,
467+
string commitSha,
454468
Encoding encoding)
455469
{
456-
return Observable.Defer(async () =>
470+
using (var repo = gitService.GetRepository(repository.LocalPath))
457471
{
458-
using (var repo = gitService.GetRepository(repository.LocalPath))
459-
{
460-
var remote = await gitClient.GetHttpRemote(repo, "origin");
461-
string sha;
462-
463-
if (head)
464-
{
465-
sha = pullRequest.Head.Sha;
466-
}
467-
else
468-
{
469-
try
470-
{
471-
sha = await gitClient.GetPullRequestMergeBase(
472-
repo,
473-
pullRequest.Base.RepositoryCloneUrl,
474-
pullRequest.Base.Sha,
475-
pullRequest.Head.Sha,
476-
pullRequest.Base.Ref,
477-
pullRequest.Number);
478-
}
479-
catch (NotFoundException ex)
480-
{
481-
throw new NotFoundException($"The Pull Request file failed to load. Please check your network connection and click refresh to try again. If this issue persists, please let us know at [email protected]", ex);
482-
}
483-
}
484-
485-
var file = await ExtractToTempFile(repo, pullRequest.Number, sha, fileName, encoding);
486-
return Observable.Return(file);
487-
}
488-
});
472+
var remote = await gitClient.GetHttpRemote(repo, "origin");
473+
return await ExtractToTempFile(repo, pullRequest.Number, commitSha, relativePath, encoding);
474+
}
489475
}
490476

491477
public Encoding GetEncoding(ILocalRepositoryModel repository, string relativePath)

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

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -486,27 +486,6 @@ public override async Task Refresh()
486486
}
487487
}
488488

489-
/// <summary>
490-
/// Gets a file as it appears in the pull request.
491-
/// </summary>
492-
/// <param name="file">The changed file.</param>
493-
/// <param name="head">
494-
/// If true, gets the file at the PR head, otherwise gets the file at the PR merge base.
495-
/// </param>
496-
/// <returns>The path to a temporary file.</returns>
497-
public Task<string> ExtractFile(IPullRequestFileNode file, bool head)
498-
{
499-
var path = file.RelativePath;
500-
var encoding = pullRequestsService.GetEncoding(LocalRepository, path);
501-
502-
if (!head && file.OldPath != null)
503-
{
504-
path = file.OldPath;
505-
}
506-
507-
return pullRequestsService.ExtractFile(LocalRepository, model, path, head, encoding).ToTask();
508-
}
509-
510489
/// <summary>
511490
/// Gets the full path to a file in the working directory.
512491
/// </summary>

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,12 @@ public PullRequestFilesViewModel(
4444
this.service = service;
4545

4646
DiffFile = ReactiveCommand.CreateAsyncTask(x =>
47-
editorService.OpenDiff(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, false));
47+
editorService.OpenDiff(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, "HEAD"));
4848
ViewFile = ReactiveCommand.CreateAsyncTask(x =>
4949
editorService.OpenFile(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, false));
5050
DiffFileWithWorkingDirectory = ReactiveCommand.CreateAsyncTask(
5151
isBranchCheckedOut,
52-
x => editorService.OpenDiff(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, true));
52+
x => editorService.OpenDiff(pullRequestSession, ((IPullRequestFileNode)x).RelativePath));
5353
OpenFileInWorkingDirectory = ReactiveCommand.CreateAsyncTask(
5454
isBranchCheckedOut,
5555
x => editorService.OpenFile(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, true));

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

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using System;
2-
using System.Diagnostics.CodeAnalysis;
2+
using System.Linq;
33
using System.Reactive;
4+
using System.Threading.Tasks;
45
using GitHub.Extensions;
56
using GitHub.Models;
67
using GitHub.Services;
@@ -13,11 +14,10 @@ namespace GitHub.ViewModels.GitHubPane
1314
/// </summary>
1415
public class PullRequestReviewFileCommentViewModel : IPullRequestReviewFileCommentViewModel
1516
{
16-
[SuppressMessage("Microsoft.Performance", "CA1823:AvoidUnusedPrivateFields", Justification = "This will be used in a later PR")]
1717
readonly IPullRequestEditorService editorService;
18-
[SuppressMessage("Microsoft.Performance", "CA1823:AvoidUnusedPrivateFields", Justification = "This will be used in a later PR")]
1918
readonly IPullRequestSession session;
2019
readonly IPullRequestReviewCommentModel model;
20+
IInlineCommentThreadModel thread;
2121

2222
public PullRequestReviewFileCommentViewModel(
2323
IPullRequestEditorService editorService,
@@ -31,6 +31,8 @@ public PullRequestReviewFileCommentViewModel(
3131
this.editorService = editorService;
3232
this.session = session;
3333
this.model = model;
34+
35+
Open = ReactiveCommand.CreateAsyncTask(DoOpen);
3436
}
3537

3638
/// <inheritdoc/>
@@ -41,5 +43,27 @@ public PullRequestReviewFileCommentViewModel(
4143

4244
/// <inheritdoc/>
4345
public ReactiveCommand<Unit> Open { get; }
46+
47+
async Task DoOpen(object o)
48+
{
49+
try
50+
{
51+
if (thread == null)
52+
{
53+
var commit = model.Position.HasValue ? model.CommitId : model.OriginalCommitId;
54+
var file = await session.GetFile(RelativePath, commit);
55+
thread = file.InlineCommentThreads.FirstOrDefault(t => t.Comments.Any(c => c.Id == model.Id));
56+
}
57+
58+
if (thread != null && thread.LineNumber != -1)
59+
{
60+
await editorService.OpenDiff(session, RelativePath, thread);
61+
}
62+
}
63+
catch (Exception)
64+
{
65+
// TODO: Show error.
66+
}
67+
}
4468
}
4569
}

‎src/GitHub.Exports.Reactive/Models/IInlineCommentThreadModel.cs‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,19 +36,14 @@ public interface IInlineCommentThreadModel
3636
bool IsStale { get; set; }
3737

3838
/// <summary>
39-
/// Gets or sets the 0-based line number of the comment.
39+
/// Gets or sets the 0-based line number of the comment, or -1 of the thread is outdated.
4040
/// </summary>
4141
int LineNumber { get; set; }
4242

4343
/// <summary>
44-
/// Gets the SHA of the commit that the thread was left con.
44+
/// Gets the SHA of the commit that the thread appears on.
4545
/// </summary>
46-
string OriginalCommitSha { get; }
47-
48-
/// <summary>
49-
/// Gets the 1-based line number in the original diff that the thread was left on.
50-
/// </summary>
51-
int OriginalPosition { get; }
46+
string CommitSha { get; }
5247

5348
/// <summary>
5449
/// Gets the relative path to the file that the thread is on.

‎src/GitHub.Exports.Reactive/Models/IPullRequestSessionFile.cs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,12 @@ public interface IPullRequestSessionFile : INotifyPropertyChanged
3333
/// </summary>
3434
string CommitSha { get; }
3535

36+
/// <summary>
37+
/// Gets a value indicating whether <see cref="CommitSha"/> is tracking the related pull
38+
/// request HEAD or whether it is pinned at a particular commit.
39+
/// </summary>
40+
bool IsTrackingHead { get; }
41+
3642
/// <summary>
3743
/// Gets the path to the file relative to the repository.
3844
/// </summary>

0 commit comments

Comments
 (0)