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

Highlight checked out PR in list. #1106

Merged
merged 11 commits into from Mar 5, 2018
Merged

Highlight checked out PR in list. #1106

merged 11 commits into from Mar 5, 2018

Conversation

@grokys
Copy link
Contributor

@grokys grokys commented Aug 3, 2017

This PR highlights the currently checked out PR in the PR list.

Blue:

devenv_2018-02-21_16-00-37

Dark:

2018-02-21_16-01-05

Light:

2018-02-21_16-01-29

Not yet implemented

To link a branch to a PR we use a property set in the git .config file - currently this is only set when a branch is a) checked out in the PR details view b) the PR details are opened for the current branch's PR.

We can actually do better here: when we load the PR list we have enough information to mark all PR branches with their related PR but there are a couple of things preventing us from doing this:

  1. TrackingCollection.OriginalCompleted is fired in the PR list when the list starts loading for the first time, not when the load completes. This works correctly on subsequent refreshes - appears to be a bug in TrackingCollection - now fixed
  2. TrackingCollection.original would need to be exposed so we can get an unfiltered list of all PRs
  3. Navigating to the PR list causes a reload of all PRs (#1105) which means we'd be doing this way more often than we should; as the operation involves opening the repository and reading/writing config values this may not be a good idea. This appears to be caused by the fact that in the navigation controller there is no distinction between loading a page for the first time and moving to an already loaded page. - now fixed

I think this should probably be implemented in another PR, so bear in mind that the current PR may not be highlighted until you open its details.

@grokys grokys requested review from shana, donokuda and jcansdale Aug 3, 2017
@donokuda
Copy link
Member

@donokuda donokuda commented Aug 8, 2017

The GitHub pane is throwing this error when I try to test this branch out:

An exception was encountered while constructing the content of this frame.  This information is also logged in "C:\Users\donokuda\AppData\Roaming\Microsoft\VisualStudio\15.0_f08b9986Exp\ActivityLog.xml".

Exception details:
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> Microsoft.VisualStudio.Composition.CompositionFailedException: An exception was thrown while initializing part "GitHub.VisualStudio.UI.Views.GitHubPaneView". ---> System.Windows.Markup.XamlParseException: 'Set property 'System.Windows.ResourceDictionary.DeferrableContent' threw an exception.' Line number '3' and line position '21'. ---> System.Xaml.XamlObjectWriterException: 'Provide value on 'MS.Internal.Markup.StaticExtension' threw an exception.' Line number '4' and line position '45'. ---> System.Xaml.XamlParseException: Type reference cannot find type named '{clr-namespace:Markdig.Wpf;assembly=Markdig.Wpf}Styles'.
   at MS.Internal.Xaml.Context.ObjectWriterContext.ServiceProvider_Resolve(String qName)
   at MS.Internal.Xaml.ServiceProviderContext.System.Windows.Markup.IXamlTypeResolver.Resolve(String qName)
   at MS.Internal.Markup.StaticExtension.ProvideValue(IServiceProvider serviceProvider)
   at MS.Internal.Xaml.Runtime.ClrObjectRuntime.CallProvideValue(MarkupExtension me, IServiceProvider serviceProvider)
   --- End of inner exception stack trace ---
   at MS.Internal.Xaml.Runtime.ClrObjectRuntime.CallProvideValue(MarkupExtension me, IServiceProvider serviceProvider)
   at System.Xaml.XamlObjectWriter.Logic_ProvideValue(ObjectWriterContext ctx)
   at System.Xaml.XamlObjectWriter.Logic_AssignProvidedValue(ObjectWriterContext ctx)
   at System.Xaml.XamlObjectWriter.WriteEndObject()
   at System.Xaml.XamlWriter.WriteNode(XamlReader reader)
   at System.Xaml.XamlServices.Transform(XamlReader xamlReader, XamlWriter xamlWriter, Boolean closeWriter)
   at System.Windows.ResourceDictionary.EvaluateMarkupExtensionNodeList(XamlReader reader, IServiceProvider serviceProvider)
   at System.Windows.ResourceDictionary.GetKeyValue(KeyRecord key, IServiceProvider serviceProvider)
   at System.Windows.ResourceDictionary.SetKeys(IList`1 keyCollection, IServiceProvider serviceProvider)
   at System.Windows.ResourceDictionary.SetDeferrableContent(DeferrableContent deferrableContent)
   at System.Windows.Baml2006.WpfSharedBamlSchemaContext.<>c.<Create_BamlProperty_ResourceDictionary_DeferrableContent>b__297_0(Object target, Object value)
   at System.Windows.Baml2006.WpfKnownMemberInvoker.SetValue(Object instance, Object value)
   at MS.Internal.Xaml.Runtime.ClrObjectRuntime.SetValue(XamlMember member, Object obj, Object value)
   at MS.Internal.Xaml.Runtime.ClrObjectRuntime.SetValue(Object inst, XamlMember property, Object value)
   --- End of inner exception stack trace ---
   at System.Windows.Markup.WpfXamlLoader.Load(XamlReader xamlReader, IXamlObjectWriterFactory writerFactory, Boolean skipJournaledProperties, Object rootObject, XamlObjectWriterSettings settings, Uri baseUri)
   at System.Windows.Markup.WpfXamlLoader.LoadBaml(XamlReader xamlReader, Boolean skipJournaledProperties, Object rootObject, XamlAccessLevel accessLevel, Uri baseUri)
   at System.Windows.Markup.XamlReader.LoadBaml(Stream stream, ParserContext parserContext, Object parent, Boolean closeStream)
   at System.Windows.Application.LoadComponent(Object component, Uri resourceLocator)
   at GitHub.VisualStudio.UI.Views.GitHubPaneView.InitializeComponent() in C:\Users\donokuda\Source\Repos\VisualStudio\src\GitHub.VisualStudio\UI\Views\GitHubPaneView.xaml:line 1
   at GitHub.VisualStudio.UI.Views.GitHubPaneView..ctor(INotificationDispatcher notifications) in C:\Users\donokuda\Source\Repos\VisualStudio\src\GitHub.VisualStudio\UI\Views\GitHubPaneView.xaml.cs:line 26
   --- End of inner exception stack trace ---
   at Microsoft.VisualStudio.Composition.RuntimeExportProviderFactory.RuntimeExportProvider.RuntimePartLifecycleTracker.CreateValue()
   at Microsoft.VisualStudio.Composition.ExportProvider.PartLifecycleTracker.Create()
   at Microsoft.VisualStudio.Composition.ExportProvider.PartLifecycleTracker.MoveNext(PartLifecycleState nextState)
   at Microsoft.VisualStudio.Composition.ExportProvider.PartLifecycleTracker.GetValueReadyToExpose()
   at Microsoft.VisualStudio.Composition.RuntimeExportProviderFactory.RuntimeExportProvider.<>c__DisplayClass13_0.<CreateExportFactory>b__0()
   at Microsoft.VisualStudio.Composition.ExportProvider.<>c__DisplayClass54_0.<CreateExportFactory>b__0()
   at Microsoft.VisualStudio.Composition.DelegateServices.<>c__DisplayClass2_0`1.<As>b__0()
   at System.ComponentModel.Composition.ExportFactory`1.CreateExport()
   at GitHub.Services.ExportFactoryProvider.GetView(UIViewType viewType) in C:\Users\donokuda\Source\Repos\VisualStudio\src\GitHub.Exports\Services\ExportFactoryProvider.cs:line 55
   at GitHub.App.Factories.UIFactory.CreateViewAndViewModel(UIViewType viewType) in C:\Users\donokuda\Source\Repos\VisualStudio\src\GitHub.App\Factories\UIFactory.cs:line 36
   at GitHub.VisualStudio.UI.UIProvider.GetView(UIViewType which, ViewWithData data) in C:\Users\donokuda\Source\Repos\VisualStudio\src\GitHub.VisualStudio\Services\UIProvider.cs:line 66
   at GitHub.VisualStudio.UI.GitHubPane..ctor() in C:\Users\donokuda\Source\Repos\VisualStudio\src\GitHub.VisualStudio\UI\GitHubPane.cs:line 55
   --- End of inner exception stack trace ---
   at System.RuntimeTypeHandle.CreateInstance(RuntimeType type, Boolean publicOnly, Boolean noCheck, Boolean& canBeCached, RuntimeMethodHandleInternal& ctor, Boolean& bNeedSecurityCheck)
   at System.RuntimeType.CreateInstanceSlow(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, StackCrawlMark& stackMark)
   at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, StackCrawlMark& stackMark)
   at System.Activator.CreateInstance(Type type, Boolean nonPublic)
   at System.Activator.CreateInstance(Type type)
   at Microsoft.VisualStudio.Shell.Package.InstantiateToolWindow(Type toolWindowType)
   at Microsoft.VisualStudio.Shell.Package.CreateToolWindow(Type toolWindowType, Int32 id, UInt32 flags)
   at Microsoft.VisualStudio.Shell.Package.CreateToolWindow(Type toolWindowType, Int32 id, ProvideToolWindowAttribute tool)
   at Microsoft.VisualStudio.Shell.Package.FindToolWindow(Type toolWindowType, Int32 id, Boolean create, ProvideToolWindowAttribute tool)
   at Microsoft.VisualStudio.Shell.Package.CreateToolWindow(Guid& toolWindowType, Int32 id)
   at Microsoft.VisualStudio.Shell.Package.Microsoft.VisualStudio.Shell.Interop.IVsToolWindowFactory.CreateToolWindow(Guid& toolWindowType, UInt32 id)
   at Microsoft.VisualStudio.Platform.WindowManagement.WindowFrame.ConstructContent()
@grokys grokys changed the title Highlight checked out PR in list. [WIP] Highlight checked out PR in list. Aug 8, 2017
@grokys
Copy link
Contributor Author

@grokys grokys commented Aug 8, 2017

@donokuda I think this is one of those things that needs to be fixed by cleaning the solution and resetting the experimental instance. However, we can do that later - we've punted this PR to the next release due to the caveats listed.

@shana shana force-pushed the master branch from 49ba6cf to a82bce9 Aug 16, 2017
@jcansdale jcansdale mentioned this pull request Dec 18, 2017
8 of 9 tasks complete
grokys added 2 commits Feb 14, 2018
 Conflicts:
	src/GitHub.App/ViewModels/GitHubPane/PullRequestListViewModel.cs
	src/GitHub.VisualStudio.UI/Styles/ThemeBlue.xaml
	src/GitHub.VisualStudio.UI/Styles/ThemeDark.xaml
	src/GitHub.VisualStudio.UI/Styles/ThemeLight.xaml
	src/GitHub.VisualStudio/Views/GitHubPane/PullRequestListItem.xaml
	test/UnitTests/GitHub.App/ViewModels/GitHubPane/PullRequestListViewModelTests.cs
@grokys
Copy link
Contributor Author

@grokys grokys commented Feb 14, 2018

@jcansdale @donokuda this should be ready for review again now, bearing in mind the gotcha in the "Not yet implemented" section of the PR description.

@grokys grokys changed the title [WIP] Highlight checked out PR in list. Highlight checked out PR in list. Feb 14, 2018
donokuda and others added 3 commits Feb 15, 2018
Tweak highlight colors
@shana
Copy link
Collaborator

@shana shana commented Feb 21, 2018

@grokys @donokuda Do you have updated screenshots on how this is looking now that you can post here?

@grokys
Copy link
Contributor Author

@grokys grokys commented Feb 21, 2018

Good point @shana - updated.

@StanleyGoldman StanleyGoldman self-requested a review Feb 21, 2018
Copy link
Contributor

@StanleyGoldman StanleyGoldman left a comment

I tested this and it works well from what I see.

the current PR may not be highlighted until you open its details

I didn't find that comment to be true. The pull request list view updates the highlighted value automatically.

(s, r) => new { Session = s, Repository = r })
.Subscribe(x =>
{
CheckedOutPullRequest = x.Session?.RepositoryOwner == x.Repository?.Owner ?

This comment has been minimized.

@StanleyGoldman

StanleyGoldman Feb 21, 2018
Contributor

Why are you only checking the repository owner here?

jcansdale added 2 commits Mar 5, 2018
Copy link
Collaborator

@jcansdale jcansdale left a comment

I've just merged the latest commits and checked that this is working. I haven't noticed the current PR not being highlighted until its details are opened. It was highlighted fine when I first viewed the PR list.

@jcansdale jcansdale merged commit 797d1b8 into master Mar 5, 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 feature/highlight-current-pr branch Mar 5, 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

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