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;

   // ...
}