-
Notifications
You must be signed in to change notification settings - Fork 680
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
Improve outline #5536
Improve outline #5536
Conversation
@microsoft-github-policy-service agree |
Not sure how to get this reviewed - @dibarbet? |
if (!element.DisplayName.startsWith(prefix)) { | ||
return { name: element.DisplayName, details: '' }; | ||
} | ||
const name = element.DisplayName.slice(prefix.length); |
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.
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.
Thanks for the review, nw about the delay :)
The problem is with namespace elements - without the slice the namespace Foo.Bar.Baz
(without any parents) will resolve as Baz
, which causes some confusion with the breadcrumbs, making it seem like the namespace is just Baz.
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 see what you mean, looks like the server just populates the name with the last identifier.
Can we do this:
- Limit the slicing to only happen for namespace case
- For the rest, use the name and display name as-is for name and details
We could change the server side to fix this, but seems like overkill for such a small change, so I'd rather not.
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.
Hey - I've done that, with a slight tweak - I've used DisplayName
for the name for some types, and Name
for others, so that more information is given - for example, below each function's Name
is just OverloadMethod
, which is a bit worse IMO in the outliner on small sizes (and gives less information in the breadcrumb). Having it be the DisplayName
is how it is currently. It's up to you though, if you think the Name
version is better I can change it to that.
DisplayName
Name
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.
Ah absolutely, using DisplayName there looks better - good call.
@trolleyman apologies for the delay - if you are ever waiting for a reply for more than a day or two, please ping me directly, sometimes I miss the original notification emails. |
…leyman/omnisharp-vscode into improve-outline-breadcrumbs
thanks for the contribution! |
@dibarbet Thanks for the review! :) |
This abbreviates the names of namespaces, delegates, structs, enums, classes and interfaces when in the outliner. This especially helps the repetitiveness of the breadcrumbs at the top of the editor, making it much easier to use (as it uses up much less screen space).
Breadcrumbs before & after
Code