Improve information shown in workspace search results

[devoncarew]

I think this is a result of merging two API call results?

[DanTup]

Strange; I was sure I tested this and it didn't happen. I just tried now searching for a top-level class but still don't see it. Could you confirm what your Dart file looks like?

[DanTup]

I wonder if something weird is happening with the results, maybe #63 is related. Are you able to capture the service output when this happens? (this might be tricky until there's an option to log to file)

[DanTup]

@devoncarew I'm gonna do a little more testing and then do a build for 0.7. I've pushed some minor things back to 0.8 as I'd like to release the other stuff; this the only remaining case. If you can repro this or think it's potentially an issue, if you let me know before I build, I'll revert that change until 0.8.

[DanTup]

I went ahead and pushed 0.7. If we can repro this I can always push an update :)

[devoncarew]

Cool; I can re-test and try and narrow down the issue this evening.

[devoncarew]

Hmm; I had two foo methods, and two FooBar classes in that sample project, in separate libraries :( User error -

[devoncarew]

If we change the containerName in the workspace search results to be the library path, we get this:

which could help users ID where the symbols are coming from.

[DanTup]

Seems like a good change to me! 👍

[DanTup]

That said; are we then losing the container name; eg:

/// test.dart

class Foo {
    string name;
}

class Bar {
    string name;
}

Would these both appear as name (test.dart)? It's less of an issue in the document one because of the highlighting, but maybe worth doing something here. We could show Bar.name but I guess with nested classes etc., the issue might continue.

I think the service returns a full path of parents; maybe containerName could be all parents up the tree?

[devoncarew]

We may or may not have enough info to do this, but I think:

• always showing the file path for containerName would be consistent
• for top-level symbols, we could just display the symbol name
• for class members, we could display Foo.bar

[DanTup]

Seems reasonable, though there's also nested functions and classes; though I think always combining them (assuming we have the info) would work (eg. Foo.Bar.function.subFunction)?

[devoncarew]

👍, makes sense

[DanTup]

Apparently Dart doesn't have nested classes (https://github.com/dart-lang/sdk/issues/3755)! I'll try and sort the other stuff though.

[DanTup]

@devoncarew What do you think about this?

private convertResult(result: as.SearchResult): SymbolInformation {
    let displayName = result.path
        .slice(0, -2) // Drop last two items (filename + library).
        .reverse() // Reverse so that parents come at start.
        .map(p => p.name + (p.parameters || "")) // Extract names and add params onto the end.
        .join("."); // Combine into a dotted string.

    return {
        name: displayName,
        kind: getSymbolKindForElementKind(result.path[0].kind),
        location: {
            uri: Uri.file(result.location.file),
            range: toRange(result.location)
        },
        containerName: null // Code displays filename if we don't set this.
    };
}

[DanTup]

Ugh... This looked wonky for imported packages (showed the full pub path as the location). I made a change so that it always displays the name of the last item in the paths array (this means it'll be library name if there is one, else the filename) which sounded logically sound, but it seems that Code strips everything before the last dot (so it just says dart) even though when you pass null it displays the filename (intact)! :(

[DanTup]

Raised microsoft/vscode#10504 about this behaviour. I'd like to go with using the name of the last item, but this Code behaviour makes it useless.

The options currently are:

1. Let Code display the full file path
2. Provide our own string, but if it contains dots we only get the stuff after the last dot

Neither of these is good; so I'm gonna park this pending a response.

[devoncarew]

Hmm, seems odd that we could display file paths w/ dots earlier, but not in this latest iteration.

[DanTup]

I believe all the paths we've ever displayed were by passing null as containerName and letting Code do them (which are relative if it's in the project and absolute if not). Though if you think this might not be the case, I would be chuffed if I've just done something wrong!

[DanTup]

It's insanely dirty (and may fail based on the font), but replacing the dots with something that looks a bit like a dot kind works:

a768869

Not sure I'm comfortable with that though 😞

[DanTup]

@devoncarew I wasn't happy with this, so I've gone back and forth making lots of changes... I settled on trying to make it as similar to the document symbol list as possible (show name and parent, rather than class.member etc.). I had to do some gymnastics to make filenames display nicely (discussion here).

If you get a min, can you take a look (it's in master) and see if it seems to behave and display reasonably?

[devoncarew]

I'll take a look; from a brief glance, I see the long paths in the display. It might be nice to display package refs as package:foo/foo.dart? And - going forward - we can't rely on having a packages/ top level dir in each project. Dart is moving away from the use of symlinks. In any case, happy to take a look in a bit.

[devoncarew]

That issue w/ truncating everything before the last dot is really annoying :)

[devoncarew]

FYI, I don't see the truncating issue in the latest VSCode Insiders build.

[devoncarew]

Hmm, disregard the above.

[DanTup]

from a brief glance, I see the long paths in the display

Hmm, could you give an example? Or maybe breakpoint/log in the replacePaths function and see why it isn't matching? (it does only work for hosted/pub.dartlang.org currently, so if your packages aren't in a folder like that, could be why?)

It might be nice to display package refs as package:foo/foo.dart?

Yeah, I really wanted to do that but thought I didn't have the required info; but now I'm wondering if what I've already done handles that... Maybe I'll take a stab at this tomorrow evening (though you're free to have a stab if you wish).

going forward - we can't rely on having a packages/ top level dir in each project. Dart is moving away from the use of symlinks

I figured with the symlinks gone, /packages would end up being real copies (like NuGet).. is that not the case? Will pubcache be the only actual location? (How will this work with Dartium?)

That issue w/ truncating everything before the last dot is really annoying :)

Tell me about it! :D I spent so long today just going back and forth trying to make this work.. But, where were you seeing it - the code I pushed shouldn't show that anymore (or were you tweaking the code?)

FYI, I don't see the truncating issue in the latest VSCode Insiders build.

Interesting! Maybe I should install it. I didn't realise it worked side-by-side, so probably makes sense. I don't mind shipping known issues like the filenames being wonky if it'll fix itself in the next Code update!

Oh, just saw your comment on the MS case; does still happen :(

[DanTup]

The crazy behaviour with stripping stuff has gone, so should indeed be fixed in Insiders (that's nightly?). We should just assume veryone will soon update, so if it shows something weird in the meantime, so be it!

[DanTup]

We should tweak this to not rely on hosted/pub.dartlang.org based on Brian's comments here.

[DanTup]

Moved last comment to #95; I think it's an edge case.