Skip to content
This repository has been archived by the owner on Mar 19, 2022. It is now read-only.

Fix symbol lists using tmPreferences files #4

Merged
merged 9 commits into from
Jan 14, 2020

Conversation

willstott101
Copy link

@willstott101 willstott101 commented Dec 6, 2019

Currently (as explained in #3) all function invocations/references show up as definitions in Sublime's index and goto tools.

This is a QDH which excludes function invocations from the Symbol table, which makes more features work correctly. The downside is, it completely removes the ability for sublime to find you these references (which used to come up as definitions) outside of good old Ctrl+F.

This patch manages which TypeScript scopes are included in the symbol lists/indexes using tmPreferences files. For the most part with simple files it seems to work correctly. More rigorous testing will definitely be required, please let me know if you find anything not working as expected.

I've been using the following file with the "Goto Symbol" functionality to quickly check what's considered a definition. Only actual definitions of functions and classes should show up in that list with my patch. Indexed references and symbols should also work as well as is expected by sublime atm.

SadlyNotADefinition is excluded as a definition because the tmLanguage scope names do not distinguish between SadlyNotADefinition and ParentClass.

function hello() {
    
}

function how(): NotADefinition {
    const NotADefinition varName = hello();
}

class Bye extends NotADefinition {
    public recurse: NotADefinition;

    what(arg: NotADefinition): NotADefinition {
        hello();
    }
}

(new Bye()).what();

class Generic<SadlyNotADefinition extends ParentClass = DefaultParentClass> {

}

@michaelblyons
Copy link

Can you also un-index entity.name.type.ts in favor of entity.name.type.class.ts? Sublime is indexing all of my type-hints. 😒

@willstott101
Copy link
Author

@michaelblyons Could you make an minimal file that has something in it that you don't want to have highlighted?

This file I'm proposing to add removes everything from the index where type declarations would be illegal, as I found that worked better than trying to remove some specific scopes and add others - which was fiddly and kinda depended on context anyway.

Honestly I forgot about types in most places. I've added the file I was testing with above, please add to it.

@michaelblyons
Copy link

export class Foo {
//           ^^^ This is important
    public bar: String;
//              ^^^^^^ This is not worth indexing
}

@willstott101
Copy link
Author

Turns out there is a meta.type.annotation we can ignore, that feels cleanest to me, because there are technically non-class types and we don't have to care about that distinction in this patch.

…nnot be distinguished given the scopes available in the tmLanguage.

Also flatten the list of type names, not sure why I was using source.ts in the first place.
@michaelblyons
Copy link

michaelblyons commented Dec 16, 2019

Oh, cool. I didn't realize your file was de-listing scopes from the indexing. That is indeed much nicer. It also gets method parameter type annotations. Thanks!

If you could also add meta.return.type.ts, that'd be awesome.

foo() : String {
//      ^^^^^^ For this
    return 'foo';
}

@willstott101
Copy link
Author

willstott101 commented Dec 16, 2019

Seems odd that the return types aren't considered type annotations, good catch.

I've also ignored Type Parameters in generics definitions, unfortunately there's no way to distinguish between the assigned name of a generic type and it's source type. But I think ignoring all is probably more intuitive.

class Generic<SadlyNotADefinition extends ParentClass = DefaultParentClass> {

}

EDIT: SadlyNotADefinition, ParentClass, DefaultParentClass all have exactly the same assigned scopes.

@willstott101
Copy link
Author

willstott101 commented Dec 16, 2019

OK, so it turns out that there is a showInIndexedReferenceList which chooses the scopes shown as References, not sure how I missed that in earlier trawling.

Unfortunately I don't completely understand the way these plist files interact (load order / precedence etc) so there will be some phaffing/reading to get the file completely right, but we ought to actually be able to come up with a complete solution using these tmPreferences files.

The default values for these settings - which we want to completely override are as follows.

# filename is: Symbol List.tmPreferences
name: Symbol List
scopes: [
    entity.name.function,
    entity.name.type,
    meta.toc-list
]
showInSymbolList: true
# no name but filename is: Indexed Symbol List.tmPreferences
scopes: [
    entity.name.function,
    entity.name.type
]
showInIndexedSymbolList: true
# no name but filename is: Indexed Reference List.tmPreferences
scopes: [
        meta.function-call variable.function,
        meta.function-call support.function,
        meta.method-call variable.function
]
showInIndexedReferenceList: true

Perhaps it's possible to set source.ts to unset all of these, and then conditionally reset them ourselves. But as I say IDK how multiple files interact or when they're loaded. I'll try and look into this further soon.

I've opened an issue about documenting this setting: guillermooo/sublime-undocs#271

@willstott101 willstott101 changed the title Exclude function calls from the symbol lists... [WIP] Fix symbol lists using tmPreferences files. Dec 16, 2019
@willstott101 willstott101 changed the title [WIP] Fix symbol lists using tmPreferences files. [WIP] Fix symbol lists using tmPreferences files Dec 16, 2019
…est files seems pretty reasonable like this. Will need extended testing on a proper project to be sure - or an automated tested solution.
@willstott101
Copy link
Author

willstott101 commented Dec 17, 2019

I'm pretty happy now with how this behaves on my tiny little test file. Will need extended testing on a proper project to be sure... or an automated testing tool ;)

@willstott101 willstott101 marked this pull request as ready for review January 8, 2020 11:35
@willstott101
Copy link
Author

Can you also un-index entity.name.type.ts in favor of entity.name.type.class.ts? Sublime is indexing all of my type-hints.

I initially wrote this off because of the entity.name.type.class.ts comment, however... I noticed that as clauses (myVariable as MyClass) only tag the type as entity.name.type.ts, so that's the only way to exclude it. Once I'd done that I realised this scope exists for all and only for all type annotations, and definitions always seem to be more specific (entity.name.type.class/interface.ts).

As a result this is way simpler now and I'm fairly confident we can merge it. It's without a doubt a million times better than the default from sublime anyway.

@willstott101 willstott101 changed the title [WIP] Fix symbol lists using tmPreferences files Fix symbol lists using tmPreferences files Jan 8, 2020
<string>0</string>
</dict>
</dict>
</plist>
Copy link
Owner

@braver braver Jan 8, 2020

Choose a reason for hiding this comment

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

  • Can't these two Reference List Customizations sit in the same file? (I dunno, just a question). Or at least give them a name that functionally differentiates them, other than simple numbering.
  • I think the file should be called Indexed Reference List.tmPreferences to be in line with other packages.
  • I kinda like having that newline at the end.

Copy link
Author

@willstott101 willstott101 Jan 13, 2020

Choose a reason for hiding this comment

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

  • I can't see any way to make them the same file in the plist format. They have to be loaded in a specific order which is why I used numbers, however I can re-name to Exclusions and Inclusions still keeping the order.
  • Ok re-named on my laptop.
  • Added newlines on my laptop.

Have done the changes in an airport without wifi and replying on my phone... so won't be till later that I can actually push.

@braver braver merged commit 075b52e into braver:master Jan 14, 2020
@michaelblyons
Copy link

Thank you both!

@braver
Copy link
Owner

braver commented Jan 14, 2020

😄🎉

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

Successfully merging this pull request may close these issues.

3 participants