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

Commit 799d5b9

Browse files
grokysgrokys
committed
Fix LHS comments.
We need to indicate what side of the diff has been affected in `IPullRequestSessionFile.LinesChanged`.
1 parent 7af0101 commit 799d5b9

File tree

13 files changed

+129
-78
lines changed

13 files changed

+129
-78
lines changed

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,12 @@
44

55
namespace GitHub.Models
66
{
7+
public enum DiffSide
8+
{
9+
Right,
10+
Left,
11+
}
12+
713
/// <summary>
814
/// Represents a file in a pull request.
915
/// </summary>
@@ -46,6 +52,6 @@ public interface IPullRequestSessionFile : INotifyPropertyChanged
4652
/// Gets an observable that is raised with a collection of 0-based line numbers when the
4753
/// review comments on the file are changed.
4854
/// </summary>
49-
IObservable<IReadOnlyList<int>> LinesChanged { get; }
55+
IObservable<IReadOnlyList<Tuple<int, DiffSide>>> LinesChanged { get; }
5056
}
5157
}

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

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,17 +15,15 @@ public class PullRequestTextBufferInfo
1515
/// </summary>
1616
/// <param name="session">The pull request session.</param>
1717
/// <param name="relativePath">The relative path to the file in the repository.</param>
18-
/// <param name="isLeftComparisonBuffer">
19-
/// Whether the buffer represents the left-hand-side of a comparison.
20-
/// </param>
18+
/// <param name="side">Which side of a diff comparision the buffer represents.</param>
2119
public PullRequestTextBufferInfo(
2220
IPullRequestSession session,
2321
string relativePath,
24-
bool isLeftComparisonBuffer)
22+
DiffSide? side)
2523
{
2624
Session = session;
2725
RelativePath = relativePath;
28-
IsLeftComparisonBuffer = isLeftComparisonBuffer;
26+
Side = side;
2927
}
3028

3129
/// <summary>
@@ -39,8 +37,8 @@ public PullRequestTextBufferInfo(
3937
public string RelativePath { get; }
4038

4139
/// <summary>
42-
/// Gets a value indicating whether the buffer represents the left-hand-side of a comparison.
40+
/// Gets a value indicating which side of a diff comparision the buffer represents.
4341
/// </summary>
44-
public bool IsLeftComparisonBuffer { get; }
42+
public DiffSide? Side { get; }
4543
}
4644
}

‎src/GitHub.InlineReviews/Models/PullRequestSessionFile.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ namespace GitHub.InlineReviews.Models
1919
/// <seealso cref="PullRequestSessionManager"/>
2020
public class PullRequestSessionFile : ReactiveObject, IPullRequestSessionFile
2121
{
22-
readonly Subject<IReadOnlyList<int>> linesChanged = new Subject<IReadOnlyList<int>>();
22+
readonly Subject<IReadOnlyList<Tuple<int, DiffSide>>> linesChanged = new Subject<IReadOnlyList<Tuple<int, DiffSide>>>();
2323
IReadOnlyList<DiffChunk> diff;
2424
string commitSha;
2525
IReadOnlyList<IInlineCommentThreadModel> inlineCommentThreads;
@@ -63,8 +63,8 @@ public IReadOnlyList<IInlineCommentThreadModel> InlineCommentThreads
6363
{
6464
var lines = (inlineCommentThreads ?? Enumerable.Empty<IInlineCommentThreadModel>())?
6565
.Concat(value ?? Enumerable.Empty<IInlineCommentThreadModel>())
66-
.Select(x => x.LineNumber)
67-
.Where(x => x >= 0)
66+
.Select(x => Tuple.Create(x.LineNumber, x.DiffLineType == DiffChangeType.Delete ? DiffSide.Left : DiffSide.Right))
67+
.Where(x => x.Item1 >= 0)
6868
.Distinct()
6969
.ToList();
7070
inlineCommentThreads = value;
@@ -73,12 +73,12 @@ public IReadOnlyList<IInlineCommentThreadModel> InlineCommentThreads
7373
}
7474

7575
/// <inheritdoc/>
76-
public IObservable<IReadOnlyList<int>> LinesChanged => linesChanged;
76+
public IObservable<IReadOnlyList<Tuple<int, DiffSide>>> LinesChanged => linesChanged;
7777

7878
/// <summary>
7979
/// Raises the <see cref="LinesChanged"/> signal.
8080
/// </summary>
8181
/// <param name="lines">The lines that have changed.</param>
82-
public void NotifyLinesChanged(IReadOnlyList<int> lines) => linesChanged.OnNext(lines);
82+
public void NotifyLinesChanged(IReadOnlyList<Tuple<int, DiffSide>> lines) => linesChanged.OnNext(lines);
8383
}
8484
}

‎src/GitHub.InlineReviews/Services/IPullRequestSessionService.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ IReadOnlyList<IInlineCommentThreadModel> BuildCommentThreads(
6464
/// <returns>
6565
/// A collection of updated line numbers.
6666
/// </returns>
67-
IReadOnlyList<int> UpdateCommentThreads(
67+
IReadOnlyList<Tuple<int, DiffSide>> UpdateCommentThreads(
6868
IReadOnlyList<IInlineCommentThreadModel> threads,
6969
IReadOnlyList<DiffChunk> diff);
7070

‎src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ void InvalidateLiveThreads(PullRequestSessionLiveFile file, ITextSnapshot snapsh
340340
{
341341
if (file.TrackingPoints != null)
342342
{
343-
var linesChanged = new List<int>();
343+
var linesChanged = new List<Tuple<int, DiffSide>>();
344344

345345
foreach (var thread in file.InlineCommentThreads)
346346
{
@@ -351,18 +351,18 @@ void InvalidateLiveThreads(PullRequestSessionLiveFile file, ITextSnapshot snapsh
351351
var position = trackingPoint.GetPosition(snapshot);
352352
var lineNumber = snapshot.GetLineNumberFromPosition(position);
353353

354-
if (lineNumber != thread.LineNumber)
354+
if (thread.DiffLineType != DiffChangeType.Delete && lineNumber != thread.LineNumber)
355355
{
356-
linesChanged.Add(lineNumber);
357-
linesChanged.Add(thread.LineNumber);
356+
linesChanged.Add(Tuple.Create(lineNumber, DiffSide.Right));
357+
linesChanged.Add(Tuple.Create(thread.LineNumber, DiffSide.Right));
358358
thread.LineNumber = lineNumber;
359359
thread.IsStale = true;
360360
}
361361
}
362362
}
363363

364364
linesChanged = linesChanged
365-
.Where(x => x >= 0)
365+
.Where(x => x.Item1 >= 0)
366366
.Distinct()
367367
.ToList();
368368

@@ -381,7 +381,7 @@ private IDictionary<IInlineCommentThreadModel, ITrackingPoint> BuildTrackingPoin
381381

382382
foreach (var thread in threads)
383383
{
384-
if (thread.LineNumber >= 0)
384+
if (thread.LineNumber >= 0 && thread.DiffLineType != DiffChangeType.Delete)
385385
{
386386
var line = snapshot.GetLineFromLineNumber(thread.LineNumber);
387387
var p = snapshot.CreateTrackingPoint(line.Start, PointTrackingMode.Positive);

‎src/GitHub.InlineReviews/Services/PullRequestSessionService.cs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -99,11 +99,11 @@ public IReadOnlyList<IInlineCommentThreadModel> BuildCommentThreads(
9999
}
100100

101101
/// <inheritdoc/>
102-
public IReadOnlyList<int> UpdateCommentThreads(
102+
public IReadOnlyList<Tuple<int, DiffSide>> UpdateCommentThreads(
103103
IReadOnlyList<IInlineCommentThreadModel> threads,
104104
IReadOnlyList<DiffChunk> diff)
105105
{
106-
var changedLines = new List<int>();
106+
var changedLines = new List<Tuple<int, DiffSide>>();
107107

108108
foreach (var thread in threads)
109109
{
@@ -130,8 +130,9 @@ public IReadOnlyList<int> UpdateCommentThreads(
130130

131131
if (changed)
132132
{
133-
if (oldLineNumber != -1) changedLines.Add(oldLineNumber);
134-
if (newLineNumber != -1 && newLineNumber != oldLineNumber) changedLines.Add(newLineNumber);
133+
var side = thread.DiffLineType == DiffChangeType.Delete ? DiffSide.Left : DiffSide.Right;
134+
if (oldLineNumber != -1) changedLines.Add(Tuple.Create(oldLineNumber, side));
135+
if (newLineNumber != -1 && newLineNumber != oldLineNumber) changedLines.Add(Tuple.Create(newLineNumber, side));
135136
}
136137
}
137138

‎src/GitHub.InlineReviews/Tags/InlineCommentTagger.cs‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public sealed class InlineCommentTagger : ITagger<InlineCommentTag>, IDisposable
3131
readonly IPullRequestSessionManager sessionManager;
3232
bool needsInitialize = true;
3333
string relativePath;
34-
bool leftHandSide;
34+
DiffSide side;
3535
IPullRequestSession session;
3636
IPullRequestSessionFile file;
3737
IDisposable fileSubscription;
@@ -101,8 +101,8 @@ public IEnumerable<ITagSpan<InlineCommentTag>> GetTags(NormalizedSnapshotSpanCol
101101
var snapshot = span.Snapshot;
102102
var line = snapshot.GetLineFromLineNumber(thread.LineNumber);
103103

104-
if ((leftHandSide && thread.DiffLineType == DiffChangeType.Delete) ||
105-
(!leftHandSide && thread.DiffLineType != DiffChangeType.Delete))
104+
if ((side == DiffSide.Left && thread.DiffLineType == DiffChangeType.Delete) ||
105+
(side == DiffSide.Right && thread.DiffLineType != DiffChangeType.Delete))
106106
{
107107
linesWithComments[thread.LineNumber - startLine] = true;
108108

@@ -116,12 +116,12 @@ public IEnumerable<ITagSpan<InlineCommentTag>> GetTags(NormalizedSnapshotSpanCol
116116
{
117117
foreach (var line in chunk.Lines)
118118
{
119-
var lineNumber = (leftHandSide ? line.OldLineNumber : line.NewLineNumber) - 1;
119+
var lineNumber = (side == DiffSide.Left ? line.OldLineNumber : line.NewLineNumber) - 1;
120120

121121
if (lineNumber >= startLine &&
122122
lineNumber <= endLine &&
123123
!linesWithComments[lineNumber - startLine]
124-
&& (!leftHandSide || line.Type == DiffChangeType.Delete))
124+
&& (side == DiffSide.Right || line.Type == DiffChangeType.Delete))
125125
{
126126
var snapshotLine = span.Snapshot.GetLineFromLineNumber(lineNumber);
127127
result.Add(new TagSpan<InlineCommentTag>(
@@ -151,8 +151,8 @@ async Task Initialize()
151151
session = bufferInfo.Session;
152152
relativePath = bufferInfo.RelativePath;
153153
file = await session.GetFile(relativePath);
154-
fileSubscription = file.LinesChanged.Subscribe(NotifyTagsChanged);
155-
leftHandSide = bufferInfo.IsLeftComparisonBuffer;
154+
fileSubscription = file.LinesChanged.Subscribe(LinesChanged);
155+
side = bufferInfo.Side ?? DiffSide.Right;
156156
NotifyTagsChanged();
157157
}
158158
else
@@ -175,7 +175,7 @@ async Task InitializeLiveFile()
175175
if (relativePath != null)
176176
{
177177
var liveFile = await sessionManager.GetLiveFile(relativePath, view, buffer);
178-
fileSubscription = liveFile.LinesChanged.Subscribe(NotifyTagsChanged);
178+
fileSubscription = liveFile.LinesChanged.Subscribe(LinesChanged);
179179
file = liveFile;
180180
}
181181
else
@@ -191,6 +191,11 @@ static void ForgetWithLogging(Task task)
191191
task.Catch(e => VsOutputLogger.WriteLine("Exception caught while executing background task: {0}", e)).Forget();
192192
}
193193

194+
void LinesChanged(IReadOnlyList<Tuple<int, DiffSide>> lines)
195+
{
196+
NotifyTagsChanged(lines.Where(x => x.Item2 == side).Select(x => x.Item1));
197+
}
198+
194199
void NotifyTagsChanged()
195200
{
196201
var entireFile = new SnapshotSpan(buffer.CurrentSnapshot, 0, buffer.CurrentSnapshot.Length);

‎src/GitHub.InlineReviews/ViewModels/InlineCommentPeekViewModel.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public sealed class InlineCommentPeekViewModel : ReactiveObject, IDisposable
3737
IDisposable threadSubscription;
3838
ITrackingPoint triggerPoint;
3939
string relativePath;
40-
bool leftBuffer;
40+
DiffSide side;
4141

4242
/// <summary>
4343
/// Initializes a new instance of the <see cref="InlineCommentPeekViewModel"/> class.
@@ -117,7 +117,7 @@ public async Task Initialize()
117117
if (info != null)
118118
{
119119
relativePath = info.RelativePath;
120-
leftBuffer = info.IsLeftComparisonBuffer;
120+
side = info.Side ?? DiffSide.Right;
121121
file = await info.Session.GetFile(relativePath);
122122
session = info.Session;
123123
await UpdateThread();
@@ -136,13 +136,13 @@ public async Task Initialize()
136136
fileSubscription = file.LinesChanged.Subscribe(LinesChanged);
137137
}
138138

139-
async void LinesChanged(IReadOnlyList<int> lines)
139+
async void LinesChanged(IReadOnlyList<Tuple<int, DiffSide>> lines)
140140
{
141141
try
142142
{
143143
var lineNumber = peekService.GetLineNumber(peekSession, triggerPoint).Item1;
144144

145-
if (lines.Contains(lineNumber))
145+
if (lines.Contains(Tuple.Create(lineNumber, side)))
146146
{
147147
await UpdateThread();
148148
}

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ async Task DoOpenFile(IPullRequestFileNode file, bool workingDirectory)
9797

9898
if (!workingDirectory)
9999
{
100-
AddBufferTag(buffer, ViewModel.Session, fullPath, false);
100+
AddBufferTag(buffer, ViewModel.Session, fullPath, null);
101101
}
102102
}
103103

@@ -154,11 +154,11 @@ async Task DoDiffFile(IPullRequestFileNode file, bool workingDirectory)
154154
var diffViewer = ((IVsDifferenceCodeWindow)docView).DifferenceViewer;
155155

156156
var session = ViewModel.Session;
157-
AddBufferTag(diffViewer.LeftView.TextBuffer, session, relativePath, true);
157+
AddBufferTag(diffViewer.LeftView.TextBuffer, session, relativePath, DiffSide.Left);
158158

159159
if (!workingDirectory)
160160
{
161-
AddBufferTag(diffViewer.RightView.TextBuffer, session, relativePath, false);
161+
AddBufferTag(diffViewer.RightView.TextBuffer, session, relativePath, DiffSide.Right);
162162
}
163163

164164
if (workingDirectory)
@@ -172,19 +172,19 @@ async Task DoDiffFile(IPullRequestFileNode file, bool workingDirectory)
172172
}
173173
}
174174

175-
void AddBufferTag(ITextBuffer buffer, IPullRequestSession session, string path, bool isLeftBuffer)
175+
void AddBufferTag(ITextBuffer buffer, IPullRequestSession session, string path, DiffSide? side)
176176
{
177177
buffer.Properties.GetOrCreateSingletonProperty(
178178
typeof(PullRequestTextBufferInfo),
179-
() => new PullRequestTextBufferInfo(session, path, isLeftBuffer));
179+
() => new PullRequestTextBufferInfo(session, path, side));
180180

181181
var projection = buffer as IProjectionBuffer;
182182

183183
if (projection != null)
184184
{
185185
foreach (var source in projection.SourceBuffers)
186186
{
187-
AddBufferTag(source, session, path, isLeftBuffer);
187+
AddBufferTag(source, session, path, side);
188188
}
189189
}
190190
}

‎test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,13 @@ Line 2
509509

510510
Assert.Equal(1, file.InlineCommentThreads.Count);
511511
Assert.Equal(4, file.InlineCommentThreads[0].LineNumber);
512-
Assert.Equal(new[] { 2, 4 }, linesChanged.ToArray());
512+
Assert.Equal(
513+
new[]
514+
{
515+
Tuple.Create(2, DiffSide.Right),
516+
Tuple.Create(4, DiffSide.Right),
517+
},
518+
linesChanged.ToArray());
513519
}
514520
}
515521

@@ -666,7 +672,7 @@ public async Task UpdatingCurrentSessionPullRequestTriggersLinesChanged()
666672
var raised = false;
667673
var pullRequest = target.CurrentSession.PullRequest;
668674

669-
file.LinesChanged.Subscribe(x => raised = x.Count == 1 && x[0] == expectedLineNumber);
675+
file.LinesChanged.Subscribe(x => raised = x.Count == 1 && x[0].Item1 == expectedLineNumber);
670676

671677
// LinesChanged should be raised even if the IPullRequestModel is the same.
672678
await target.CurrentSession.Update(target.CurrentSession.PullRequest);

0 commit comments

Comments
 (0)