Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moved changed files tree into its own view. #1492

Merged

Conversation

@grokys
Copy link
Contributor

@grokys grokys commented Feb 15, 2018

Ported from #1415. Moves the PR details changed files tree into its own view so that it can be shared by the PR review authoring view.

Part of #1491

@grokys grokys mentioned this pull request Feb 15, 2018
17 of 17 tasks complete
@grokys grokys requested a review from jcansdale Mar 16, 2018
Ported from #1415. Moves the PR details changed files tree into its own view so that it can be shared by the PR reviews view.
@grokys grokys force-pushed the refactor/pullrequestfilesviewmodel branch from e28ef32 to f72df0c Mar 16, 2018
// If the target line doesn't have a unique match, search this number of lines above looking for a match.
public const int MatchLinesAboveTarget = 4;

readonly IGitHubServiceProvider serviceProvider;

This comment has been minimized.

@jcansdale

jcansdale Mar 16, 2018
Collaborator

It looks like this is no longer being used.

This comment has been minimized.

@grokys

grokys Mar 16, 2018
Author Contributor

It is still being used!

@@ -2,47 +2,52 @@
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
xmlns:ghfvs="https://github.com/github/VisualStudio"

This comment has been minimized.

@jcansdale

jcansdale Mar 16, 2018
Collaborator

Should we be replacing this? Isn't it the new style of XAML reference that was recently merged?

This comment has been minimized.

@grokys

grokys Mar 16, 2018
Author Contributor

Oops, merge/rebase error!

}

[Import]
ITeamExplorerServiceHolder TeamExplorerServiceHolder { get; set; }

[Import]
IVisualStudioBrowser VisualStudioBrowser { get; set; }

This comment has been minimized.

@jcansdale

jcansdale Mar 16, 2018
Collaborator

I'm curious why we're not using an ImportingConstructor for this. Is this something specific to views?

This comment has been minimized.

@grokys

grokys Mar 16, 2018
Author Contributor

If you don't have a default constructor, the designer can't create an instance. We could have a default constructor as well as the importing constructor, but at the time this was written I guess we decided to do it like this.

xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
xmlns:cache="clr-namespace:GitHub.UI.Helpers;assembly=GitHub.UI"

This comment has been minimized.

@jcansdale

jcansdale Mar 16, 2018
Collaborator

Could we use xmlns:ghfvs="https://github.com/github/VisualStudio" for some of these?

This comment has been minimized.

@grokys

grokys Mar 16, 2018
Author Contributor

Yep, we could!

Copy link
Collaborator

@jcansdale jcansdale left a comment

See inline comments. There is also the following CA error:

MSBUILD : error CA1812: Microsoft.Performance : 'TextViewCommandDispatcher' is an internal class that is apparently never instantiated. If so, remove the code from the assembly. If this class is intended to contain only static methods, consider adding a private constructor to prevent the compiler from generating a default constructor.

grokys added 2 commits Mar 16, 2018
Enable navigation from diff view to editor
@grokys
Copy link
Contributor Author

@grokys grokys commented Mar 16, 2018

@jcansdale fixed those issues. Turns out #1407 wasn't ported to this branch, which was why TextViewCommandDispatcher wasn't being used. Ported that.

Copy link
Collaborator

@jcansdale jcansdale left a comment

Looks good LGTM.

@jcansdale jcansdale merged commit 31cdd81 into feature/pr-reviews-master Mar 23, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@jcansdale jcansdale deleted the refactor/pullrequestfilesviewmodel branch Mar 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.