Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Avoid disposal of QuickInputExt on hide #13485

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

tortmayr
Copy link
Contributor

What it does

Remove implicit dispose() in QuickInputExt.hide(). This prevented quick input/quick pick instances from being shown again after they were hidden. This aligns the behavior with VS Code.
Note that there is still another issue related to this:
The following code sequence will open an empty quickpick overlay:

const quickPick = vscode.window.createQuickPick();
quickPick.items = [{ label: 'A' }, { label: 'B' }];
quickPick.show();
quickPick.hide();
quickPick.show();

This issue is also present in VS Code and is caused by the underlying monaco implementation.
If we also want to fix this we would probably have to directly change this in monaco.
There exists a workaround: Setting the items again before showing results in a correct quick pick overlay.

const quickPick = vscode.window.createQuickPick();
quickPick.items = [{ label: 'A' }, { label: 'B' }];
quickPick.show();
quickPick.hide();
quickPick.items = [{ label: 'A' }, { label: 'B' }];
quickPick.show();

Fixes #13072

Contributed on behalf of STMicroelectronics

How to test

You can use the following plugin (vsix+sourcecode) to test this:
theia13072.zip

This plugin contributed 3 commands

  • Test quickpick (hide,show): Creates a quick pick, hides it, then shows it again
  • Test quickpick (show,hide,show): Creates a quick pick,shows it, hides it, then shows it again. This is the error case described above that results in an empty quickpick overlay
  • Test quickpick (show,hide,show): Same as the show-hide-show command but it also incorporate the workaround (setting items again before showing). This should result in a correct quickpick overlay.

Here is also a quick demo how the commands behave on master and the PR branch:
Master:

quickpick-master.mp4

PR:

quickpick-fix.mp4

Follow-ups

Review checklist

Reminder for reviewers

Remove implicit `dispose()` in `QuickInputExt.hide()`.
This prevented quick input/quick pick instances from being shown again after they were hidden.
This aligns the behavior with VS Code.

Fixes eclipse-theia#13072

Contributed on behalf of STMicroelectronics
@JonasHelming JonasHelming requested a review from jonah-iden March 14, 2024 22:35
@tsmaeder tsmaeder self-requested a review March 21, 2024 14:43
Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

@tortmayr tortmayr merged commit 3cf66f9 into eclipse-theia:master Mar 21, 2024
14 checks passed
@jfaltermeier jfaltermeier added this to the 1.48.0 milestone Mar 28, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Cannot show QuickPick again after hiding it
3 participants