Change the header of inheritance margin#53964
Conversation
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
whoops. hnit approve too fast. i was looking at only a single commit.
src/VisualStudio/Core/Def/Implementation/InheritanceMargin/InheritanceMarginHelpers.cs
Outdated
Show resolved
Hide resolved
|
I like the idea, but the wording seems slightly off to me. Maybe "Implemented Interface/Type Members" ? |
src/EditorFeatures/Core/InheritanceMargin/InheritanceMarginServiceHelpers.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/InheritanceMargin/InheritanceMarginServiceHelpers.cs
Show resolved
Hide resolved
Can you clarify which portion you are referring to? |
|
@ryzngard Also tag @CyrusNajmabadi : ) |
| var overridingSymbols = GetOverridingSymbols(memberSymbol); | ||
| // For all implementing symbols, make sure it is in source. | ||
| // For example, if the user is viewing IEnumerable from metadata, | ||
| // then don't show the derived overriden & implemented types in System.Collections |
There was a problem hiding this comment.
was this just to limit size? otherwise, i can see this being nice functionality :)
There was a problem hiding this comment.
I'd leave them out for now, but considering adding them back once we can get filter buttons on the bottom of the list like completion has.
There was a problem hiding this comment.
makes sense to me. i def coudl see problems having only a combined list for metadata-symbols. some things would have a far too unweildy number of results to display.
| var overridingSymbols = allOverridingSymbols.WhereAsArray(symbol => symbol.Locations.Any(l => l.IsInSource)); | ||
|
|
||
| builder.AddIfNotNull(item); | ||
| if (overridingSymbols.Any() || overriddenSymbols.Any() || implementedSymbols.Any()) |
There was a problem hiding this comment.
if CreateInheritanceMemberItemForClassOrStructMemberAsync already can return a nul value, then consider moving this code into it.
| CreateInheritanceItemAsync(solution, | ||
| symbol, | ||
| InheritanceRelationship.ImplementingType, | ||
| cancellationToken), cancellationToken) |
There was a problem hiding this comment.
indentation is wonky here. also code is inconsistently wrapped with similar pattern above. i.e. CreateInheritanceItemAsync is not wrapped in teh first form, but is in the second.
src/EditorFeatures/Core/InheritanceMargin/InheritanceMarginServiceHelpers.cs
Outdated
Show resolved
Hide resolved
| CreateInheritanceItemAsync(solution, | ||
| symbol, | ||
| InheritanceRelationship.DerivedType, | ||
| cancellationToken), cancellationToken) |
There was a problem hiding this comment.
please indent children in from parents. otherwise, they look like siblings :)
There was a problem hiding this comment.
I think I might be confused by the inline parameter
The text makes me feel the parameters are already indented somehow. 🙁
FYI @akhera99 , this might not meet the bar of a bug, but just let you aware this problem.
There was a problem hiding this comment.
Interesting. Visually I still see it as not indented because the text starts on the same vertical space. I can see how you got confused though
Fix #53489
Change the title based on the suggestions by @sharwell
Example1:
Before:
After:
Example2:
Before:
After:
TOTO:
I might miss some comments and unnecessary code.