Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Added Extension Sorting capability according to downloads and last published date #13080

Merged
merged 6 commits into from
Feb 20, 2017

Conversation

saurabh95
Copy link
Contributor

@saurabh95 saurabh95 commented Feb 6, 2017

Now on opening the Extension Manager, a drop-down is shown on the left side of the search bar, which indicates the sorting of extensions according to the selected criteria(currently sort methods available are Last Updated and Download Count), on change of the selection we store the the changed selection in the preferences and if the Extension Manager is invoked again, then we use the same sorting criteria which was selected by user earlier. The sorting is available for Available and Theme Extensions.
I have attached a snapshot of the feature in use where the sort order and the download count has been highlighted for illustration.

downloadcount

Copy link
Collaborator

@petetnt petetnt left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, needs tests 👍

@@ -292,6 +292,20 @@ define(function (require, exports, module) {
});
};

ExtensionManagerViewModel.prototype._setSortedExtensionList = function (extensions, isTheme) {
this.filterSet = this.sortedFullSet = registry_utils.sortRegistry(extensions, "registryInfo")
.filter(function (entry) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There seems to be some redundancy here, seems like same could be achieved with:

return entry.registryInfo && entry.registryInfo.metadata;

Do correct me if I am wrong 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that can be changed :)

@ficristo
Copy link
Collaborator

ficristo commented Feb 6, 2017

Nice! I think @zaggino and @dnbard could have some opinions on this.
See @dnbard extension: https://github.com/dnbard/brackets-extension-rating

I have two doubts:

  • why we should threat the installed tab differently?
  • registry_utils comes from brackets-registry: I guess it is necessary to change it but it seems to me it should be more "self contained" (= don't require Brackets code)

@saurabh95
Copy link
Contributor Author

@ficristo the installed tab is treated differently as we do not have the download count of the installed extensions in the installInfo, and also currently the sorting of installed extensions is done according to the update available.
Yeah we can isolate the registry_utils from Brackets code, we can pass the preference as the argument.

@saurabh95
Copy link
Contributor Author

@ficristo @petetnt Addressed review comments and also added some tests.

@petetnt
Copy link
Collaborator

petetnt commented Feb 9, 2017

Great stuff @saurabh95!

Some issues I spotted:

  1. For me the extension manager opens on Installed page and the sort by-toggle is visible (and does nothing). Needs similar check to https://github.com/adobe/brackets/pull/13080/files#diff-3491ce7bb262eb073c8aabc3d3e064c0R376 when opening the window too.

  2. The sort toggle breaks the layout with really narrow windows (but the search bar does it too). Feel free to disregard / bump to another issue.
    image

  3. The icons for download counts don't seem to work on Windows (10)
    image

Copy link
Collaborator

@petetnt petetnt left a comment

Choose a reason for hiding this comment

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

Reviewed again, couple of small issues, otherwise LGTM 👍

PreferencesManager.set("extensions.sort", "downloadCount");
model._setSortedExtensionList(ExtensionManager.extensions, true);
expect(model.filterSet).toEqual(["theme-2", "theme-1"]);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

A simple check could be added that Brackets actually renders the totalDownloads to the UI 👍

@@ -136,8 +136,16 @@ define(function (require, exports, module) {
sortedEntries.push(registry[key]);
});
sortedEntries.sort(function (entry1, entry2) {
return getPublishTime((subkey && entry2[subkey]) || entry2) -
getPublishTime((subkey && entry1[subkey]) || entry1);
if (sortBy !== "publishedDate") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a big deal, but in general I prefer if you spell out all cases:

if (sortBy === "downloadCount") {
} else if (sortBy === "publishedDate") {
} else {
  console.error("Unknown sortBy " + sortBy ". Defaults to....")
}

Or something like this.

Copy link
Collaborator

@ficristo ficristo left a comment

Choose a reason for hiding this comment

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

I didn't test this so I trust @petetnt.
But in general seems good.

@saurabh95
Copy link
Contributor Author

@petetnt I have added a test for checking downloadCount in view and also added a check for active tab on launch of Extension Manager to hide/show the sort option, regarding the download icon, I will replace it with an image.

Copy link
Collaborator

@swmitra swmitra left a comment

Choose a reason for hiding this comment

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

Good stuff @saurabh95 👍

@saurabh95
Copy link
Contributor Author

A screenshot with the new icon.

untitled

@swmitra
Copy link
Collaborator

swmitra commented Feb 20, 2017

Merging Now with the new Icon.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants