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

Disable and enable extensions #11184

Merged
merged 11 commits into from
Jul 8, 2015
Merged

Disable and enable extensions #11184

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
314d7ff
Support disabling extensions via .disabled file.
busykai May 29, 2015
c8af09f
Support disabling/enabling extensions in UI.
busykai May 30, 2015
04e0ae1
Fix improper chaining of promises.
busykai May 31, 2015
6bd7c64
Address review comments.
busykai Jun 1, 2015
e4394af
Ignore .disabled.
busykai Jun 1, 2015
97755d4
Handle error condition correctly.
busykai Jun 1, 2015
6cc7685
Merge branch 'master' into kai/disable-enable-extensions
prksingh Jun 5, 2015
03b1e44
Change layout to show the update button separately
busykai Jun 25, 2015
d7b6675
Make .disabled file rule more concise.
busykai Jun 25, 2015
1eef654
Change the extension manager layout.
busykai Jun 29, 2015
753c84c
Use a common function to handle enabling/disabling
busykai Jul 7, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 140 additions & 16 deletions src/extensibility/ExtensionManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ define(function (require, exports, module) {
* Extension status constants.
*/
var ENABLED = "enabled",
DISABLED = "disabled",
START_FAILED = "startFailed";

/**
Expand Down Expand Up @@ -103,8 +104,9 @@ define(function (require, exports, module) {
/**
* Requested changes to the installed extensions.
*/
var _idsToRemove = [],
_idsToUpdate = [];
var _idsToRemove = {},
_idsToUpdate = {},
_idsToDisable = {};

PreferencesManager.stateManager.definePreference(FOLDER_AUTOINSTALL, "object", undefined);

Expand Down Expand Up @@ -186,8 +188,9 @@ define(function (require, exports, module) {
*/
function _reset() {
exports.extensions = extensions = {};
_idsToRemove = [];
_idsToUpdate = [];
_idsToRemove = {};
_idsToUpdate = {};
_idsToDisable = {};
}

/**
Expand Down Expand Up @@ -240,8 +243,9 @@ define(function (require, exports, module) {
* @param {string} path The local path of the loaded extension's folder.
*/
function _handleExtensionLoad(e, path) {
function setData(id, metadata) {
function setData(metadata) {
var locationType,
id = metadata.name,
userExtensionPath = ExtensionLoader.getUserExtensionPath();
if (path.indexOf(userExtensionPath) === 0) {
locationType = LOCATION_USER;
Expand All @@ -265,27 +269,33 @@ define(function (require, exports, module) {
metadata: metadata,
path: path,
locationType: locationType,
status: (e.type === "loadFailed" ? START_FAILED : ENABLED)
status: (e.type === "loadFailed" ? START_FAILED : (e.type === "disabled" ? DISABLED : ENABLED))
};

synchronizeEntry(id);
loadTheme(id);
exports.trigger("statusChange", id);
}

function deduceMetadata() {
var match = path.match(/\/([^\/]+)$/),
name = (match && match[1]) || path,
metadata = { name: name, title: name };
return metadata;
}

ExtensionUtils.loadPackageJson(path)
ExtensionUtils.loadMetadata(path)
.done(function (metadata) {
setData(metadata.name, metadata);
setData(metadata);
})
.fail(function () {
.fail(function (disabled) {
// If there's no package.json, this is a legacy extension. It was successfully loaded,
// but we don't have an official ID or metadata for it, so we just create an id and
// "title" for it (which is the last segment of its pathname)
// and record that it's enabled.
var match = path.match(/\/([^\/]+)$/),
name = (match && match[1]) || path,
metadata = { name: name, title: name };
setData(name, metadata);
var metadata = deduceMetadata();
metadata.disabled = disabled;
setData(metadata);
});
}

Expand Down Expand Up @@ -398,6 +408,60 @@ define(function (require, exports, module) {
}
return result.promise();
}

/**
* Disables the installed extension with the given id.
*
* @param {string} id The id of the extension to disable.
* @return {$.Promise} A promise that's resolved when the extenion is disabled or
* rejected with an error that prevented the disabling.
*/
function disable(id) {
var result = new $.Deferred(),
extension = extensions[id];
if (extension && extension.installInfo) {
Package.disable(extension.installInfo.path)
.done(function () {
extension.installInfo.status = DISABLED;
extension.installInfo.metadata.disabled = true;
result.resolve();
exports.trigger("statusChange", id);
})
.fail(function (err) {
result.reject(err);
});
} else {
result.reject(StringUtils.format(Strings.EXTENSION_NOT_INSTALLED, id));
}
return result.promise();
}

/**
* Enables the installed extension with the given id.
*
* @param {string} id The id of the extension to enable.
* @return {$.Promise} A promise that's resolved when the extenion is enabled or
* rejected with an error that prevented the enabling.
*/
function enable(id) {
var result = new $.Deferred(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This function enable() and the former disable() have almost the same definition, do we want to merge them into a single function with an additional parameter- enable flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rroshan1, I don't feel attracted by the idea. Two separate functions are in line with the rest of the actions (e.g. install/remove). It would be OK if Package had a single function, but the actual logic underneath is completely different. Do you feel strong about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@busykai In my opinion, there should be an internal function (e.g. _setDisabledState), taking the argument boolean disable, which does both. It can then be called from both of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rroshan1, @marcelgerber if we had a single function in Package to enable/disable an extension, I would undoubtedly do something like what you guys suggest. However, I am reluctant to sacrifice readability of the code for "compactness". Firstly, the code will have to call Package.(enable|disabled) via variable; secondly, instead of using clear literals (true, false, DISABLED, ENABLED) variables should first evaluated and then used. I don't see how it makes the code more readable or, in general, how it is useful.

The reason we don't have single function in Package and instead have two is because the logic is very different for the actions which need to be taken.

For what it's worth, remove() is very similar in how it calls and treats the result. Should we start creating "unifying" functions with a big switch at the beginning? Sometimes it's worth to repeat 10-15 lines for expressiveness. If I would duplicated 40+ lines (which I wouldn't), then it would be worth discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing is, whenever we have to make any changes, we need to apply these not once, but twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh yes!! Thanks @TomMalbran for pointing out..

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, so how this is nicer? Why not do enableDisableRemove then to eliminate remove function as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, whatever. I disagree completely and I don't understand why this was worth such a discussion. I will make the change you're asking for just to end it. Added lines count will be more than remove lines count.

P.S. I think it would be helpful if someone with the authority would document this in code style guides so that nobody would have to lose their time in a discussion like this.

P.P.S. bikeshed

Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, I think @marcelgerber had the best suggestion in this little thread which amounted to a private function that toggled but allowing for separate public functions for enable and disable.

But that doesn't change that I agree with @busykai that having 1 function for each of the actions (install, remove, enable, disable) is the most readable and conveys the intent of the functions for future readers of the code.

@marcelgerber 's suggestion was a blend of efficiency and readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made the change, let's not discuss this any longer.

extension = extensions[id];
if (extension && extension.installInfo) {
Package.enable(extension.installInfo.path)
.done(function () {
extension.installInfo.status = ENABLED;
extension.installInfo.metadata.disabled = false;
result.resolve();
exports.trigger("statusChange", id);
})
.fail(function (err) {
result.reject(err);
});
} else {
result.reject(StringUtils.format(Strings.EXTENSION_NOT_INSTALLED, id));
}
return result.promise();
}

/**
* Updates an installed extension with the given package file.
Expand Down Expand Up @@ -452,7 +516,7 @@ define(function (require, exports, module) {
}
exports.trigger("statusChange", id);
}

/**
* Returns true if an extension is marked for removal.
* @param {string} id The id of the extension to check.
Expand All @@ -461,7 +525,7 @@ define(function (require, exports, module) {
function isMarkedForRemoval(id) {
return !!(_idsToRemove[id]);
}

/**
* Returns true if there are any extensions marked for removal.
* @return {boolean} true if there are extensions to remove
Expand All @@ -470,6 +534,39 @@ define(function (require, exports, module) {
return Object.keys(_idsToRemove).length > 0;
}

/**
* Marks an extension for disabling later, or unmarks an extension previously marked.
*
* @param {string} id The id of the extension
* @param {boolean} mark Whether to mark or unmark the extension.
*/
function markForDisabling(id, mark) {
if (mark) {
_idsToDisable[id] = true;
} else {
delete _idsToDisable[id];
}
exports.trigger("statusChange", id);
}

/**
* Returns true if an extension is mark for disabling.
*
* @param {string} id The id of the extension to check.
* @return {boolean} true if it's been mark for disabling, false otherwise.
*/
function isMarkedForDisabling(id) {
return !!(_idsToDisable[id]);
}

/**
* Returns true if there are any extensions marked for disabling.
* @return {boolean} true if there are extensions to disable
*/
function hasExtensionsToDisable() {
return Object.keys(_idsToDisable).length > 0;
}

/**
* If a downloaded package appears to be an update, mark the extension for update.
* If an extension was previously marked for removal, marking for update will
Expand Down Expand Up @@ -542,6 +639,25 @@ define(function (require, exports, module) {
}
);
}

/**
* Disables extensions marked for disabling.
*
* If the return promise is rejected, the argument will contain an array of objects. Each
* element is an object identifying the extension failed with "item" property set to the
* extension id which has failed to be disabled and "error" property set to the error.
*
* @return {$.Promise} A promise that's resolved when all extensions marked for disabling are
* disabled or rejected if one or more extensions can't be disabled.
*/
function disableMarkedExtensions() {
return Async.doInParallel_aggregateErrors(
Object.keys(_idsToDisable),
function (id) {
return disable(id);
}
);
}

/**
* Updates extensions previously marked for update.
Expand Down Expand Up @@ -764,7 +880,8 @@ define(function (require, exports, module) {
// Listen to extension load and loadFailed events
ExtensionLoader
.on("load", _handleExtensionLoad)
.on("loadFailed", _handleExtensionLoad);
.on("loadFailed", _handleExtensionLoad)
.on("disabled", _handleExtensionLoad);


EventDispatcher.makeEventDispatcher(exports);
Expand All @@ -775,24 +892,31 @@ define(function (require, exports, module) {
exports.getExtensionURL = getExtensionURL;
exports.remove = remove;
exports.update = update;
exports.disable = disable;
exports.enable = enable;
exports.extensions = extensions;
exports.cleanupUpdates = cleanupUpdates;
exports.markForRemoval = markForRemoval;
exports.isMarkedForRemoval = isMarkedForRemoval;
exports.unmarkAllForRemoval = unmarkAllForRemoval;
exports.hasExtensionsToRemove = hasExtensionsToRemove;
exports.markForDisabling = markForDisabling;
exports.isMarkedForDisabling = isMarkedForDisabling;
exports.hasExtensionsToDisable = hasExtensionsToDisable;
exports.updateFromDownload = updateFromDownload;
exports.removeUpdate = removeUpdate;
exports.isMarkedForUpdate = isMarkedForUpdate;
exports.hasExtensionsToUpdate = hasExtensionsToUpdate;
exports.removeMarkedExtensions = removeMarkedExtensions;
exports.disableMarkedExtensions = disableMarkedExtensions;
exports.updateExtensions = updateExtensions;
exports.getAvailableUpdates = getAvailableUpdates;
exports.cleanAvailableUpdates = cleanAvailableUpdates;

exports.hasDownloadedRegistry = false;

exports.ENABLED = ENABLED;
exports.DISABLED = DISABLED;
exports.START_FAILED = START_FAILED;

exports.LOCATION_DEFAULT = LOCATION_DEFAULT;
Expand Down
Loading