QuickPick: onDidHide not fired for subsequent hide when a show is called after a hide

[stevenguh]

Issue Type: Bug

Calling show before waiting on the onDidHide will cause onDidHide to not fire on the subsequent hide for QuickPick.

Steps to Reproduce:

1. Open a QuickPick using the following code

const qp = window.createQuickPick();
qp.ignoreFocusOut = true;

qp.onDidHide(() => console.log("onDidHide"));
qp.show();

qp.hide();
qp.show();

// Manually hide in the UI

2. Manually hide the QuickPick by pressing esc

RESULT:
You should see the following results in the console.

first show
second show
onDidHide

Only one onDidHide was triggered and no additional onDidHide is trigger after a manual hiding with ESC from the UI.

EXPECTED:
Expected there is an additional onDIdHide being triggered after manually hide the quick pick with ESC like the follow ing output.

first show
second show
onDidHide
onDidHide

The only workaround is to wait onDIdHide is triggered before calling show again.
For example:

const qp = window.createQuickPick();
const hide = () => new Promise<void>((r) => {
	qp.hide();
	const disposable = qp.onDidHide(() => {
		disposable.dispose();
		r();
	});
});
qp.ignoreFocusOut = true;
qp.onDidHide(() => console.log("onDidHide"));


console.log("first show");
qp.show();

await hide();
console.log("second show");
qp.show();

// Manually hide in the UI

The output of this workaround is as expected

first show
onDidHide
second show
onDidHide

VS Code version: Code 1.61.2 (6cba118, 2021-10-19T15:49:28.381Z)
OS version: Darwin x64 20.6.0
Restricted Mode: No

System Info

Item	Value
CPUs	Intel(R) Core(TM) i7-4870HQ CPU @ 2.50GHz (8 x 2500)
GPU Status	2d_canvas: enabled gpu_compositing: enabled metal: disabled_off multiple_raster_threads: enabled_on oop_rasterization: enabled opengl: enabled_on rasterization: enabled skia_renderer: disabled_off_ok video_decode: enabled webgl: enabled webgl2: enabled
Load (avg)	3, 4, 4
Memory (System)	16.00GB (0.65GB free)
Process Argv	--crash-reporter-id a7d32175-b55e-44b1-b233-59944ad1fc06
Screen Reader	no
VM	0%

[stevenguh]

Reopened after adding the context of the bug.

[TylerLeonhardt]

Yes this is a tricky one...it's a race condition... this comes from the fact that hide() has to make a round trip and we only block on half of it.

When you call hide() it sends a message from the Extension Host to the renderer of VS Code (which exists in another process) to say "hey I want you to hide me"... and then the UI is hidden and then the renderer emits an onDidHide event that gets sent back to the ExtensionHost to say it happened. Calling hide() doesn't wait for the ack that it happened... it only waits until the message was sent to the other side (renderer).

That code that sends a message is here:

vscode/src/vs/workbench/api/common/extHostQuickOpen.ts

Lines 399 to 400 in ab8955c

	this._visible = false;
	this.update({ visible: false });

The code that does handle the onDidHide that comes back is here:

vscode/src/vs/workbench/api/common/extHostQuickOpen.ts

Lines 422 to 425 in ab8955c

	if (this._expectingHide) {
	this._expectingHide = false;
	this._onDidHideEmitter.fire();
	}

The problem we're seeing is...

you call hide()
then you call show()
then onDidHide gets triggered from the first hide()
then you hide the quickpick manually which triggers a 2nd onDidHide

but that 2nd time onDidHide is triggered, _expectingHide is already false, so it doesn't actually gets triggered.

I'm not sure we can have hide() wait on the response because then hide() would need to become async which would be breaking... so I'm really not sure if there's anything we can do here that makes sense esp considering you have a decent workaround of awaiting for the onDidHide.

I am wondering... Why are you calling hide and show so close together?

[TylerLeonhardt]

@chrmarti not sure if you have any thoughts on this.

[stevenguh]

Thanks for the explanation of the current implementation.

I'm not sure we can have hide() wait on the response because then hide() would need to become async which would be breaking... so I'm really not sure if there's anything we can do here that makes sense esp considering you have a decent workaround of awaiting for the onDidHide.

I was imagining there's a queue internally to handle show and hide, so the hide and show will process sequentially internally. The end result I expect would be onDidHide will triggered the same amount of time that hide has been called.

If there was an internally queue to handle show and hide, I don't imagine it requires changes to the current API.

I am wondering... Why are you calling hide and show so close together?

Not the exact code of how I discover this bug, but something like this. I would like to hide the menu first before execute command (because some commands don't work when the quickpick is in focus).

async function doSomething() {
  if (hasCommand) {
    await executeCommand(command);
  }
}
quickpick.hide();
await doSomething();
quickpick.show();

// Then manually hide the quickpick in the uI

[TylerLeonhardt]

If there was an internally queue to handle show and hide, I don't imagine it requires changes to the current API.

This would require the API to be asnyc. Which we cannot change.

In any case, after talking to @chrmarti, he and I both think that _expectingHide should only be set to false so long as there isn't a show() in progress. Let me see how challenging that would be to add.

[stevenguh]

This would require the API to be asnyc. Which we cannot change

The show and hide will enqueue without waiting, so the API will still be synchronous.

---

Regardless, seems like you are able to fix it in another way. Thanks for looking into the issue.