-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Moved changed files tree into its own view. #1492
Moved changed files tree into its own view. #1492
Conversation
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.
e28ef32 to
f72df0c
Compare
| // 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we could!
There was a problem hiding this 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.
Enable navigation from diff view to editor
|
@jcansdale fixed those issues. Turns out #1407 wasn't ported to this branch, which was why |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good LGTM.
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