Extend Extension API with QuickPickSeparator Support

[go2sh]

With #21244 QuickPick Controls got the support for separators. This functionality is currently not available through the extensions API. For a nice look and feel, it would be great to make the separators available for extensions.

It seams to be not to hard to implement and I'am willing to do it, but I'am not sure from an API standpoint. Would you accept such a change?

The API could be changed in to ways:

• Add a property to the QuickPickItem to show a seperator above, add this seperator in the main thread.
• Add a Interface QuickPickSeparator to the extension api and change all occurrences of items to (T | QuickPickSeparator)[] or ReadonlyArray<T | QuickPickSeparator>

[chrmarti]

Adding an interface similar to the internal one would make sense:

interface QuickPickSeparator {
	type: 'separator';
	label?: string;
}

PR welcome. When adding QuickPickSeparator to the existing API, we will need to make sure we don't break existing clients.

/fyi @jrieken

[jrieken]

PR welcome

Not sure, API via PRs are always hairy because API gets discussed a lot.

[go2sh]

@jrieken @chrmarti I create a draft for the API changes: go2sh@b49406c
Feel free to have a look and discuss it.

I'am also almost done with the changes for the extension host components.

[go2sh]

Okay I'am done. I found two things to consider:

There are some vscode internal extensions, which use the name type already for a different usage. So it might conflict and thus break some stuff. How do you handle this case?

The type of activeItems and selectedItems is hard. If it's just T code like in the tests:

pick.activeItems = [pick.items[0]];

will break. To avoid this, the type could be T | QuickPickSeperator and the value could be filtered on setting.

Maybe Using a flag for the separator might be less destructive.

[eamodio]

To me, adding a new property to the existing QuickPickItem is an easier way to go

Something like

export interface QuickPickItem {
	...
	beginGroup?: boolean;
}

or

export interface QuickPickItem {
	...
	beginGroup?: boolean | string;
}

If you want to be able to put a label on the separator

[jrieken]

👏 I like that much better @eamodio. Maybe with "begin" but just "group" or "groupInformation"

[chrmarti]

We had it as a property on the items in our internal API and it led to complicated code when using it because some otherwise random item suddenly also represents a group of items. That is why I introduced the QuickPickSeparator interface when doing the new implementation.

[eamodio]

The separator method, seems like it would be problematic with search in the quickpick (or do you just throw away all separators in that case?)

@jrieken with this:

export interface QuickPickItem {
	...
	group?: string;
}

Would you expect it would still behave as a beginning marker, or an actual grouping construct? While a grouping construct might be nice, it feels like it makes things harder on the QuickPick side if the items in a group aren't consecutive. Although completely out of scope here -- have a true grouping construct, like QuickPickGroup that also supported items (and ideally a promise of items), could be a great way of having async groups in the quickpick. For example, in some GitLens quick pick the commands are static, but other parts could be dynamically provided. But I'm getting out of scope for this 😄

[chrmarti]

Search rearranges the items according to how well they match, so items are no longer in the original groups. Because of that we remove the separators. (See, e.g., the theme picker.)

[Zingam]

Is this still not implemented? I work on a feature for the CMake extension, which would really benefit of having an item separator to separate the toolchain kits from an action to rescan the system for kits.

[alefragnani]

Some comments from my #11468 (closed yesterday as #duplicate in favor of this one), which I think are relevant to be present here

---

Original request

The idea is to show items in the pick list, just like Go to Symbol, where you have some kind of groups of items (that appears when you type '@:' in the pick list)?

---

More details added 2 days ago

I was thinking about a group/category data attached to each QuickPickItem and the QuickPick itself would deal with grouping and counting the items in each group (when properly turned on with a new QuickPickOptions option). At first sight, I thought the groups (line separator and labels/counting) would be generated automatically.

But, if the proposed API (QuickPickItem and QuickPickSeparator) could give me the same power/flexibility as we see today in Go to Symbol command, it would be great.

I just wonder how/if I should deal with the ordering, visibility and counting myself when the user filters the PickList, with the QuickPickSeparator. As you see, the counter changes, so should I "recreate/update" that QuickPickSeparator too?

I didn't dig in VS Code source to see how the internal API works today, but with the right documentation and samples, I guess it wouldn't be a problem.

---

Thank you

[eamodio]

How about something like this:

export interface QuickPick {
	...
	groups?: QuickPickItemGroup[];
}

export interface QuickPickItemGroup {
	id: string;
	label?: string;
	options?: { showCount?: boolean }; // probably don't need to support this in v1
}

export interface QuickPickItem {
	...
	group?: string;
}

Where you could specify an ordered array of groups, with optional labels, and then apply the group id to each item via the group property. If groups isn't used or a group doesn't exist, then it just falls back the to "default group" which would always be the last "group"

This somewhat follows our menu pattern (with the exception of the defined groups, since we want to apply labels/other visual indicators.

/cc @chrmarti @jrieken

[jrieken]

That seems very complicated. Given that a group is just a label, it could be just this

export interface QuickPickItem {
	group?: string; // or groupLabel, groupName, category, etc
}

[eamodio]

Well without specified groups there wouldn't be a controlled ordering (especially if the label is also it's identifier). Nor the ability to have label-less groups (just a separator really).

[jrieken]

I believe the order is always as it is defined inside the array and when we re-order we don't show groups. For groups without labels - I am not sure that's desirable.

[connor4312]

I like the groups property approach, but that doesn't allow arbitrary/unnamed separators. In theory I could have an empty string group, but that would be the only one of that group I could have.

[connor4312]

Another solution might be to introduce something like

export class QuickPickGroup<T extends QuickPickItem>  {
	constructor(children: ReadonlyArray<T>, name?: string);

	readonly children: ReadonlyArray<T>;

	readonly name?: string;
}

then modify the quickpick signatures like

export function showQuickPick<T extends QuickPickItem>(items: (T | QuickPickGroup<T>)[], options?: QuickPickOptions, token?: CancellationToken): Thenable<T | undefined>;

This has the advantage of being unambiguously type-safe while supporting separators/anonymous groups as well as named groups, and is also (I think) fairly easy to construct.

[eamodio]

I would be worried about a parent/child construct for the group/items, as it could get strange for searching/filtering. I am in favor of a simpler solution, like here: #74967 (comment)

export interface QuickPickItem {
	...
	beginGroup?: boolean | string;
}

Even though it doesn't cover all the bases, it gets us close enough IMO. I like the above solution, since it doesn't express that we actually support grouping, and instead just adds a single marker for where a separator should appear (and what its optional label should be). While using a group property, would suggest IMO suggest that things would move around (stay in groups), regardless of the order they were added in (which would also be an issue if you wanted more than 1 label-less group or more than 1 group with the same label). This way there is no change in how you add items into the quickpick and the ordering of them. This would play well with search/filtering (as it can easily be ignored).

[alefragnani]

I'm wondering how simply adding beginGroup would work while filtering. Should I recreate the items to say which one (still visible) does beginGroup on each filter?

I mean, the idea of groups is to combine/share a common attribute (group) between items. So, when you say "all symbols of type Property share the same "Property" group", no matter which one does beginGroup. When all Property items are listed, the QuickPick should automatically add a separator for the next group. It's exactly the behavior that I see today in Command Palette.

I see beginGroup as separators in menus/toolbars, when you want to split different static groups. This is not the case here.

The idea you reported 5 months ago (#74967 (comment)) are much more aligned to what I would like to see. A new QuickPickItemGroup which could be assigned to each QuickPickItem by its id. And QuickPick should deal with all sorting/counting/separators by itself, just like it already does on your internal API. Just one more thing. An icon/codicon to each group 😉

Thank you

[eamodio]

I was assuming groups would disappear when filtering -- which I thought was the case for current internal usages, but I see now that "groups" are preserved when filtering. The internal api uses the IQuickPickSeparator approach that @chrmarti suggested, so I don't think there still really isn't a true notion of a group, rather its more that separators don't get filtered out unless they have a separator or nothing after them.

At the same time, I do still like my other proposal (#74967 (comment)) that you mentioned, but I feel now that it would change the quickpick api too drastically, since ideally we'd need to truly support groups, rather than just simulating them via separators. For example, if I have 3 items to a quick pick and the 1st and the 3rd have the same group, I would expect that they be put together and the 2nd item would be after them.

I feel the beginGroup proposal aligns with the current quick pick behavior, and could be implemented using IQuickPickSeparator, where the beginGroup flag would just be sugar to the internal creation of a IQuickPickSeparator.

[alefragnani]

For example, if I have 3 items to a quick pick and the 1st and the 3rd have the same group, I would expect that they be put together and the 2nd item would be after them.

Only if grouping is ON, like when you open Go to Symbols and type :. Otherwise they would be could be separated, if are not alphabetically in sequence.

In the end, as I said in my previous issue, if the proposed QuickPickSeparator gives me the same power/flexibility as we see today in Go to Symbol command (which appears it does, as you said it is more aligned to the internal API), it would be great. I was just looking for and easier solution 😁.

Thank you

[chrmarti]

To clarify: The default sorting removes the groups when the user starts filtering. There is a request (and proposed API) to disable the default sorting and leave that to the extension. (Internally we have the flexibility to keep the groups with a custom sorting.)

We had the beginGroup flag / label in an older implementation internally and the code using it could be more complex because it had deal with the asymmetry of items vs. 'items starting a group'. An item starting a group wasn't special except for that flag, e.g., you couldn't just reuse items without also making sure there were no old group markers on the wrong items.

I felt using separators as items in the list resulted in simpler code using the internal API than the beginGroup flag did. I preferred that to the group objects having items as children because it seemed simpler.

[TylerLeonhardt]

Given @chrmarti's last response, it sounds like we have first hand experience that an QuickPickSeparator interface was easier to work with for the end user. Given that, when I have time to work on this, I'd like to start there. It's also the simplest implementation as it's just exposing existing parts.

[TylerLeonhardt]

So with the discussion in API sync, I'm proposing:

	export interface QuickPickItem {
		kind?: string | { label: string; };
	}

• The kind aligns nicely with our other APIs.
• (not sure if needed right now...) The extra { label: string; } allows for future proofing of separators if we ever introduce like (color, a click listener, etc)
• No breaking changes needed whatsoever

an example:

const a = await window.showQuickPick([
		{
			label: 'asdf',
			kind: 'asdf',

		},
		{
			label: 'asdf',
			kind: 'asdf'
		},
		{
			label: 'asdf',
			kind: 'asdf'
		},
		{
			label: 'asdf',
			kind: 'asdf'
		},
		{
			label: 'zxcv',
			kind: 'zxcv'
		},
		{
			label: 'zxcv',
			kind: 'zxcv'
		},
		{
			label: 'zxcv',
			kind: 'zxcv'
		},
		{
			label: 'star',
			kind: 'star'
		},
		{
			label: 'idk',
		},
	]);

[TylerLeonhardt]

My conclusion is that this is very easy to use for the extension author and I was able to add these separators to existing quick picks with little effort.

I like the groups property approach, but that doesn't allow arbitrary/unnamed separators. In theory I could have an empty string group, but that would be the only one of that group I could have.

I missed this comment before and it brings up a good point... how would we add an arbitrary separator between two groups?

If we still encapsulate this in within kind I wonder if we could leverage the extra { label?: string } bag somehow to say "hey this is the start"

[TylerLeonhardt]

So in standup @connor4312 and @Tyriar convinced me that kind is harder to use than a separator...

@connor4312 pointed out that the kind proposal for quickpick separators isn’t ideal because it would not be possible to have a single blank line separating two groups.

and @Tyriar pointed out that kind is ambiguous if you have a situation like this:

kind: ‘a’
kind: ‘b’
kind: ‘a’

we discussed this in the API sync as ‘doc-able’ but I do share the same feeling as he.

Further discussion with @jrieken left us with:

enum QuickPickItemKind {
  Default, 
  Separator
}

interface QuickPickItem {
  // when set to Separator the following properties are ignored....
  kind?: QuickPickItemKind
}

So that we didn't need a separate type just for separators and so we didn't use string enums or literal or types.

However, we can't go down that route because QuickPickItem#label can't be undefined... where in separators in can...

I also wanted to call out that @chrmarti mentioned a tree-like structure... which might have looked similar to what @connor4312 was suggesting here:
#74967 (comment)

[TylerLeonhardt]

However, we can't go down that route because QuickPickItem#label can't be undefined... where in separators in can...

Going down this route... where QuickPickItem#label can just be empty string

[TylerLeonhardt]

Folks were happy in the API call so let's see how people feel about the API.

[TylerLeonhardt]

Please take a look at the example in #137745 and don't be afraid to provide feedback here. Moving this to December to track finalization.

[TylerLeonhardt]

Finalized in e15397d and eb416b4