Add inline comments #1004

grokys · 2017-06-05T08:43:24Z

This PR adds inline comments for Pull Requests to editor and compare views in Visual Studio.

This PR implements the MVP for this feature, namely:

Conflicts: .ncrunch/GitHub.UI.UnitTests.v3.ncrunchproject src/GitHub.Exports/Settings/Guids.cs

…e-reviews

Use a heuristic to recalculate comment positions when opening a file in response to any changes that may be present locally.

When editing occurs, recalculate comment positions on a background thread.

One of the files might not exist of the file is added or deleted.

`IVsDifferenceService.OpenComparisonWindow2` doesn't allow us to pass extra information to the tagger, such as the path to the file being tagged (the left file path is always passed, which may be a temp file). Hack around this by storing global state in the pull request review session object (ugh).

Previously it was exposing an `AccountCacheItem` instead of an `IAccount` which wasn't that useful.

Doesn't actually do anything yet mind.

Make margin visible on Left, Right and Inline views.

The glyphs may be in the wrong place, and they're displayed erroneously on the RHS too, but they're there.

This required a rewrite of our diff handling - now using VS' inbuilt differ rather than DiffPlex.

There's a distinction - reviews are made of comments.

GitHub.VisualStudio was previously referencing it directly meaning that it wasn't found on a machine without VS2015 installed.

It was causing a crash on VS2017 for some reason.

jcansdale · 2017-07-01T09:46:17Z

src/GitHub.Exports/Models/DiffUtilities.cs

+                    }
+                    else
+                    {
+                        j = 0;


I wonder if we could do second, more forgiving pass that still matches if lines have been added? That would address the inconsistency where you can add lines above the chunk, but if you happen to add a line within the chunk the comment dissapears (for me this is the most disconcerting bit).

I'm sure there are fancier things we could do later, but this would be simple, consistent and predictable. What do you think?

Actually, one really simple thing we could do is treat / as white space when matching lines. That way a developer could // out lines and not immediately loose their inline comments.

Simply ignoring white-space and comments actually works pretty well. It means you can comment out the line with an inline-comment and add stuff below. For example:

You can see I've added j = 777, but the inline-comment is still there for reference.

…sualStudio into feature/inline-reviews

shana · 2017-07-04T17:02:18Z

src/GitHub.App/Services/GitClient.cs

+            return Task.Factory.StartNew(() =>
+            {
+                var commit = repository.Lookup<Commit>(commitSha);
+                if(commit == null)


Space missing!

shana · 2017-07-04T17:18:54Z

src/GitHub.App/Services/PullRequestService.cs

+            var tempFile = Path.Combine(tempDir, tempFileName);
+
+            Directory.CreateDirectory(tempDir);
+            File.WriteAllText(tempFile, contents, encoding);


Which thread is this happening in? If it's on the main thread, IO is slow for some reason and the file is large, this may block the UI. All IO should be off on a side thread to avoid this.

shana · 2017-07-05T09:52:36Z

src/GitHub.App/Services/PullRequestService.cs

+                if (mergeBase == null)
                {
-                    throw new FileNotFoundException($"Could not retrieve {fileName}@{pullRequest.Base.Sha}");
+                    throw new FileNotFoundException($"Couldn't find merge base between {baseSha} and {headSha}.");


The exception type doesn't match the actual exception, this isn't an IO operation. Perhaps a custom exception type could be useful here for conveying meaning?

I was reluctant to create a new exception type for something that really shouldn't happen. I've changed it to InvalidOperationException which we're using elsewhere in this class. Still not ideal, but at least it isn't an IO exception.

In #1036 I changed it to LibGit2Sharp.NotFoundException

Thanks for the heads up. I'll revert my other commit. Did you fix a test for that as well?

Yep, I changed the test to expect NotFoundException too.

shana · 2017-07-05T10:12:18Z

src/GitHub.App/Services/PullRequestService.cs

+        static string CreateTempFile(string fileName, string commitSha, string contents, Encoding encoding)
+        {
+            var tempDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
+            var tempFileName = $"{Path.GetFileNameWithoutExtension(fileName)}@{commitSha}{Path.GetExtension(fileName)}";


We shouldn't be trusting that the filename coming from the PR is valid under the current locale and encoding or even valid to Windows. There are certain filenames that can be created in other OSs that the Windows fs can't deal with, and there are certain characters and encodings that might not match the local. In this case, ignoring the string formatting warning is not a good option, and using the original filename to create a temp file is probably not a good option either.

We're already creating a GUID in the directory name, if we use the GUID instead for the filename we avoid both issues entirely. Alternatively, if you want to reuse the cached temp file, you could encode the filename with Convert.ToBase64String(Encoding.UTF8.GetBytes(filename)) and put it in a known temp directory.

At the moment I don't think the file path is surfaced anywhere, but it is visible in the #1036 PR (which isn't ideal).

Maybe we should find a way to sanitize the file name and not include the commitSha? Would URL-encoding the filename work? I think we definitely need to need to keep the file extension for automatic syntax highlighting.

shana · 2017-07-05T10:13:26Z

src/GitHub.App/Services/PullRequestService.cs

+            var tempFileName = $"{Path.GetFileNameWithoutExtension(fileName)}@{commitSha}{Path.GetExtension(fileName)}";
+            var tempFile = Path.Combine(tempDir, tempFileName);
+
+            Directory.CreateDirectory(tempDir);


We have an OS facade to abstract the filesystem out for these things. We should use it everywhere, for consistency.

shana · 2017-07-05T10:26:52Z

src/GitHub.Extensions/StringExtensions.cs

            return sb.ToString();
        }

+        public static bool EqualsIgnoringLineEndings(this string a, string b)


This isn't being used anywhere, is it still needed?

shana · 2017-07-05T10:45:48Z

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

            }
        }
+
+        private void ViewCommentsClick(object sender, RoutedEventArgs e)


👀 private 👀

shana · 2017-07-05T10:46:00Z

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

+                null);
+        }
+
+        private async void ViewFileCommentsClick(object sender, RoutedEventArgs e)


👀 private 👀

shana · 2017-07-05T10:50:18Z

src/GitHub.App/Services/PullRequestService.cs

+            {
+                foreach (var b in encoding.GetPreamble())
+                {
+                    if(b != stream.ReadByte())


shana · 2017-07-05T11:02:05Z

src/GitHub.InlineReviews/Glyph/GlyphMarginVisualManager.cs

+                glyphs = dictionary;
+            }
+        }
+        static ITextViewLine GetStartingLine(IList<ITextViewLine> lines, Span span)


Missing newline

shana · 2017-07-05T11:03:35Z

src/GitHub.InlineReviews/Glyph/GlyphMarginVisualManager.cs

+            }
+        }
+
+        public FrameworkElement MarginVisual => glyphMarginGrid;


Properties should be defined after methods

shana · 2017-07-05T11:08:00Z

src/GitHub.InlineReviews/Peek/InlineCommentPeekResult.cs

+
+        public event EventHandler Disposed;
+
+        public void Dispose()


This needs implementing

I don't think there's actually anything we need to dispose here. The reason this method appears is that IPeekResult implements IDisposable so we need to implement it too.

I've marked it as sealed because it isn't intended to be extended.

shana · 2017-07-05T11:13:51Z

src/GitHub.InlineReviews/ViewModels/CommentViewModel.cs

+        public ReactiveCommand<object> CancelEdit { get; }
+
+        /// <inheritdoc/>
+        public ReactiveCommand<Unit> CommitEdit { get; }


All of these properties should be after methods

shana · 2017-07-05T11:18:44Z

src/GitHub.InlineReviews/ViewModels/InlineCommentPeekViewModel.cs

+        /// <summary>
+        /// Gets the thread of comments to display.
+        /// </summary>
+        public ICommentThreadViewModel Thread


This should probably called something other than Thread, since there's a Thread type that can easily be confused with it, and makes the code harder to read. We call a series of related comments a Discussion on .com.

I'd not considered this; I've used Thread in the names of quite a few types as well.

shana · 2017-07-05T11:21:46Z

src/GitHub.InlineReviews/Peek/InlineCommentPeekableItemSource.cs

+                    nextCommentCommand,
+                    previousCommentCommand);
+                viewModel.Initialize().Forget();
+                peekableItems.Add(new InlineCommentPeekableItem(viewModel));


The viewmodel may not be finished initializing by the time it's added to this list. What is the impact of that?

It should be fine - the view model will update itself in-place. The reason I did it like this was to make clear that there's async work to be done - the other options were to do the async work in the ctor or make Initialize an async void method.

shana · 2017-07-05T11:23:15Z

src/GitHub.InlineReviews/Services/PullRequestSession.cs

+        IEnumerable<string> FilePaths
+        {
+            get { return PullRequest.ChangedFiles.Select(x => x.FileName); }
+        }


Properties after methods

shana · 2017-07-05T11:29:47Z

src/GitHub.InlineReviews/Services/PullRequestSessionService.cs

+            catch (FileNotFoundException)
+            {
+                var pullHeadRef = $"refs/pull/{pullRequestNumber}/head";
+                await gitClient.Fetch(repo, "origin", sha, pullHeadRef);


This needs to use an https remote, so it needs to do var remote = await gitClient.GetHttpRemote(repo); and call fetch with the returned remote.

Already fixed this in the work associated with the PR details right-click menu.

shana

Left a bunch of comments and there are things to address in subsequent PRs. Been using this branch to review, it works nicely! 😄

grokys and others added 30 commits April 20, 2017 21:28

Merge branch 'master' into feature/inline-reviews

8efee79

Conflicts: .ncrunch/GitHub.UI.UnitTests.v3.ncrunchproject src/GitHub.Exports/Settings/Guids.cs

Merge branch 'fixes/shareddictionarymanager-merge' into feature/inlin…

d9f5779

…e-reviews

Recalculate comment positions.

32ffa2f

Use a heuristic to recalculate comment positions when opening a file in response to any changes that may be present locally.

Recalculate comment positions on edit.

3591bf8

When editing occurs, recalculate comment positions on a background thread.

Use tracking points to track comment positions.

7d6bf93

Display # of file comments in changes tree.

cf0b56b

Fix extracting files for diffs.

67e6c70

One of the files might not exist of the file is added or deleted.

Make inline comments display again.

150d0f8

Expose current user from ModelService.

bd9126c

Previously it was exposing an `AccountCacheItem` instead of an `IAccount` which wasn't that useful.

Display a text box to add a reply to a comment.

2795e7b

Doesn't actually do anything yet mind.

Added comment commit and cancel buttons.

7df9cf0

Initial implementation of PR comment replies.

8b5c5c1

Display errors in CommentView.

2918f93

Notify PR review session of added comments.

669f52e

Simplify rx query.

4687721

Show scrollbars in CommentBlockView.

91a6e61

Enable Glyph margin when diffing PR files

c100249

Make margin visible on Left, Right and Inline views.

Allow GitHub.InlineReviews to load in VS 2017

b59a49f

Count comments that have an OriginalPosition.

b858d9d

Handle ConvertTabsToSpaces editor setting.

790c864

Handle options not being defined.

8e4ec75

Removed compare view hack.

af581e3

Display review glyphs on LHS of diff view.

3e41d5d

The glyphs may be in the wrong place, and they're displayed erroneously on the RHS too, but they're there.

Fix positioning of review comments.

f6491f2

This required a rewrite of our diff handling - now using VS' inbuilt differ rather than DiffPlex.

Display comment count in PR details view.

3ec2226

Renamed Review -> InlineComment

5ed5c29

There's a distinction - reviews are made of comments.

Install Microsoft.VisualStudio.Editor package.

3a73da0

GitHub.VisualStudio was previously referencing it directly meaning that it wasn't found on a machine without VS2015 installed.

Use one way binding for CommentCount.

32a985e

It was causing a crash on VS2017 for some reason.

AllUsers should by false when Experimental

5980af1

jcansdale added 2 commits June 30, 2017 15:46

Remove redundant span assignments

2a98831

Remove redundant comments

21821b2

jcansdale reviewed Jul 1, 2017

View reviewed changes

grokys added 2 commits July 4, 2017 15:17

Be sure to call GetHttpRemote.

41a76f8

Merge branch 'feature/inline-reviews' of https://github.com/github/Vi…

46011fb

…sualStudio into feature/inline-reviews

shana reviewed Jul 4, 2017

View reviewed changes

shana reviewed Jul 5, 2017

View reviewed changes

shana mentioned this pull request Jul 5, 2017

Create new issues from comments #1029

Open

shana reviewed Jul 5, 2017

View reviewed changes

src/GitHub.App/Services/PullRequestService.cs

{

foreach (var b in encoding.GetPreamble())

{

if(b != stream.ReadByte())

Copy link

Contributor

shana Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if space!

shana reviewed Jul 5, 2017

View reviewed changes

shana mentioned this pull request Jul 5, 2017

Cleanup and document GitHub.InlineReviews classes #1030

Open

shana approved these changes Jul 5, 2017

View reviewed changes

shana merged commit 9020124 into master Jul 5, 2017

shana deleted the feature/inline-reviews branch July 5, 2017 11:46

jcansdale mentioned this pull request Jul 6, 2017

Added extra items to file menu in PR details view. #1036

Merged

paladique mentioned this pull request Jul 7, 2017

Error opening file: Could not retrieve <<filename>>@<<githash>> when trying to compare files in pull request #1026

Closed

jcansdale mentioned this pull request Jul 18, 2017

Surfacing interesting inline comments threads #1077

Open

Add inline comments #1004

Add inline comments #1004

Uh oh!

Conversation

grokys commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcansdale Jul 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shana left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

grokys commented Jun 5, 2017 •

edited

Loading

jcansdale Jul 3, 2017 •

edited

Loading