-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Change the header of inheritance margin #53964
Change the header of inheritance margin #53964
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this just to limit size? otherwise, i can see this being nice functionality :)
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
builder.AddIfNotNull(item); | ||
if (overridingSymbols.Any() || overriddenSymbols.Any() || implementedSymbols.Any()) |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please indent children in from parents. otherwise, they look like siblings :)
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.