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

Moved changed files tree into its own view. - #1492

Merged
jcansdale merged 4 commits into
feature/pr-reviews-masterfrom
refactor/pullrequestfilesviewmodel
Mar 23, 2018
Merged

Moved changed files tree into its own view.#1492
jcansdale merged 4 commits into
feature/pr-reviews-masterfrom
refactor/pullrequestfilesviewmodel

Conversation

@grokys

@grokysgrokys commented Feb 15, 2018

Copy link
Copy Markdown
Contributor

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

@grokysgrokys mentioned this pull request Feb 15, 2018
17 tasks
@grokys
grokys requested a review from jcansdaleMarch 16, 2018 09:56
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
grokysforce-pushed the refactor/pullrequestfilesviewmodel branch from e28ef32 to f72df0cCompareMarch 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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yep, we could!

@jcansdalejcansdale left a comment

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
ContributorAuthor

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

@jcansdalejcansdale left a comment

Copy link
Copy Markdown
Collaborator

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-masterMar 23, 2018
@jcansdale
jcansdale deleted the refactor/pullrequestfilesviewmodel branch March 23, 2018 16:33
Sign up for freeto 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

@grokys@jcansdale