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

plugin: include registration of view containers #8034

Merged
merged 1 commit into from
Jun 23, 2020
Merged

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Jun 17, 2020

What it does

Fixes: #8016

The following pull-request updates the plugin system to include
the registration of view containers in the open view prefix menu.

How to test

  1. add the gitlens extension
  2. close the gitlens view if it is visible
  3. trigger the open view prefix quick-open menu (opened by the view > open view menu)
  4. select gitlens, and verify that the view is opened correctly
  5. trigger the open view again selecting gitlens (the view should remain open (not toggle))

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added the plug-in system issues related to the plug-in system label Jun 17, 2020
@vince-fugnitto vince-fugnitto self-assigned this Jun 17, 2020
@vince-fugnitto vince-fugnitto requested a review from lmcbout June 17, 2020 15:54
Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

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

Works well
Thanks @vince-fugnitto

@akosyakov
Copy link
Member

I find it confusing without grouping. Maybe we can add group attribute to QuickOpenViewItem? and aligned to use values from VS Code?

@vince-fugnitto
Copy link
Member Author

I find it confusing without grouping.

@akosyakov I believe that master is also confusing, as we include items such as Line History which is contributed by Gitlens but there is no info in the open view.

That is why I initially proposed #8026, to try and group based on the actual containers and contributed views, but the step further would be to group based on where they are contributed.

For example, for views contributed to the explorer such as npm scripts, we group them all under explorer.

@akosyakov
Copy link
Member

akosyakov commented Jun 19, 2020

ok, let's do it in another PR, but take care that view containers are only activated, not toggled

Fixes: #8016

The following pull-request updates the plugin system to include
the registration of `view containers` in the `open view` prefix menu.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto
Copy link
Member Author

@lmcbout do you mind re-testing, I've updated the 'how to test' section following the recent changes.

Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

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

Works as expected
No toggling for the view when re-selectiong View -> open-view
The other views DO not TOGGLE either
Tested on Ubuntu with Chrome and FireFox

@vince-fugnitto
Copy link
Member Author

I'll merge tomorrow if there are no objections.

@vince-fugnitto vince-fugnitto merged commit 2bba7ec into master Jun 23, 2020
@vince-fugnitto vince-fugnitto deleted the vf/gh-8016 branch June 23, 2020 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vscode: plugin system incorrectly registers 'open views'
3 participants