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

Improve outline #5536

Merged
merged 10 commits into from
Feb 3, 2023
Merged

Conversation

trolleyman
Copy link
Contributor

@trolleyman trolleyman commented Jan 16, 2023

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

image

image

Outline Before Outline After
image image

Code

namespace OmnisharpTest
{
    namespace Foo
    {
        namespace Bar.Baz
        {
            public class BazClass
            {
                public class InnerBazClass
                {
                    public enum InnerEnum
                    {
                        A,
                        B,
                        C,
                    }

                    public interface IFoo
                    {
                        public void Method1(string many1, int many2, BazClass many3, InnerEnum many4, string many5);
                    }
                }

                public const string ConstantString = "ABCDEF";
                public static readonly OtherClass? OtherClassInstance = null;

                public string Field;
                public int Property { get; set; }

                public int this[int index] { get => 1; }

                public delegate int TestDelegate(string arg);

                public event TestDelegate? TestEvent;

                public BazClass(string field)
                {
                    Field = field;
                }

                ~BazClass()
                {
                }

                public void Method2(string argument1, int argument2, BazClass argument3, string argument4, string argument5)
                {
                    const string SomeConstant = "ABCDEF";

                    Console.WriteLine(SomeConstant);

                    void TestFunction(string testArg2)
                    {
                        ;
                    }
                    TestFunction("a");
                }
            }

            public class OtherClass
            {

            }

            public readonly struct WrappedInt
            {
                public static WrappedInt operator +(WrappedInt a) => a;
                public static WrappedInt operator -(WrappedInt a) => new WrappedInt(-a._inner);
                public static WrappedInt operator +(WrappedInt a, WrappedInt b) => new WrappedInt(a._inner + b._inner);
                public static WrappedInt operator -(WrappedInt a, WrappedInt b) => new WrappedInt(a._inner - b._inner);

                private readonly int _inner;

                public WrappedInt(int inner)
                {
                    _inner = inner;
                }

                public override string ToString() => _inner.ToString();
            }
        }
    }
}

namespace OmnisharpTest.Foo
{
    namespace OmnisharpTest.Foo
    {
        namespace Test123
        {

        }
    }
}

@trolleyman
Copy link
Contributor Author

@microsoft-github-policy-service agree

@trolleyman
Copy link
Contributor Author

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);
Copy link
Member

Choose a reason for hiding this comment

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

I think that in most cases the element.Name property should be the non-fully qualified name of the class/type/interace/etc.

e.g.
image

So I think we might not need to do any slicing here, instead just pass in the 'name' as the as the name and the displayName as the details

Copy link
Contributor Author

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.

Copy link
Member

@dibarbet dibarbet Feb 1, 2023

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:

  1. Limit the slicing to only happen for namespace case
  2. 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.

Copy link
Contributor Author

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

image

Name

image

Copy link
Member

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.

@dibarbet
Copy link
Member

dibarbet commented Feb 1, 2023

@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.

@dibarbet dibarbet merged commit 8c4a7ab into dotnet:master Feb 3, 2023
@dibarbet
Copy link
Member

dibarbet commented Feb 3, 2023

thanks for the contribution!

@trolleyman
Copy link
Contributor Author

@dibarbet Thanks for the review! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants