implement a type hierarchy command #190
Conversation
|
I can't figure out why, but there seems to be quite a delay (over half a second?) between selected an item in the list and the cursor/selection moving. From the code, it doesn't seem like there are any requests to the AS or anything, but it's quite noticeable. Do you see it too? I don't have time now, but I'll try and do some debugging; I suspect it's a Code issue, whether there's a good reason for the delay I'm not sure. I'm testing with this file; cursor is on import 'dart:core';
main() {
var ints = new List<int>();
for (var i = 0; i < 4000; i++)
ints.add(i);
print('test');
}
class Danny {}
class Fred extends Danny {}
class Fred2 extends Fred {}
|
There was a problem hiding this comment.
Looks good! I've added a few comments (trying GitHub's new "Reviews".. not sure whether they're really any better than just comments).
Happy to land this as-is if you're happy with the comments above - if any of the comments warrant changes but aren't major we can move to issues for later.
| let options = { placeHolder: name(items, 0) }; | ||
|
|
||
| // TODO: How / where to show implements? | ||
| // TODO: How / where to show mixins? |
There was a problem hiding this comment.
You mentioned these are in the description; are these TODOs still valid, or do you think the current behaviour is as good as we can reasonably make and we should just remove these?
There was a problem hiding this comment.
Don't know; mixins we can probably ignore. W/ implements, people use that quite a bit in Dart to indicate inheritance. One example I was playing around with is dart:io's File and Directory; they both implement FileSystemEntity, but in this case I really think it's more of a subclass arrangement.
| let desc = ''; | ||
|
|
||
| // extends | ||
| if (item.superclass !== undefined && name(items, item.superclass) != 'Object') |
There was a problem hiding this comment.
Since we're hiding "extends Object" would it be better to completely hide Object from the top of the list? Feels a bit weird to include it in the list but not include it in the detail (My preference would be to drop it from the list, though if you think there's good reason to keep it, I'd prefer to drop this exclusion for the "extends.." text).
There was a problem hiding this comment.
Happy to drop - I don't think it adds a lot of value.
| desc += `extends ${name(items, item.superclass)}`; | ||
|
|
||
| // implements | ||
| if (item.interfaces.length > 0) { |
There was a problem hiding this comment.
I don't know if this is behaving as expected (but it may be the AS's fault)... Given this code:
class Danny {}
class Person {}
class Fred extends Danny implements Person {}
class Fred2 extends Fred {}- If I
F4onFred, in the list I seeFred extends Danny, implements Person. - If I
F4onDanny, in the list I seeFred extends Danny - If I
F4onPerson, in the list I seeFred extends Person
It seems strange to have three different representations of the same name in the list depending on what I executed the search on. What do you think?
There was a problem hiding this comment.
It's odd; the info returned from the AS really is customized based on the 'pivot' class chosen.
There was a problem hiding this comment.
Do you think it's worth raising as an issue? It seems a bit weird to get a different picture of the hierarchy depending on this?
| } | ||
|
|
||
| // TODO: Show the library location in the details (dart:core, ...) (share code | ||
| // with dart_workspace_symbol_provider.ts). |
There was a problem hiding this comment.
Do you think we could add this in a nice way? I think with only two labels (name, detail) it might get a bit noisy if we try to put both the extends/inherits/withs as well as the location. I think current implementation might be best we can do until (if) Code give us a custom tree option?
There was a problem hiding this comment.
I can remove the todo:. I'd assumed that the details wouldn't show expect for selected entries, but didn't confirm.
|
Hey - I can take a look at the comments this evening; I'll make some tweaks based on your feedback. Thanks! |
|
Updated! I do see the slowdown. Possibly a result of the implementation? I'm not sure where the time is going, but the impl is:
I didn't see a 'jump to location' API call, though there must be an internal implementation that the symbol nav. and type nav. use... |
|
I've opened #194 about the delay, will investigate when I have some time. |
This shows the direct supers and all children in a flat list - supers above and children below. The names if implements and mixins are show in the item descriptions but they are not represented as separate elements in the selection list.
@DanTup