Quick input reset scroll position

[jrieken]

• use latest from master
• f1 > insert snippet
• scroll down to an extension snippet and hide it (press 👁️ icon)
• 🐛 the scroll position resets

This is happening when reassigning the items (since the press changed the label) here:

vscode/src/vs/workbench/contrib/snippets/browser/insertSnippet.ts

Line 213 in 92314d6

picker.items = makeSnippetPicks();

[chrmarti]

Fixing this might be non-trivial because you set new objects and there is no way for the picker to tell if you intend this to be a new set of choices that require the scroll position to be reset or if this is a refinement of the existing choices that require the scroll position to stay the same.

[jrieken]

I could give you the same object again, e.g I would modify them them in-place and set the same array again

[chrmarti]

Two simpler options:

• Add read-writable scrollTop to the QuickPick object. You read it, update the items and set the old value again.
• Or: Add splice() to the QuickPick object and document that as preserving the scroll position by default.

If we were to add this to the API one day, I would prefer the second option.

[TylerLeonhardt]

I don't think the splice() would help in this case because @jrieken doesn't want to remove the item, he wants to change the item.

Add read-writable scrollTop to the QuickPick object. You read it, update the items and set the old value again.

This would work, though I'm not sure how to control scroll... any pointers on that?

With that being said, I almost feel like @jrieken wants a way to say "Hey I modified the objects in some way, please refresh them".

This can be done by turning this code into (note the if/else would be factored out as it is used in another place):

                picker.onDidTriggerItemButton(ctx => {
			const isEnabled = snippetService.isEnabled(ctx.item.snippet);
			snippetService.updateEnablement(ctx.item.snippet, !isEnabled);

			if (!isEnabled) {
				ctx.item.description = undefined;
				ctx.item.buttons = [{
					iconClass: Codicon.eyeClosed.classNames,
					tooltip: nls.localize('disableSnippet', 'Hide from IntelliSense')
				}];
			} else {
				ctx.item.description = nls.localize('isDisabled', "(hidden from IntelliSense)");
				ctx.item.buttons = [{
					iconClass: Codicon.eye.classNames,
					tooltip: nls.localize('enable.snippet', 'Show in IntelliSense')
				}];
			}

			picker.itemActivation = 0;
			picker.itemsUpdated = true;
			picker.update();
		});

The if/else just sets the buttons and description based on the new state. The real magic is the last 3 lines that basically force update how the items are rendered. This gives the desired effect of clicking on an 👁️ and having it change right in front of you.

picker.itemActivation = 0;
picker.itemsUpdated = true;
picker.update();

None of these are public atm, but I could see a possible refreshItems() function that does exactly those 3 lines.

Thoughts?

[chrmarti]

The assumption so far was that items would not change and that we need to read their properties only once when they are set on the picker object. That has kept the API simple. The scrollTop and splice() (replace the changed items) suggestions are preserving this assumption.

If we wanted to support mutable items, we would need an extension-supplied event. That would in my view mix two distinct approaches in the same API.

splice() might be a bit easier to use, but now that I think about this some more I think scrollTop will cover more cases and might be the preferable solution. (E.g., imagine changing the sort order and wanting control the scroll position or similar changes affecting all items.)

[TylerLeonhardt]

@chrmarti and I talked about this... one of my fundamental misunderstandings was that a .splice() would only remove things from a list when really the idea is that we'd pass a Map into .splice() that would map "nodes to be replaced" to "nodes to replace them with".

The assumption being that this API would not scroll to top as you are just replacing nodes in the list. If you want the 'scroll to top' behavior, you can set .items.

What do you think? Something like this:

export interface QuickPick<T extends QuickPickItem> extends QuickInput {
    // ...

    splice(replacementMap: Map<T, T>): void;

   // ...
}

[TylerLeonhardt]

after talking to @jrieken, he suggested we use an id property instead that we can use to do the comparison on whether or not the list has changed. The logic would be... if the list maintains the same order as before (using the ids to determine this) we will maintain the cursor position.

What do you think @chrmarti ?

[chrmarti]

@TylerLeonhardt Sounds good. I suggest to briefly look into the implementation before committing to it. We currently maintain object identity in the API, e.g., when the extension sets some items, it will get the same objects back as the active and selected item(s). When introducing ids, you would probably expect the new object to replace the old object with the same id in all of the API. Probably not a big task, but something to consider.

[TylerLeonhardt]

When introducing ids, you would probably expect the new object to replace the old object with the same id in all of the API. Probably not a big task, but something to consider.

This a good point... I guess the question is... how far should we go with this?

If you have 2 items with the same id should it:

• throw an error at set time? Explicit but potentially hugely breaking.
• Keep the last one? Makes it so the quick pick still works but potentially breaking if the user has 2 items with the same id today. Also extension authors need to know about this behavior change.
• Only use the id (or a new property like .focusId) for focus purposes and allow 2 items with the same id

I think the "focus algorithm" could simply be... "verify the order of the ids hasn't changed at all" which means something along these lines:

	private shouldKeepFocus(items: Array<T | IQuickPickSeparator>): boolean {
		// short circuit if their lengths are different
		if (items.length !== this._items.length) {
			return false;
		} else {
			return this._items.every((item, i) => {
				const otherItem = items[i];

				// verify separators are the same
				if (otherItem.type === 'separator'
					&& item.type === 'separator'
					&& otherItem.label === item.label
				) {
					return true;
				}

				// verify items have the same id (and that that id is not undefined)
				if (otherItem.type !== 'separator'
					&& item.type !== 'separator'
					&& otherItem.id === item.id
					&& otherItem.id
				) {
					return true;
				}

				return false;
			});
		}
	}

[chrmarti]

Even without duplicate ids in the same items array, ids raise some questions, e.g.:

• Set some items with unique ids and focus one of them.
• Reading activeItems will give you the same object you set on items.
• Update items with new objects, but the same ids (same order to keep things simple).
• Does reading activeItems now give you the new object or the old object? (I'd expect the new object. If you agree, this just needs to be considered when implementing.)

If items is set to an array with duplicate ids, maybe a warning could be logged and all ids could be ignored to keep things simple and non-breaking?

[TylerLeonhardt]

• Set some items with unique ids and focus one of them.
• Reading activeItems will give you the same object you set on items.

do you think you can point me to where activeItems gets set after I set items? I was trying to narrow it down but none of the references to activeItems really stood out.

Does reading activeItems now give you the new object or the old object? (I'd expect the new object. If you agree, this just needs to be considered when implementing.)

Yes I would also expect the new object. Good point.

If items is set to an array with duplicate ids, maybe a warning could be logged and all ids could be ignored to keep things simple and non-breaking?

Yes I like this.

[chrmarti]

This one picks up changes from the UI: https://github.com/microsoft/vscode/blob/main/src/vs/base/parts/quickinput/browser/quickInput.ts#L807

We trigger this initially by setting the focus items on the list: https://github.com/microsoft/vscode/blob/main/src/vs/base/parts/quickinput/browser/quickInput.ts#L956

[TylerLeonhardt]

Ah... so what I was doing is setting:

ItemActivation to NONE

vscode/src/vs/base/parts/quickinput/browser/quickInput.ts

Line 1019 in e003fd9

private valueSelectionUpdated = true;

but what this does is makes it so that when you reset .items .activeItems becomes and empty array.

This is the easiest solution for this issue... however... it may not be ideal.

If we want to keep activeItems to the last selection that gets trickier because when the items in the list widget get set, it appears to clear the focused state... we get around this by using ItemActivation.FIRST which triggers the first item in the quick pick to be focused...however that is a problem because even if we are able to focus on the "previously focused index", that's still going to cause the cursor to jump to that item which is what we see in this issue.

To truly fix this, that would mean we need some way to say "yes, focus on the last item that was focused but reset the scroll position back to what it was before. I just sent a PR out that does this.

[karrtikr]

We have a related use case in the Python extension where the scroll focus is lost when adding items dynamically to end of quickpick list #131457

[TylerLeonhardt]

related #76193

[chrmarti]

One way to avoid introducing ids would be to have separate functions for inserting, replacing and removing items:

• .insertItems(index, additionalItems)
• .replaceItems(index, replacementItems)
• .removeItems(index, count)

[TylerLeonhardt]

I like these suggestions @chrmarti and I think they're the only way to give folks the flexibility they're looking for all around:

• .items will always reset scroll position
• .insertItems() and the others will always preserve scroll position

This also makes me feel a bit better since it won't be potentially breaking user that already use the property id in their quickpick items.

I shall take this to API sync next week.

[karrtikr]

I have another suggestion similar to scrollTop option discussed earlier which I believe is simpler for the consumers to use, although I'm not sure if it can implemented easily:

• Introduce quickpick.assign(<new list>, doNotScroll = true), which directly replaces the current quickpick.items list without scrolling.
• As evident in the two use cases where we have this issue:
  • Quick input reset scroll position #109969 (comment)
  • Scroll focus is lost when adding items dynamically to end of quickpick list #131457

...It's likely more natural that the consumers have the new list instead of the diff. So if we were to use .insertItems()/.removeItems() etc., it's the job of the consumer to compare his new list with the old one, find the diffs, and then decide which API to call accordingly.

Even if the consumers have the diff, it might be a different kind of diff than what the API expects. It probably easier for the user to simply prepare the new list using whatever diff they have and assign it directly.

So I believe if this is just to avoid scrolling, quickpick.assign can be much simpler to use, thoughts?

[chrmarti]

@karrtikr Replacing the entire list and keeping the scroll position fixed only works well if what the user sees doesn't move. That's the case when appending items to the end or updating existing items (in a way that doesn't change their height). Other changes to the list could move the visible items and the extension has no way to find out which items are visible.

The extension needs to be careful about changes to the list to preserve the scroll position, so maybe that already implies that the extension is either aware of the delta and can apply it step-by-step or it is not aware of the delta and doesn't know how to preserve the scroll position.

Maybe .replaceItems(index, replacementItems) (alongside .insertItems() and .remoteItems()) could offer a second signature .replaceItems(index, deleteCount, replacementItems) to support what you suggest.

[TylerLeonhardt]

@chrmarti that second signature sounds very similar to the splice up above.

[karrtikr]

The extension needs to be careful about changes to the list to preserve the scroll position, so maybe that already implies that the extension is either aware of the delta and can apply it step-by-step or it is not aware of the delta and doesn't know how to preserve the scroll position.

@chrmarti Good point, I agree extension has to have some idea about the delta already.

That's the case when appending items to the end or updating existing items. The extension needs to be careful about changes to the list to preserve the scroll position

But I think this is true for both the API suggestions, and extension has to be careful about the scroll either way. What would be the advantage of editing the lists using delta rather than directly replacing it?

One thing I could think of is that having a separate .insertItems()/.removeItems() API, we could even preserve visible items when inserting/deleting items in the middle of the list, by adjusting the scroll position accordingly.

I like the second signature, but it seems having insertItems() API would be redundant in that case.

[chrmarti]

@chrmarti that second signature sounds very similar to the splice up above.

@TylerLeonhardt True and maybe a reason to avoid it initially. One difference is that the .splice() proposal meant to cover everything with a single function. With the 3 functions we can ensure and document the scroll behavior.

But I think this is true for both the API suggestions, and extension has to be careful about the scroll either way. What would be the advantage of editing the lists using delta rather than directly replacing it?

One thing I could think of is that having a separate .insertItems()/.removeItems() API, we could even preserve visible items when inserting/deleting items in the middle of the list, by adjusting the scroll position accordingly.

@karrtikr Indeed, having a general solution is the goal.

I like the second signature, but it seems having insertItems() API would be redundant in that case.

@karrtikr Not when it comes to preserving the scroll position. insertItems() would know that the items are new. Supporting deleteCount with .replaceItems() would introduce an ambiguity which is why I would suggest to not support it initially. When the scroll position is not important, extensions will continue to set .items and when it is important, they can use the 3 functions to apply the delta.

[TylerLeonhardt]

@jrieken proposed something different... a

stableItems: boolean

Property on quickpick. This is similar to ignoreFocusOut: boolean and it would specifically handle the behavior of what to do with the scroll position.

[TylerLeonhardt]

I have a PR out that adds a resetScrollPosition: boolean: #132013

[TylerLeonhardt]

verification: original issue has good steps.