Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix: [Plugin-API] complete QuickPick hide api #5766

Merged
merged 2 commits into from
Jul 26, 2019

Conversation

hacke2
Copy link
Contributor

@hacke2 hacke2 commented Jul 22, 2019

Fixes #5765

What it does

See https://code.visualstudio.com/api/references/vscode-api#QuickPick.

Hides this input UI. This will also fire an QuickInput.onDidHide event.

when execute hide method, vscode will:

  • Hide UI
  • Emit onDidHide event

But theia only emit onDidHide event.

How to test

import * as vscode from 'vscode';

export function activate(context: vscode.ExtensionContext) {
  const disposable = vscode.commands.registerCommand('hide QuickPick after 2000ms', () => {
    const quickOpen = vscode.window.createQuickPick();

    quickOpen.items = [
      { label: 'A' }, {label: 'B'}
    ];

    quickOpen.show();

    setTimeout(() => {
       quickOpen.hide();
     }, 2000);
  });

  context.subscriptions.push(disposable);
}

Review checklist

  • as an author, I have thoroughly tested my changes and carefully reviewed following the review guidelines

Reminder for reviewers

@hacke2 hacke2 requested review from benoitf and evidolob as code owners July 22, 2019 06:08
@hacke2 hacke2 force-pushed the fix/plugin-ext-quickopen-hide branch from ac72183 to bc3e5a8 Compare July 22, 2019 06:09
@hacke2 hacke2 closed this Jul 22, 2019
@hacke2 hacke2 deleted the fix/plugin-ext-quickopen-hide branch July 22, 2019 06:25
@hacke2 hacke2 restored the fix/plugin-ext-quickopen-hide branch July 22, 2019 06:25
@hacke2 hacke2 reopened this Jul 22, 2019
@akosyakov
Copy link
Member

@akosyakov akosyakov added the vscode issues related to VSCode compatibility label Jul 22, 2019
@hacke2
Copy link
Contributor Author

hacke2 commented Jul 22, 2019

@akosyakov I don't know how to test plugin in theia but I'm sure it's effective in monaco 0.14.

@kittaakos
Copy link
Contributor

how to test plugin in theia

@hacke2, here is a Wiki page describing how to test it: https://github.com/theia-ide/theia/wiki/Testing-VS-Code-Extensions
If you have any feedback on the docs, let us know. I'll make the adjustments.

@hacke2
Copy link
Contributor Author

hacke2 commented Jul 22, 2019

@akosyakov @kittaakos I've tested it. No problem.

What should I do next?

@akosyakov
Copy link
Member

@hacke2 wait, someone has to verify it independently

@kittaakos
Copy link
Contributor

@hacke2, please squash your commits, and I do the review. Thanks!

@hacke2 hacke2 force-pushed the fix/plugin-ext-quickopen-hide branch from 7bc6b57 to ef47a35 Compare July 23, 2019 07:10
@hacke2
Copy link
Contributor Author

hacke2 commented Jul 23, 2019

@kittaakos Done.

@hacke2
Copy link
Contributor Author

hacke2 commented Jul 23, 2019

hide

Add a Gif

@kittaakos
Copy link
Contributor

@hacke2, can you share the package.json of your extension? I am trying to follow the steps defined here to verify your changes. Thanks!

@kittaakos
Copy link
Contributor

can you share the package.json of your extension?

Never mind. I added your example: kittaakos/vscode-extension-samples@7f1230c

@kittaakos
Copy link
Contributor

I have verified it. Without the changes, the Command Palette does not close automatically after the 2000 ms timeout, it works with the changes. 👍

I am OK with the Theia related modifications, @benoitf are you OK with the plugin API?

@JPinkney
Copy link
Contributor

If its possible can we try to get this merged asap, I'm currently working on fixes for #5187. In order to fix up the comments I have to go back and re-do the way the createQuickPick API works and it would be easier to build ontop of this if its expected to go through

@kittaakos
Copy link
Contributor

@JPinkney, sure. Please find someone who approves the plugin API, and we can merge.

akosyakov
akosyakov previously approved these changes Jul 23, 2019
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

please take care of a comment before merging

@akosyakov akosyakov dismissed their stale review July 23, 2019 21:42

structural mismatch

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

@hacke2 thank you for the contribution

code-wise looks good, merging since changes were already tested

@akosyakov akosyakov merged commit 2befdf4 into eclipse-theia:master Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vscode] window.createQuickPick hide is incomplete
5 participants