Fixing connection title updates operation in async server tree.

[aasimkhan30]

Issue: #23830

@smartguest can you please check if your connection title updates still happen correctly?

[Charles-Gagnon]

Why are we refreshing ALL titles anyways? Shouldn't those generally only change one at a time?

When we re-render will it keep any currently-expanded nodes open?

[aasimkhan30]

Yeah, re-render just affects the html div that is rendered for the node. It has no effect on the tree and the node expansion status.

I think ideally, we should only re-render the titles that have changed. However, that might be difficult to determine. I am guessing that's why all titles are refreshed. @smartguest, can you confirm?

[Charles-Gagnon]

Ah, ok that should be fine then. But @smartguest please follow up and see if we can improve this - ideally we should only be re-rendering a node after it's edited.

[smartguest]

Currently if one change is made to a connection profile or if a new one appears, it will affect the title generated by all other connection profiles, right now we currently do not check if the profile title was altered or not. I will need to look into how I can improve this (I think the current design was based on how vscode uses the tree titles, they need to be set via input first) . There are other issues with connection title that I am currently looking at that need to be addressed first before optimization can proceed. @aasimkhan30 @Charles-Gagnon

[Charles-Gagnon]

Ah, ok that should be fine then. But @smartguest please follow up and see if we can improve this - ideally we should only be re-rendering a node after it's edited.

[Charles-Gagnon]

I don't think this will get connections in sub-groups, can you verify?

[aasimkhan30]

Actually, that api fetches nested connections as well.

azuredatastudio/src/sql/platform/connection/common/connectionProfileGroup.ts

Lines 258 to 269 in b74919b

	public static getConnectionsInGroup(group: ConnectionProfileGroup): ConnectionProfile[] {
	let connections: ConnectionProfile[] = [];
	if (group && group.connections) {
	group.connections.forEach((con) => connections.push(con));
	}
	if (group && group.children) {
	group.children.forEach((subgroup) => {
	connections = connections.concat(this.getConnectionsInGroup(subgroup));
	});
	}
	return connections;
	}

Need to test first

[smartguest]

If you have two items with a distinguishing titles and delete one of them, the title doesn't return to the original form as it should do. This means refresh needs to happen upon deletion as well. Also, there is a test failure that needs to be addressed as well.

[aasimkhan30]

If you have two items with a distinguishing titles and delete one of them, the title doesn't return to the original form as it should do. This means refresh needs to happen upon deletion as well. Also, there is a test failure that needs to be addressed as well.

@smartguest, did it break because of my changes or it never worked in the first place?

[aasimkhan30]

@smartguest , just check deletion scenario. It does not update the title. However, I feel that should be fine.

For example:
I have 2 Connection:

Now, I delete the first one. It becomes.

However, based on the what @smartguest is suggesting, we still need to return the title to original form after deleting. Won't it make it confusing for the users? They might think they deleted the one with 300 timeout ?

@cheenamalhotra and @Charles-Gagnon what do you think?

[aasimkhan30]

@smartguest, also checked with today's insider. The behavior is consistent for deletion.

[cheenamalhotra]

Main reason to reset title is to not show advanced options unless there's a conflicting profile. However, it can cause confusions, and this is why I was thinking we could show all options on hover (html title attribute) - @smartguest we used to have that in design, but it went away, can that be done if we want to regenerate URI?

[Charles-Gagnon]

I agree with Aasim, it seems confusing and could cause some unnecessary panic if we're dynamically changing the title. I'm not sure hover is a good solution either - there's a lot of options so are you expecting to show all of them? That'll be pretty difficult to tell at a glance what the differences are - especially if there's only one or two properties changed.

[Charles-Gagnon]

@aasimkhan30 In your example above, if you restart ADS does the title reset at that point (so the one left no longer displays the additional connectTimeout value)?

[aasimkhan30]

@aasimkhan30 In your example above, if you restart ADS does the title reset at that point (so the one left no longer displays the additional connectTimeout value)?

Yes, restarting resets the value. @Charles-Gagnon

[Charles-Gagnon]

That's not great either IMO. This seems worth having a design discussion about at some point @cheenamalhotra @smartguest