Add a button to fetch when viewing partial content by RikkiGibson · Pull Request #1542 · microsoft/vscode-pull-request-github

RikkiGibson · 2020-03-08T00:09:30Z

Resolves #1462

This currently works by running a 'git fetch' and then reloading the whole window. I would appreciate some guidance on how to refresh things in a more granular way.

questions I have:

do comment caches need to be cleared after the fetch? I thought maybe they do if the comments are associated with the line numbers of the partial content.
do we need to re-resolve all file changes for the current PR? or maybe for all loaded PRs with the same base branch? It looks like the file change information is written right into the tree nodes, so it's difficult to refresh it without recreating the tree and maybe the associated diff views. but I wasn't 100% sure of what I was looking at.
haven't looked into automated testing yet, should I add some tests for this change?

RMacfarlane · 2020-03-16T19:17:59Z

Hey, thanks for creating this PR! Sorry for the delay in reviewing it!

Yes, both comments and the file content would need to be refreshed when this is done. For both of these, the data is stored at the tree node level, in pullRequestNode.ts. The comment cache is through the comment controller there. The file content that actually gets displayed is from the provideDocumentContent method.

From the command here, I think it would be possible to traverse up from the fileChangeNode to the PR node by following the parent, so one possibility is to do that and then add a method for specifically clearing the comment cache and rehydrating it with refreshExistingPREditors. In this scenario, an editor is already open with the old file content, so I think we also need to fire the onDidChange event for the TextDocumentContentProvider to refresh the editor content. (This event is currently not exposed, but is private on the _inMemContentProvider on the PR node.) Another potential solution is just calling refresh on the tree, which should handle resetting the comments and file contents, but would still need some additional event to refresh the editors that are already open.

There are some existing integration tests in prsTree.test.ts, but I don't think we have anything that checks the comment cache/file content on file open. They're pretty cumbersome to write because there's a high degree of mocking, both local git and responses from github, so I wouldn't worry about them

RikkiGibson · 2020-03-16T20:49:22Z

Thanks for the detailed guidance! I will take a shot at gracefully refreshing the content when I get the chance, sometime in the next couple of weeks.

RikkiGibson · 2020-03-30T01:28:59Z

Hi @RMacfarlane, I think I have figured out how to refresh the PR content pretty gracefully.

Some documents take a while to finish updating after the onChange event is kicked off, so I wanted there to be a progress notification while that was going on. I found it was necessary to do some more complex stuff to listen for the documents to finish updating. See allEditorsUpdatedPromise. I would like to know if there's a better/simpler way of doing what I'm trying to do.

RikkiGibson · 2020-03-30T18:11:25Z

Here, I recorded a better demo 😄

RikkiGibson · 2020-04-19T23:00:14Z

Hi @RMacfarlane, please let me know if there's anything I can do to help drive this PR :)

RMacfarlane

Nice, this looks really great! Just had some extremely minor comments - the tslint warnings print above the compilation output so are easy to miss. Will merge this in after that is fixed. Thank you!

RMacfarlane · 2020-04-20T16:58:26Z

src/commands.ts


 	context.subscriptions.push(vscode.commands.registerCommand('pr.openDiffView', async (fileChangeNode: GitFileChangeNode | InMemFileChangeNode) => {
+		const GIT_FETCH_COMMAND = 'Run \'git fetch\'';
+		const TITLE = "GitHub Pull Requests";


tslint warning here: " should be '

RMacfarlane · 2020-04-20T16:58:56Z

src/view/treeNodes/pullRequestNode.ts

+				const remainingEditors = initiallyVisibleEditors.slice();
+
+				const handler = (document: vscode.TextDocument) => {
+					const index = remainingEditors.findIndex(editor => editor.document == document);


tslint warning: == should be ===

RMacfarlane · 2020-04-20T17:00:05Z

src/commands.ts

 		}

 		vscode.commands.executeCommand('vscode.diff', parentURI, headURI, fileName, opts);
+		if (isPartial) {


I think this should still be above the vscode.diff command

Sounds good. I changed 'await' to '.then()' to make sure the diff still comes up when the user hasn't selected an option in the information message yet.

RikkiGibson · 2020-04-20T18:13:20Z

@RMacfarlane Thanks for the review! I have addressed your feedback. Sorry about the lint warnings, they must have snuck in last minute. 😄

RMacfarlane · 2020-04-20T18:15:42Z

Thank you!

Add a button to fetch when viewing partial content

43e117d

RikkiGibson added 2 commits March 29, 2020 16:29

Refresh PR after fetching

bbfa28c

Listen for documents to finish reloading

3394f61

RikkiGibson changed the title ~~[WIP] Add a button to fetch when viewing partial content~~ Add a button to fetch when viewing partial content Mar 30, 2020

RMacfarlane suggested changes Apr 20, 2020

View reviewed changes

Address feedback

43cc6cc

RMacfarlane approved these changes Apr 20, 2020

View reviewed changes

RMacfarlane merged commit 245c3ee into microsoft:master Apr 20, 2020

RikkiGibson deleted the fetchPRBaseBranch branch April 20, 2020 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a button to fetch when viewing partial content#1542

Add a button to fetch when viewing partial content#1542
RMacfarlane merged 4 commits intomicrosoft:masterfrom
RikkiGibson:fetchPRBaseBranch

RikkiGibson commented Mar 8, 2020

Uh oh!

RMacfarlane commented Mar 16, 2020

Uh oh!

RikkiGibson commented Mar 16, 2020

Uh oh!

RikkiGibson commented Mar 30, 2020 •

edited

Loading

Uh oh!

RikkiGibson commented Mar 30, 2020

Uh oh!

RikkiGibson commented Apr 19, 2020

Uh oh!

RMacfarlane left a comment

Uh oh!

RMacfarlane Apr 20, 2020

Uh oh!

RMacfarlane Apr 20, 2020

Uh oh!

RMacfarlane Apr 20, 2020

Uh oh!

RikkiGibson Apr 20, 2020

Uh oh!

RikkiGibson commented Apr 20, 2020

Uh oh!

RMacfarlane commented Apr 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

RikkiGibson commented Mar 8, 2020

Uh oh!

RMacfarlane commented Mar 16, 2020

Uh oh!

RikkiGibson commented Mar 16, 2020

Uh oh!

RikkiGibson commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson commented Mar 30, 2020

Uh oh!

RikkiGibson commented Apr 19, 2020

Uh oh!

RMacfarlane left a comment

Choose a reason for hiding this comment

Uh oh!

RMacfarlane Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

RMacfarlane Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

RMacfarlane Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Apr 20, 2020

Uh oh!

RMacfarlane commented Apr 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

RikkiGibson commented Mar 30, 2020 •

edited

Loading