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

Snapshot tracks only visible unresolved items #4980

Merged
merged 1 commit into from
Jul 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public static IDependenciesSnapshot Implement(

if (hasUnresolvedDependency.HasValue)
{
mock.Setup(x => x.HasUnresolvedDependency).Returns(hasUnresolvedDependency.Value);
mock.Setup(x => x.HasVisibleUnresolvedDependency).Returns(hasUnresolvedDependency.Value);
}

if (activeTarget != null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public static Mock<ITargetedDependenciesSnapshot> ImplementMock(

if (hasUnresolvedDependency.HasValue)
{
mock.Setup(x => x.HasUnresolvedDependency).Returns(hasUnresolvedDependency.Value);
mock.Setup(x => x.HasVisibleUnresolvedDependency).Returns(hasUnresolvedDependency.Value);
}

if (catalogs != null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public void Constructor()
Assert.Same(projectPath, snapshot.ProjectPath);
Assert.Same(targetFramework, snapshot.ActiveTargetFramework);
Assert.Same(dependenciesByTargetFramework, snapshot.DependenciesByTargetFramework);
Assert.False(snapshot.HasUnresolvedDependency);
Assert.False(snapshot.HasVisibleUnresolvedDependency);
Assert.Null(snapshot.FindDependency("foo"));
}

Expand All @@ -70,7 +70,7 @@ public void CreateEmpty()
Assert.Same(projectPath, snapshot.ProjectPath);
Assert.Same(TargetFramework.Empty, snapshot.ActiveTargetFramework);
Assert.Empty(snapshot.DependenciesByTargetFramework);
Assert.False(snapshot.HasUnresolvedDependency);
Assert.False(snapshot.HasVisibleUnresolvedDependency);
Assert.Null(snapshot.FindDependency("foo"));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,31 +50,31 @@ public void ToViewModel()
KnownMonikers.AboutBox,
KnownMonikers.Abbreviation);

var dependencyResolved = IDependencyFactory.FromJson(@"
{
""ProviderType"": ""Yyy"",
""Id"": ""tfm1\\yyy\\dependencyExisting"",
""Name"":""dependencyExisting"",
""Caption"":""DependencyExisting"",
""HasUnresolvedDependency"":""false"",
Copy link
Member Author

Choose a reason for hiding this comment

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

This change gets rid of HasUnresolvedDependency, which isn't even on IDependency. For this simple test type I prefer the strongly typed approach.

""SchemaName"":""MySchema"",
""SchemaItemType"":""MySchemaItemType"",
""Priority"":""1"",
""Resolved"":""true""
}", iconSet: iconSet);

var dependencyUnresolved = IDependencyFactory.FromJson(@"
{
""ProviderType"": ""Yyy"",
""Id"": ""tfm1\\yyy\\dependencyExisting"",
""Name"":""dependencyExisting"",
""Caption"":""DependencyExisting"",
""HasUnresolvedDependency"":""false"",
""SchemaName"":""MySchema"",
""SchemaItemType"":""MySchemaItemType"",
""Priority"":""1"",
""Resolved"":""false""
}", iconSet: iconSet);
var dependencyResolved = new TestDependency
{
ProviderType = "Yyy",
Id = "tfm1\\yyy\\dependencyExisting",
Name = "dependencyExisting",
Caption = "DependencyExisting",
SchemaName = "MySchema",
SchemaItemType = "MySchemaItemType",
Priority = 1,
Resolved = true,
IconSet = iconSet
};

var dependencyUnresolved = new TestDependency
{
ProviderType = "Yyy",
Id = "tfm1\\yyy\\dependencyExisting",
Name = "dependencyExisting",
Caption = "DependencyExisting",
SchemaName = "MySchema",
SchemaItemType = "MySchemaItemType",
Priority = 1,
Resolved = false,
IconSet = iconSet
};

var mockSnapshot = ITargetedDependenciesSnapshotFactory.ImplementMock(checkForUnresolvedDependencies: false).Object;
var mockSnapshotUnresolvedDependency = ITargetedDependenciesSnapshotFactory.ImplementMock(checkForUnresolvedDependencies: true).Object;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public void Constructor()
Assert.Same(projectPath, snapshot.ProjectPath);
Assert.Same(targetFramework, snapshot.TargetFramework);
Assert.Same(catalogs, snapshot.Catalogs);
Assert.False(snapshot.HasUnresolvedDependency);
Assert.False(snapshot.HasVisibleUnresolvedDependency);
Assert.Empty(snapshot.TopLevelDependencies);
Assert.Empty(snapshot.DependenciesWorld);
Assert.False(snapshot.CheckForUnresolvedDependencies("foo"));
Expand All @@ -60,7 +60,7 @@ public void CreateEmpty()
Assert.Same(projectPath, snapshot.ProjectPath);
Assert.Same(targetFramework, snapshot.TargetFramework);
Assert.Same(catalogs, snapshot.Catalogs);
Assert.False(snapshot.HasUnresolvedDependency);
Assert.False(snapshot.HasVisibleUnresolvedDependency);
Assert.Empty(snapshot.TopLevelDependencies);
Assert.Empty(snapshot.DependenciesWorld);
Assert.False(snapshot.CheckForUnresolvedDependencies("foo"));
Expand Down Expand Up @@ -176,7 +176,7 @@ public void FromChanges_AddingToEmpty()
Assert.NotSame(previousSnapshot, snapshot);
Assert.Same(updatedProjectPath, snapshot.ProjectPath);
Assert.Same(catalogs, snapshot.Catalogs);
Assert.True(snapshot.HasUnresolvedDependency);
Assert.True(snapshot.HasVisibleUnresolvedDependency);
AssertEx.CollectionLength(snapshot.DependenciesWorld, 2);
AssertEx.CollectionLength(snapshot.TopLevelDependencies, 1);
Assert.True(resolvedTop.Matches(snapshot.TopLevelDependencies.Single(), targetFramework));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ private void OnDependenciesSnapshotChanged(object sender, SnapshotChangedEventAr
{
dependenciesNode = await viewProvider.BuildTreeAsync(dependenciesNode, snapshot, cancellationToken);

await _treeTelemetryService.ObserveTreeUpdateCompletedAsync(snapshot.HasUnresolvedDependency);
await _treeTelemetryService.ObserveTreeUpdateCompletedAsync(snapshot.HasVisibleUnresolvedDependency);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this change will impact telemetry, in keeping with what's presented on screen.

}

// TODO We still are getting mismatched data sources and need to figure out better
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public async Task<IProjectTree> BuildTreeAsync(

dependenciesTree = CleanupOldNodes(dependenciesTree, currentTopLevelNodes);

ProjectImageMoniker rootIcon = _viewModelFactory.GetDependenciesRootIcon(snapshot.HasUnresolvedDependency).ToProjectSystemType();
ProjectImageMoniker rootIcon = _viewModelFactory.GetDependenciesRootIcon(snapshot.HasVisibleUnresolvedDependency).ToProjectSystemType();
Copy link
Member Author

Choose a reason for hiding this comment

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

This is where the icon is determined at the root level. This PR means only visible and unresolved descendents of the root can cause the root to have a yellow triangle.


return dependenciesTree.SetProperties(icon: rootIcon, expandedIcon: rootIcon);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public TargetDependencyViewModel(ITargetedDependenciesSnapshot snapshot)
{
Caption = snapshot.TargetFramework.FriendlyName;
Flags = DependencyTreeFlags.TargetNode.Add($"$TFM:{snapshot.TargetFramework.FullName}");
_hasUnresolvedDependency = snapshot.HasUnresolvedDependency;
_hasUnresolvedDependency = snapshot.HasVisibleUnresolvedDependency;
}

public string Caption { get; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ internal DependenciesSnapshot(
public ImmutableDictionary<ITargetFramework, ITargetedDependenciesSnapshot> DependenciesByTargetFramework { get; }

/// <inheritdoc />
public bool HasUnresolvedDependency => DependenciesByTargetFramework.Any(x => x.Value.HasUnresolvedDependency);
public bool HasVisibleUnresolvedDependency => DependenciesByTargetFramework.Any(x => x.Value.HasVisibleUnresolvedDependency);

/// <inheritdoc />
public IDependency? FindDependency(string dependencyId, bool topLevel = false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public override void BeforeAddOrUpdate(
{
ITargetedDependenciesSnapshot? snapshot = _aggregateSnapshotProvider.GetSnapshot(dependency);

if (snapshot != null && snapshot.HasUnresolvedDependency)
if (snapshot != null && snapshot.HasVisibleUnresolvedDependency)
{
context.Accept(dependency.ToUnresolved(ProjectReference.SchemaName));
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ internal interface IDependenciesSnapshot : IEquatable<IDependenciesSnapshot>

/// <summary>
/// Gets whether this snapshot contains at least one unresolved/broken dependency at any level
/// for any target framework.
/// for any target framework which is visible.
/// </summary>
bool HasUnresolvedDependency { get; }
bool HasVisibleUnresolvedDependency { get; }

/// <summary>
/// Finds dependency for given id across all target frameworks.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ internal interface ITargetedDependenciesSnapshot
ImmutableDictionary<string, IDependency> DependenciesWorld { get; }

/// <summary>
/// Specifies is this snapshot contains at least one unresolved/broken dependency at any level.
/// Specifies is this snapshot contains at least one unresolved/broken dependency at any level which is visible.
/// </summary>
bool HasUnresolvedDependency { get; }
bool HasVisibleUnresolvedDependency { get; }

/// <summary>
/// Efficient API for checking if a given dependency has an unresolved child dependency at any level.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ internal TargetedDependenciesSnapshot(
Catalogs = catalogs;
DependenciesWorld = dependenciesWorld;

bool hasUnresolvedDependency = false;
bool hasVisibleUnresolvedDependency = false;
ImmutableArray<IDependency>.Builder topLevelDependencies = ImmutableArray.CreateBuilder<IDependency>();

foreach ((string id, IDependency dependency) in dependenciesWorld)
Expand All @@ -183,9 +183,9 @@ internal TargetedDependenciesSnapshot(
string.Equals(id, dependency.Id),
"dependenciesWorld dictionary entry keys must match their value's ids.");

if (!dependency.Resolved)
if (!dependency.Resolved && dependency.Visible)
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the additional constraint.

{
hasUnresolvedDependency = true;
hasVisibleUnresolvedDependency = true;
}

if (dependency.TopLevel)
Expand All @@ -201,7 +201,7 @@ internal TargetedDependenciesSnapshot(
}
}

HasUnresolvedDependency = hasUnresolvedDependency;
HasVisibleUnresolvedDependency = hasVisibleUnresolvedDependency;
TopLevelDependencies = topLevelDependencies.ToImmutable();
}

Expand Down Expand Up @@ -230,7 +230,7 @@ internal TargetedDependenciesSnapshot(
private object SyncLock => _dependenciesChildrenMap;

/// <inheritdoc />
public bool HasUnresolvedDependency { get; }
public bool HasVisibleUnresolvedDependency { get; }

/// <inheritdoc />
public bool CheckForUnresolvedDependencies(IDependency dependency)
Expand Down