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.

Sign up for GitHub

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

Jump to bottom

Improve QuickInput/InputBox's APIs #5187

Merged
merged 2 commits into from
Aug 7, 2019
Merged

Improve QuickInput/InputBox's APIs #5187

merged 2 commits into from
Aug 7, 2019

Conversation

JPinkney
Copy link
Contributor

@JPinkney JPinkney commented May 19, 2019 •
edited
Loading

This PR brings a few enhancements to the QuickInput/InputBox API's. It implements steps, title, totalSteps, onDidTriggerButton, onDidHide, buttons, enabled, QuickInputButtons.Back.

Currently, I haven't found a way to set the progress indicator for the busy (progress indicator) portion of the QuickInput API. Also, ignoreFocusOut and prompt don't have a way to be set after the user has done inputBox.show().

2019-05-19-103803_826x219_scrot

This is based ontop of the changes made in this PR: #5108 and is related to #5109, eclipse-che/che#13007, eclipse-che/che#13227

Tested against: https://github.com/Microsoft/vscode-extension-samples/tree/master/quickinput-sample

@JPinkney JPinkney force-pushed the quickInput2 branch 2 times, most recently from 6f0e336 to 1827022 Compare May 19, 2019 14:50
@JPinkney
Copy link
Contributor Author

@benoitf and @evidolob Do you mind reviewing when you have time!

@akosyakov
Copy link
Member

@JPinkney does it resolve #5109 completely?

@JPinkney
Copy link
Contributor Author

@akosyakov It resolves everything but the busy part of the API (which shows progress indicator). I didn't find a way to implement that.

@akosyakov
Copy link
Member

@JPinkney will review it next month that it is landed in 0.8.0, please rebase and add a record for 0.8.0 in CHANGELOG

Also if you can assign your colleagues for testing that features' expectations are matching in VS Code and Theia, it would help with a review. maybe @vinokurig ?

@akosyakov
Copy link
Member

@elaihau @vince-fugnitto or maybe someone from Ericsson can test too

@vinokurig
Copy link
Contributor

@akosyakov I'll try it ASAP

@akosyakov
Copy link
Member

QuickOpenService and its implementation should not know anything about input service. Would it be possible to revert changes to them and move logic from MonacoQuickOpenServiceto QuickInputService? if access to a DOM node is needed it can be exposed via QuickOpenService.

@JPinkney
Copy link
Contributor Author

JPinkney commented Jun 4, 2019

@akosyakov I've moved all logic from MonacoQuickOpenService to QuickInputService as requested

@akosyakov
Copy link
Member

please rebase and have a look at comments

@JPinkney JPinkney mentioned this pull request Jun 18, 2019
Closed
@tsmaeder
Copy link
Contributor

@akosyakov do you have time to review this again? Wouldn't want it to go stale another time.

akosyakov
akosyakov reviewed Jul 2, 2019
View reviewed changes
packages/plugin-ext/src/main/node/plugin-resolution.ts Outdated
import { PluginCliContribution } from './plugin-cli-contribution';

@injectable()
export class PluginResolution implements PluginDeployer {
Copy link
Member

Choose a reason for hiding this comment

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

Does it belong to this PR or got here by a mistake?

@akosyakov
Copy link
Member

Design and code-wise looks good now. I am testing if everything is ok i will push an additional commit to delete unused code, adjust changelog, rebase and then merge it.

@tsmaeder
Copy link
Contributor

tsmaeder commented Jul 2, 2019

Design and code-wise looks good now. I am testing if everything is ok i will push an additional commit to delete unused code, adjust changelog, rebase and then merge it.

Awesome, thx. @JPinkney is on holiday this week, so this helps.

@akosyakov
Copy link
Member

akosyakov commented Jul 2, 2019 •
edited
Loading

  • onDidSelectItem does not work properly for showQuickPick, it should show a notification when another item is selected in the quick pick:
    onDidSelectItem

  • valueSelection is not respected for showInputBox, it should select a relevant part of a value:
    valueSelection

  • styles should be improved for the title bar, right now:

Screen Shot 2019-07-02 at 10 26 58

  • input is not validated for the multi step input, should be:
    multiinput_validation

  • resulted value is bogus from the multi step input, should be theia in the following screencast:
    bougs_result_multiinput

  • the progress is not reported properly (as already mentioned in the PR description)

  • missing title bar and buttons for the first step in the multi-step input, should be:
    Screen Shot 2019-07-02 at 10 35 41 When it is fixed one has to compare and verify behaviour of buttons in the first step with VS Code.

  • a cursor should be a pointer when one hovers over buttons in the multi-step input

  • it should be possible navigate and trigger buttons using keyboard, i.e. tab and enter

  • no feedback from the quick open, in VS Code it reports that the command failed if rg is missing
    Theia:

Screen Shot 2019-07-02 at 10 39 02

VS Code:
Screen Shot 2019-07-02 at 10 39 11

  • the multi input does not work nicely with the other quick panels, try to press F1 on some step:

Screen Shot 2019-07-02 at 10 53 08

@vinokurig
Copy link
Contributor

@akosyakov I've fixed all your comments, could you please review again

@vinokurig
Copy link
Contributor

fixes #5814

@akosyakov
Copy link
Member

@vinokurig the point was that it should no be breaking change, please read carefully here:

Client code should not be affected or changed in order to verify that it is done properly.

One should not copy code, but move only affected interfaces and reexport them.

@akosyakov
Copy link
Member

Please also make sure that git history does not contain merge and clean up commits for a code which was introduced in this PR. Please see: https://github.com/theia-ide/theia/blob/master/doc/pull-requests.md#checklist-commit-history

@vinokurig
Copy link
Contributor

@akosyakov

@vinokurig the point was that it should no be breaking change, please read carefully here:

Client code should not be affected or changed in order to verify that it is done properly.

One should not copy code, but move only affected interfaces and reexport them.

Done

Please also make sure that git history does not contain merge and clean up commits for a code which was introduced in this PR. Please see: https://github.com/theia-ide/theia/blob/master/doc/pull-requests.md#checklist-commit-history

I've squashed my commits but I don't want to modify the commit that was done by @JPinkney (1f2da76), so there is still one merge commit. Is it OK if @JPinkney or you will squash all the commits when the PR is ready to merge?

akosyakov
akosyakov reviewed Aug 5, 2019
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good clean up 👍

Is there a way to get rid of merge commit: 79edcba? GitHub tells me what it is yours, not of @JPinkney

i.e squash 5025b6a into it and then reword

I'm testing to check that styles are good now.

packages/core/src/browser/keybinding.ts Outdated Show resolved Hide resolved
packages/core/src/browser/quick-open/quick-title-bar.ts Outdated Show resolved Hide resolved
packages/core/src/browser/quick-open/quick-title-bar.ts Show resolved Hide resolved
packages/core/src/common/quick-pick-service.ts Show resolved Hide resolved
akosyakov
akosyakov reviewed Aug 5, 2019
View reviewed changes
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.

unfortunately title does not look aligned yet

@akosyakov
Copy link
Member

@vinokurig please rebase

@evidolob Would you be able to check after the rebase that imports are still good in the plugin system please?

@akosyakov
Copy link
Member

akosyakov commented Aug 6, 2019 •
edited
Loading

the progress is not reported properly (as already mentioned in the PR description)

@AlexTugarev We could implement it properly for the quick palette after your progress PR is merged? If so please file an issue for it. Right now experience is not smooth for multi steps input.

@JPinkney @vinokurig
Improve QuickInput/InputBox API's. Partially fixes: eclipse-theia#5109
412f832 
Co-authored-by: Josh Pinkney <joshpinkney@gmail.com>
Co-authored-by: Igor Vinokur <ivinokur@redhat.com>

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
@vinokurig
Copy link
Contributor

@akosyakov I've reworked styles and rebased the branch, could you please take a look.

akosyakov
akosyakov approved these changes Aug 6, 2019
View reviewed changes
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.

I've tested and it looks good to me now, please fix formatting and bogus import in the plugin system before merging.

packages/plugin-ext/src/plugin/type-converters.ts Outdated Show resolved Hide resolved
@vinokurig
QuickInput/InputBox API`s enhancements
502b24f 
Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
evidolob
evidolob approved these changes Aug 7, 2019
View reviewed changes
Copy link
Contributor

@evidolob evidolob left a comment

Choose a reason for hiding this comment

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

Works good, as for me

@vinokurig vinokurig merged commit d47d42e into eclipse-theia:master Aug 7, 2019
@vinokurig vinokurig mentioned this pull request Aug 8, 2019
Merged
1 task
@ira-gordin-sap
Copy link

Regarding "the progress is not reported properly (as already mentioned in the PR description)", I am not sure what happened with the progress?
Do you have an issue? Do you plan to fix?

@vinokurig vinokurig mentioned this pull request Sep 16, 2019
Open
@vinokurig
Copy link
Contributor

@ira-gordin-sap Filed an issue: #6195

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evidolob evidolob evidolob approved these changes

@akosyakov akosyakov akosyakov approved these changes

@benoitf benoitf Awaiting requested review from benoitf

@vinokurig vinokurig Awaiting requested review from vinokurig

Assignees
No one assigned
Labels
shell issues related to the core shell vscode issues related to VSCode compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@JPinkney @akosyakov @vinokurig @tsmaeder @tetchel @ShimonBenYair @ira-gordin-sap @evidolob