-
Notifications
You must be signed in to change notification settings - Fork 31
Extend removeRefCommand to support call hierarchy items #61
Conversation
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
@jrieken Is this PR going to be merged for this release? |
Sorry this got lost... Will look into it |
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
src/extension.ts
Outdated
@@ -254,6 +254,21 @@ export function activate(context: vscode.ExtensionContext) { | |||
view.reveal(next, { select: true }); | |||
} | |||
} | |||
else if (model instanceof CallsModel) { |
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.
no new line
src/models.ts
Outdated
async getCallChildren(call: CallItem): Promise<CallItem[]> { | ||
if (call.children) | ||
return call.children; | ||
call.children = await this.resolveCalls(call); |
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.
Is resolveCalls
now only used from this method? Should it be private async _resolveCalls...
then or should they be merged into on?
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.
They can be merged into a single method, but that doesn't accommodate SRP. I've changed it to make resolveCalls
a private method.
src/provider.ts
Outdated
} | ||
|
||
getParent(element: CallHierarchyItem) { | ||
return element.parent; | ||
return element ? element.parent : undefined; |
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.
why? element should never be falsy
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.
Sorry. I didn't notice the parameter's type.
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
Signed-off-by: Babak Khalkhali Shandiz <[email protected]>
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.
lgtm
Thank you! |
This PR is following this issue on VS Code repo. In short, it was asked to also enable the "remove" button for call hierarchy trees to help users cancel out any items they've reviewed.
The problem with adding this feature was that the implementation of the call hierarchy tree, would resolve outgoing/incoming calls on demand (i.e., when the user clicked to expand a node) and then the children data were not taken hold of in the models.
To fulfill the request, a
children
property was added to theCallItem
model, andmove
,remove
and change events were added to theCallsModel
class. The originalremoveRefCommand
of the extension was updated accordingly.ReferencesModel._del
, it's been moved to the outer context and referenced from withinCallsModel
, too. Also, to be consistent,ReferencesModel._tail
was moved with the former.