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

When sub or group is refreshed, refresh the children as well #339

Merged
merged 2 commits into from
Jul 21, 2022

Conversation

nturinski
Copy link
Member

Fixes #349

When "refresh" is triggered on the root or a parent, it will only call its refresh, and then call loadMoreChildren on itself. However, because loadMoreChildrenImpl doesn't actually create new tree items (since we are caching all of the AppResourceTreeItems), refreshing the root, the subscription, and the groups didn't actually do anything.

By adding refreshImpl to SubscriptionTreeItem and GroupTreeItemBase, the AppResourceTreeItems will actually get refreshed now and it'll be a meaningful refresh.

I can't fix the root level refresh without affecting performance because if I call refresh in SubscriptionTreeItem.loadMoreChildrenImpl, it will activate all of the extensions at once.

The root level refresh only works for subscription filtering so I would almost opt to get rid of the refresh button on the ribbon and add it to the groups themselves. It would looks like this:

image

@nturinski nturinski requested a review from a team as a code owner July 21, 2022 00:50
@bwateratmsft
Copy link
Contributor

@alexweininger didn't microsoft/vscode-azurestorage#294 do this?

@nturinski
Copy link
Member Author

nturinski commented Jul 21, 2022

Once it's resolved, I don't think that resolving it again will do anything. Since ResolvableTreeItemBase has a call to this.resolve in its refreshImpl, calling refresh on every tree item under that GroupTreeItemBase should also resolve it on top of the actually refreshing the tree item.

@alexweininger
Copy link
Member

didn't microsoft/vscode-azurestorage#294 do this?

The logic added in 294 only re-resolves if a new resolver has been registered that matches the resource

@nturinski nturinski merged commit ecc1866 into main Jul 21, 2022
@nturinski nturinski deleted the nat/refreshGroups branch July 21, 2022 20:38
@alexweininger
Copy link
Member

The root level refresh only works for subscription filtering so I would almost opt to get rid of the refresh button on the ribbon

I don't think I'm ready to get rid of the refresh button on the ribbon... but you make a good point here. It really doesn't do much currently

@nturinski
Copy link
Member Author

The root level refresh only works for subscription filtering so I would almost opt to get rid of the refresh button on the ribbon

I don't think I'm ready to get rid of the refresh button on the ribbon... but you make a good point here. It really doesn't do much currently

I think when we move to v2, we could probably fix it not refreshing anything so probably good to just leave it for now.

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.

Blob containers do not refresh in VS Code extension
3 participants