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

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

Merged
merged 6 commits into from
Jun 12, 2018
Merged

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

merged 6 commits into from
Jun 12, 2018

Conversation

ryenus
Copy link
Contributor

@ryenus ryenus commented Apr 12, 2018 •
edited
Loading

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
Copy link

msftclas commented Apr 12, 2018 •
edited
Loading

CLA assistant check
All CLA requirements met.

@ryenus ryenus changed the title separate disabled extensions from installed ones separate view for disabled extensions Apr 13, 2018
@sandy081 sandy081 requested a review from ramya-rao-a April 13, 2018 08:54
@sandy081 sandy081 removed their assignment Apr 13, 2018
@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Apr 16, 2018 •
edited
Loading

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
Copy link
Contributor Author

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
Copy link
Contributor Author

ryenus commented Apr 20, 2018

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

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Apr 25, 2018 •
edited
Loading

@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
Copy link
Contributor Author

ryenus commented Apr 26, 2018 •
edited
Loading

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
Copy link
Contributor Author

ryenus commented Apr 26, 2018 •
edited
Loading

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

@ryenus
Copy link
Contributor Author

ryenus commented Apr 26, 2018 •
edited
Loading

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
Copy link
Contributor Author

ryenus commented Apr 26, 2018

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

@ryenus
Copy link
Contributor Author

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 mentioned this pull request May 25, 2018
Closed
ryenus added 2 commits June 8, 2018 01:15
@ryenus
add Enabled/Disabled extensions list view
c5e41be
@ryenus
extract viewOrder calculation logic
d5f52f8 
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
ramya-rao-a suggested changes Jun 8, 2018
View reviewed changes
Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

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.

src/vs/workbench/parts/extensions/electron-browser/extensionsViews.ts Outdated Show resolved Hide resolved
src/vs/workbench/parts/extensions/electron-browser/extensionsViews.ts Outdated Show resolved Hide resolved
src/vs/workbench/parts/extensions/electron-browser/extensionsViews.ts Outdated Show resolved Hide resolved
src/vs/workbench/parts/extensions/electron-browser/extensionsViewlet.ts Show resolved Hide resolved
src/vs/workbench/browser/parts/views/views.ts Show resolved Hide resolved
@ramya-rao-a ramya-rao-a changed the title separate view for disabled extensions 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
ramya-rao-a
ramya-rao-a approved these changes Jun 10, 2018
View reviewed changes
Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

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
@ryenus
Copy link
Contributor Author

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
Copy link
Contributor

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

Happy Coding!

@ramya-rao-a ramya-rao-a mentioned this pull request Jun 12, 2018
Closed
@ryenus ryenus deleted the separate-disabled-exts branch June 13, 2018 17:05
@ramya-rao-a ramya-rao-a mentioned this pull request Jun 15, 2018
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ramya-rao-a ramya-rao-a ramya-rao-a approved these changes

@sandy081 sandy081 sandy081 left review comments

Assignees

@ramya-rao-a ramya-rao-a

Labels
None yet
Projects
None yet
Milestone
June 2018
Development

Successfully merging this pull request may close these issues.
4 participants
@ryenus @msftclas @ramya-rao-a @sandy081