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

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

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 Compare March 16, 2018 13:10
// 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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this is no longer being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is still being used!

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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, merge/rebase error!

ITeamExplorerServiceHolder TeamExplorerServiceHolder { get; set; }

[Import]
IVisualStudioBrowser VisualStudioBrowser { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, we could!

Copy link
Collaborator

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

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

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 March 16, 2018 17:37
Enable navigation from diff view to editor
@grokys
Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

Looks good LGTM.

@jcansdale jcansdale merged commit 31cdd81 into feature/pr-reviews-master Mar 23, 2018
@jcansdale jcansdale deleted the refactor/pullrequestfilesviewmodel branch March 23, 2018 16:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants