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

Allow customization of the viewlets in the default Extensions view #47766

Merged
merged 6 commits into from Jun 12, 2018

Conversation

Projects
None yet
4 participants
@ryenus
Contributor

ryenus commented Apr 12, 2018

When there're lots of extensions installed, and many of them disabled, it's kinda messy to have all the enabled/disabled extensions mixed in one list.

It's more clean to separate the disabled extensions into a dedicated list view:

BeforeAfter
@msftclas

This comment has been minimized.

msftclas commented Apr 12, 2018

CLA assistant check
All CLA requirements met.

@ryenus ryenus changed the title from separate disabled extensions from installed ones to separate view for disabled extensions Apr 13, 2018

@sandy081 sandy081 requested a review from ramya-rao-a Apr 13, 2018

@sandy081 sandy081 removed their assignment Apr 13, 2018

@ramya-rao-a

This comment has been minimized.

Member

ramya-rao-a commented Apr 16, 2018

In this case the Installed section would have to be renamed to Enabled.
Another way to go about this is to have 4 views shown in the default view

  • Installed
  • Recommended
  • Enabled (hidden by default)
  • Disabled (hidden by default)

To hide any of the above, user can right click on the section header and choose Hide.
To show any of the above, user can do View -> Open View and choose any number from the above 5

@ryenus

This comment has been minimized.

Contributor

ryenus commented Apr 17, 2018

Thank you @ramya-rao-a, the first option is more close to the actual purpose, that is to change Installed to Enabled, to make it easier to go through installed extensions and enable/disable certain ones.

I've updated the PR, please let me know if rebase/squash is needed.

@ryenus

This comment has been minimized.

Contributor

ryenus commented Apr 20, 2018

Here's the updated screenshot, with Installed changed to Enabled:

@ramya-rao-a

This comment has been minimized.

Member

ramya-rao-a commented Apr 25, 2018

@ryenus

When trying your code, the recommended section appears first for me. This wouldnt result in a smooth transition for users.

image

This is most likely because we cache the state of the view (for each section, we cache its order in the view, height and whether it was collapsed or not). Since the new sections werent part of the cache, they show up later.

If we go with the other option then,

  • we maintain the status quo for most users
  • we give option to customize the default view to their liking
  • also, 3 sections feels a bit crowded in smaller screens if we go the current way
@ryenus

This comment has been minimized.

Contributor

ryenus commented Apr 26, 2018

If we go with the other option ... we maintain the status quo for most users

Indeed! I mainly use the stable release, so my local dev build is too clean for me to notice the caching issue.

Let's keep the Installed section. And for Enabled/Disabled, what about make them collapsed by default?

@ryenus

This comment has been minimized.

Contributor

ryenus commented Apr 26, 2018

Now the Enabled/Disabled views are moved to the bottom,
shown as collapsed by default, and can be hidden.

@ryenus

This comment has been minimized.

Contributor

ryenus commented Apr 26, 2018

hmm, please hold on, I tried to disable an extension, reload, then the view orders are messed up,
with Installed sits at the bottom, meanwhile Enabled/Disabled at the top, what a surprise.

@ryenus

This comment has been minimized.

Contributor

ryenus commented Apr 26, 2018

Now view sorting works as expected, as well as customized order:

@ryenus

This comment has been minimized.

Contributor

ryenus commented May 8, 2018

@ramya-rao-a, now the sorting issue is resolved.

The problem was viewState.order could be undefined even though viewState was valid.
I've added strict checking for viewState.order to safely fall back to viewDescriptor.order.

@ryenus ryenus referenced this pull request May 25, 2018

Closed

Fix view order #50411

ryenus added some commits Jun 7, 2018

extract viewOrder calculation logic
and return Number.MAX_VALUE instead of Number.POSITIVE_INFINITY for
views end up with undefined order, when comparing two views like this,
we get Number.MAX_VALUE - Number.MAX_VALUE = 0, rather than NaN
caused by infinity arithmetic.
@ramya-rao-a

Thanks @ryenus, the PR is looking good!
I have left a few comments, please do take a look.

@sandy081 As part of this feature, we would want to enable the canToggleVisibility on the Installed section in the extensions view. If someone chooses to have just the Enabled section or Enabled and Disabled section separately, then it doesnt make sense to not let them hide the "Installed" section.

@@ -593,6 +595,38 @@ export class InstalledExtensionsView extends ExtensionsListView {
}
}
export class EnabledExtensionsView extends ExtensionsListView {
public static isEnabledExtensionsQuery(query: string): boolean {

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Jun 8, 2018

Member

This method isnt used anywhere. Do we really need it? Same goes for the DisabledExtensionsView .isDisabledExtensionsQuery

return ExtensionsListView.isEnabledExtensionsQuery(query);
}
async show(query: string): TPromise<IPagedModel<IExtension>> {

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Jun 8, 2018

Member

Why not go with a simple

async show(query: string): TPromise<IPagedModel<IExtension>> {
		return super.show('@enabled');
	}

?

Same for the the show method in DisabledExtensionsView

This comment has been minimized.

@ryenus

ryenus Jun 9, 2018

Contributor

done! you're definitely right, I've inlined the code and removed these methods, and refined the query string checking logic a bit more, cheers!

@@ -236,7 +236,9 @@ export class ExtensionsListView extends ViewletPanel {
const result = local
.sort((e1, e2) => e1.displayName.localeCompare(e2.displayName))
.filter(e => runningExtensions.every(r => !areSameExtensions(r, e)) && (e.name.toLowerCase().indexOf(value) > -1 || e.displayName.toLowerCase().indexOf(value) > -1));
.filter(e => e.type === LocalExtensionType.User // exclude disabled builtin extensions

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Jun 8, 2018

Member

If we ignore the built-in extensions here, then the only way one can get to them is to go the built-in view
Any reason for not including them here?

This comment has been minimized.

@ryenus

ryenus Jun 9, 2018

Contributor

The original reasoning is, in the Installed Extensions view, only the user installed extensions are listed, therefore the same is done for the Disabled Extensions view.
And, for an built-in extension to be disabled, the user must go to one of the built-in views to disable it, so somehow it also makes sense for the user to go to the built-in views to re-enable it.

But I agree that a disabled built-in extension is nevertheless a disabled extension, and including the disabled built-in extensions here would make it easier for the user to re-enable it, which is likely to be less annoying.

So maybe doing less is a better option here, please let me know if you prefer to include the builtin ones.

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Jun 9, 2018

Member

Let's include the built-in disabled ones here.
Even today, when you go to the disabled extensions view, the disabled built in ones do appear there. So let's maintain that

ctor: EnabledExtensionsView,
when: ContextKeyExpr.and(ContextKeyExpr.not('searchExtensions')),
weight: 30,
canToggleVisibility: true,

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Jun 8, 2018

Member

We should add canToggleVisibility: true to the "Installed" view as well. If someone chooses to have just the Enabled section or Enabled and Disabled section separately, then it doesnt make sense to not let them hide the "Installed" section.

cc @sandy081

This comment has been minimized.

@sandy081

sandy081 Jun 11, 2018

Member

I think seeing only Enabled extensions might be what many users want. Showing again installed extensions might add noise. So users can configure to see enabled extensions and hide installed.

Also how about configuring the default views to show on open? We can have a good defaults (Enabled and Recommended) and hide the installed view by default? Users can customise from the context menu to what to see by default?

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Jun 12, 2018

Member

@sandy081

We can have a good defaults (Enabled and Recommended) and hide the installed view by default? Users can customise from the context menu to what to see by default?

That sounds good. I'll add that in a separate commit

@@ -320,26 +320,17 @@ export class ContributableViewsModel extends Disposable {
}
private compareViewDescriptors(a: IViewDescriptor, b: IViewDescriptor): number {
const viewStateA = this.viewStates.get(a.id);

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Jun 8, 2018

Member

@sandy081 FYI on this change to compareViewDescriptors

From @ryenus,

return Number.MAX_VALUE instead of Number.POSITIVE_INFINITY for
views end up with undefined order, when comparing two views like this,
we get Number.MAX_VALUE - Number.MAX_VALUE = 0, rather than NaN
caused by infinity arithmetic.

This comment has been minimized.

@sandy081

@ramya-rao-a ramya-rao-a changed the title from separate view for disabled extensions to Allow customization of the viewlets in the default Extensions view Jun 8, 2018

@ramya-rao-a ramya-rao-a added this to the June 2018 milestone Jun 8, 2018

if (!query || ExtensionsListView.isEnabledExtensionsQuery(query)) {
return super.show(queryEnabled);
}
return super.show(queryEnabled + ' ' + query);

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Jun 9, 2018

Member

The minute there is a query involved, the default ExtensionsListView gets used. So the code flow wouldn't reach here.
Therefore, let's keep it simple and have this as

async show(query: string): TPromise<IPagedModel<IExtension>> {
 		return super.show('@enabled'));		 		
}
@ramya-rao-a

The changes look good now @ryenus
Just waiting on @sandy081 on his thoughts for the changes to the compareViewDescriptors and making the Installed section hide-able after which we can merge the changes

@ramya-rao-a ramya-rao-a merged commit e8fc894 into Microsoft:master Jun 12, 2018

2 checks passed

VSTS: VS Code 20180610.3 succeeded with issues
Details
license/cla All CLA requirements met.
Details
@ryenus

This comment has been minimized.

Contributor

ryenus commented Jun 12, 2018

@ramya-rao-a I'm really glad to see this is now merged, thank you so much for all the help!

@ramya-rao-a

This comment has been minimized.

Member

ramya-rao-a commented Jun 12, 2018

Its our pleasure @ryenus, thanks for all your work.

Happy Coding!

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