Disable and enable extensions #11184

Merged
merged 11 commits into from Jul 8, 2015

Projects

None yet
@busykai
Member
busykai commented May 30, 2015

This PR provides a simple mechanism to disable and enable again an extension and complete support of the functionality in the Extension Manager.

Extension loader looks for .disabled file in the extension directory, if it's present the information is included and propagated to the entire extensibility subsystem. Extension Manager can disable and enable extensions using the same mechanism.

busykai added some commits May 29, 2015
@busykai busykai Support disabling extensions via .disabled file.
Extension can also be disabled using `"disable": true` in the package.json.

Details:
    - Rename getPackageJson to getMetadata.
    - Check for presence of .disabled file in the extension directory and add
      it to the metadata.
    - Do not load extension if it is disabled instead, reject with disabled
      status.
    - Add DISABLED extension state.
    - Extension is still gets on record and is enumerated with all its
      metadata, but it's not started.
314d7ff
@busykai busykai Support disabling/enabling extensions in UI. c8af09f
@busykai
Member
busykai commented May 31, 2015

CC: @albertinad, you made me do it. now you have to review it. :)

@mackenza mackenza commented on the diff May 31, 2015
src/nls/root/strings.js
"MARKED_FOR_REMOVAL" : "Marked for removal",
"UNDO_REMOVE" : "Undo",
"MARKED_FOR_UPDATE" : "Marked for update",
"UNDO_UPDATE" : "Undo",
+ "MARKED_FOR_DISABLING" : "Marked for disabling",
@mackenza
mackenza May 31, 2015 Contributor

I am not keen on this phrase. Perhaps Marked as disabled would be more appropriate. It doesn't have to follow the semantics of removal and update.

@busykai
busykai May 31, 2015 Member

I don't like it either, so we could probably come with something better. Marked as disabled is not good because it implies a completed action. Marked to be disabled, perhaps?

@TomMalbran
TomMalbran May 31, 2015 Contributor

Well it does. Since it is not disabled until the dialog closes, so is not true that it is "Marked as disabled".

@mackenza
mackenza May 31, 2015 Contributor

You are thinking that the user knows of the implementation. According to the UI, I hit the disable button and now it is Marked as Disabled. This is further backed by the fact that there is an undo link... meaning I am undoing the action of marking as disabled. The user has no idea (or should they) when the actual file gets written to disk.

@busykai
busykai May 31, 2015 Member

@TomMalbran is right. User has to be aware anyways: after extension manager is dismissed, there will be a dialog asking for confirmation and if canceled all extensions marked will be unmarked (and left intact).

So what about Marked to be disabled vs Marked for disabling? With all the context, do we need to change it in the first place?

@mackenza
mackenza May 31, 2015 Contributor

Neither one is English, so it doesn't matter.

You are focusing too much on the disabled part. The action you are taking in the UI is marking and once you click on disable it is marked.

... but choose whatever you want.

@busykai
busykai May 31, 2015 Member

The label appears when the action of marking has been taken, so marked is totally appropriate. OK, I will leave it as is for now.

@mackenza
Contributor

Good stuff. As I mentioned on Slack, I think the .disabled file is a great, simple way of using convention over configuration to make this work. So I definately appreciate that.

Unless I am missing something, or maybe behaviour is different on Linux (Ubuntu 14.04 x64 here), but I don't see how you can enable a disabled extension. I was expecting an Enable button in my "Installed" list in Extension Manager.

@busykai
Member
busykai commented May 31, 2015

Unless I am missing something, or maybe behaviour is different on Linux (Ubuntu 14.04 x64 here), but I don't see how you can enable a disabled extension. I was expecting an Enable button in my "Installed" list in Extension Manager.

This sounds as a bug. Checked, it should be Enable. Somehow it regressed while I was writing tests. :( Will fix.

@mackenza
Contributor

My disabled extensions show up in the list, but the buttons all say Disable

@busykai
Member
busykai commented May 31, 2015

@mackenza, fixed it (slightly blushing).

@TomMalbran TomMalbran commented on the diff May 31, 2015
src/extensibility/Package.js
+ * @param {string} path The absolute path to the extension to enable.
+ * @return {$.Promise} A promise that's resolved when the extenion is enable, or
+ * rejected if there was an error.
+ */
+ function enable(path) {
+ var result = new $.Deferred(),
+ file = FileSystem.getFileForPath(path + "/.disabled");
+ file.unlink(function (err) {
+ if (err) {
+ result.reject(err);
+ return;
+ }
+ ExtensionLoader.loadExtension(FileUtils.getBaseName(path), { baseUrl: path }, "main")
+ .done(result.resolve)
+ .fail(result.reject);
+ });
@TomMalbran
TomMalbran May 31, 2015 Contributor

You forgot to return the promise

@busykai
busykai Jun 1, 2015 Member

Thank you! Fixed.

@TomMalbran
Contributor

You need to unmark all extensions that were marked to be disabled after this line (https://github.com/adobe/brackets/blob/kai/disable-enable-extensions/src/extensibility/ExtensionManagerDialog.js#L209).

@TomMalbran TomMalbran commented on the diff May 31, 2015
src/extensibility/ExtensionManagerDialog.js
});
- })
- .fail(function (errorArray) {
- dlg.close();
- ExtensionManager.cleanupUpdates();
@TomMalbran
TomMalbran May 31, 2015 Contributor

Don't you still need line after if (removeErrors) { ?

@busykai
busykai Jun 1, 2015 Member

No, I don't think so. Updates are done always (in parallel) with this change independently of how it went for removes. The only place it needs to be done is when the entire confirmation is cancelled.

@TomMalbran TomMalbran commented on an outdated diff May 31, 2015
src/extensibility/ExtensionManagerDialog.js
@@ -107,57 +110,98 @@ define(function (require, exports, module) {
.text(Strings.PROCESSING_EXTENSIONS)
.append("<span class='spinner inline spin'/>");
- ExtensionManager.removeMarkedExtensions()
+ var removeExtensionsPromise,
+ updateExtensionsPromise,
+ disableExtensionsPromise,
+ removeErrors,
+ updateErrors,
+ disableErrors;
+
+ removeExtensionsPromise = ExtensionManager.removeMarkedExtensions();
+ removeExtensionsPromise
@TomMalbran
TomMalbran May 31, 2015 Contributor

.fails returns a promise, so you could remove this promise repetitions and chain with the assignment.

@zaggino
Member
zaggino commented May 31, 2015

question: will this work for default extensions? if so, can we add .disabled to .gitignore file?

@busykai
Member
busykai commented May 31, 2015

@zaggino, it will work on default extensions via manually creating and deleting .disabled file.

@TomMalbran, thanks for the thorough review will fix these.

@mackenza
Contributor

@zaggino @busykai I went into /src/extensions/default/CodeFolding, did a touch .disabled and reloaded Brackets and, indeed, no code folding.

@TomMalbran TomMalbran and 1 other commented on an outdated diff May 31, 2015
src/extensibility/ExtensionManagerDialog.js
@@ -107,57 +110,98 @@ define(function (require, exports, module) {
.text(Strings.PROCESSING_EXTENSIONS)
.append("<span class='spinner inline spin'/>");
- ExtensionManager.removeMarkedExtensions()
+ var removeExtensionsPromise,
+ updateExtensionsPromise,
+ disableExtensionsPromise,
+ removeErrors,
+ updateErrors,
+ disableErrors;
+
+ removeExtensionsPromise = ExtensionManager.removeMarkedExtensions();
+ removeExtensionsPromise
+ .fail(function (errorArray) {
+ removeErrors = errorArray;
@TomMalbran
TomMalbran May 31, 2015 Contributor

I think that the code might look better if you parsed the error array here and created the dialog, instead of doing it in the general fail.

@busykai
busykai Jun 1, 2015 Member

There's no need to create dialogs at this scope. The error condition is processed in the .fail of the consolidated promise (Async.waitForAll).

@zaggino
Member
zaggino commented May 31, 2015

@busykai great, can you add an .disabled entry to the https://github.com/adobe/brackets/blob/master/.gitignore then?

@busykai
Member
busykai commented Jun 1, 2015

@zaggino, you're right. Will add this file to .gitignore.

busykai added some commits Jun 1, 2015
@busykai busykai Address review comments.
See #11184.
6bd7c64
@busykai busykai Ignore .disabled.
It could be used to disable default extensions and must never be committed.
e4394af
@busykai busykai Handle error condition correctly.
Async.waitForAll would not fail, unless failOnReject argument is true.
97755d4
@busykai
Member
busykai commented Jun 1, 2015

Did some more testing, focused on error handling. Seems to be fine now.

@prksingh prksingh was assigned by nethip Jun 1, 2015
@MarcelGerber MarcelGerber and 2 others commented on an outdated diff Jun 1, 2015
@@ -34,3 +34,6 @@ Thumbs.db
# Files that can be automatically downloaded that we don't want to ship with our builds
/src/extensibility/node/node_modules/request/tests/
+
+# .disabled should be ignored, it could be used to disable default extensions
+.disabled
@MarcelGerber
MarcelGerber Jun 1, 2015 Member

This should be more specific: src/extensions/default/*/.disabled

We may also use this file in unit tests for this feature, and in this case it should be included.

@busykai
busykai Jun 1, 2015 Member

I think it's an overkill. It clearly says default extensions there's nothing to misunderstand.

@redmunds
redmunds Jun 1, 2015 Contributor

In installer builds, "src" is "www". Don't you want to handle both? (referring to line above, not this one)

@MarcelGerber
MarcelGerber Jun 1, 2015 Member

Why would we need a .gitignore in installer builds? It's not like the www is a git repo...

@busykai
busykai Jun 12, 2015 Member

@MarcelGerber, actually I misunderstood you, you were saying the rule itself should be more specific and I understood you were referring to the comment text. Will fix this.

@abose abose assigned rroshan1 and unassigned prksingh Jun 16, 2015
@rroshan1 rroshan1 and 4 others commented on an outdated diff Jun 17, 2015
src/extensibility/ExtensionManager.js
+ });
+ } 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(),
@rroshan1
rroshan1 Jun 17, 2015 Contributor

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?

@busykai
busykai Jun 25, 2015 Member

@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?

@MarcelGerber
MarcelGerber Jul 2, 2015 Member

@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.

@busykai
busykai Jul 2, 2015 Member

@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.

@MarcelGerber
MarcelGerber Jul 3, 2015 Member

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

@busykai
busykai Jul 6, 2015 Member

That's what the unit tests are for.

@rroshan1
rroshan1 Jul 7, 2015 Contributor

@busykai I understand your concern. You do have a valid point.
But keeping both in mind, better readability as well as de-duplication of code, I would suggest a solution that you would like too for sure. 😃

/**
     * 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) {
        return disableEnable(id, true);   // makeDisabled = true
    }

/**
     * 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) {
        return disableEnable(id, false);   // makeDisabled = false
      }

Where disableEnable() is defined as:-

function disableEnable(id, makeDisabled) {
        var result = new $.Deferred(),
            extension = extensions[id];
        if (extension && extension.installInfo) {
            Package.disable(extension.installInfo.path)
                .done(function () {
                    extension.installInfo.status = (makeDisabled === true)? DISABLED: ENABLED;
                    extension.installInfo.metadata.disabled = (makeDisabled === true)? true: 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();
    }

Please share as to what you think. Am eager to merge it soon after the changes.

@TomMalbran
TomMalbran Jul 7, 2015 Contributor

Notice that you also need to do Package[makeDisabled ? "disable" : "enable"](extension.installInfo.path).
And you dont really need to compare with true.

@rroshan1
rroshan1 Jul 7, 2015 Contributor

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

@busykai
busykai Jul 7, 2015 Member

Yeah, so how this is nicer? Why not do enableDisableRemove then to eliminate remove function as well?

@busykai
busykai Jul 7, 2015 Member

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

@mackenza
mackenza Jul 7, 2015 Contributor

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.

@busykai
busykai Jul 7, 2015 Member

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

@rroshan1
Contributor

With an update available for an extension, the buttons look like this (refer first extension):-
extensions
We can either have all 3 bttons in same line or just update in separate line..

@rroshan1
Contributor

@busykai I really liked this feature. Enabling and disabling of extensions is super smooth now. 👍
All unit tests pass and I have reviewed the PR from my side and shared my comments.
Can you please update ".disabled" entry in the .gitignore file to more specific "src/extensions/default/*/.disabled" and address my comments and we should be all ready to have this PR merged soon. 😄

@busykai
Member
busykai commented Jun 19, 2015

@rroshan1, thanks for the review. I'll get to this real soon. I'm not sure what to do with the buttons, though... @larz0, would you recommend a direction? Perhaps, stack them vertically instead (always). Or not show the disable button when there's an update (not good). Or increase the size of the dialog...

@larz0
Member
larz0 commented Jun 19, 2015

@busykai can we replace the "Delete" button with the delete icon below and see if things can fit? When the buttons are stacked on the right the right side will seem heavier compared to the left side of the modal.

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" preserveAspectRatio="xMidYMid" width="17" height="18" viewBox="0 0 17 18">
  <defs>
    <style>
      .cls-1 {
        fill: #7f8181;
        fill-rule: evenodd;
      }
    </style>
  </defs>
  <path d="M6.000,7.000 L6.000,14.000 L7.000,14.000 L7.000,7.000 L6.000,7.000 ZM8.000,7.000 L8.000,14.000 L9.000,14.000 L9.000,7.000 L8.000,7.000 ZM10.000,7.000 L10.000,14.000 L11.000,14.000 L11.000,7.000 L10.000,7.000 ZM13.000,2.000 C13.000,0.470 12.529,-0.000 11.000,-0.000 L6.000,-0.000 C4.380,-0.000 4.000,0.500 4.000,2.000 L4.000,3.000 L-0.000,3.000 L-0.000,5.000 L2.000,5.000 L2.000,16.000 C2.000,17.500 2.471,18.000 4.000,18.000 L13.000,18.000 C14.410,18.000 15.000,17.470 15.000,16.000 L15.000,5.000 L17.000,5.000 L17.000,3.000 L13.000,3.000 L13.000,2.000 ZM11.000,2.000 L11.000,3.000 L6.000,3.000 L6.000,2.000 L11.000,2.000 ZM4.000,5.000 L13.000,5.000 L13.000,16.000 L4.000,16.000 L4.000,5.000 Z" class="cls-1"/>
</svg>
@rroshan1
Contributor

@larz0 @busykai I was thinking of something like this- Update button if applicable should be on a separate line while the disable and remove buttons should be consistent.. Since mostly there won't be many update buttons, the right side won't seem to be heavy either. What do you guys think?
extensions2

@larz0
Member
larz0 commented Jun 23, 2015

@rroshan1 that works too! We shouldn't be getting more buttons in that area (fingers crossed) so let's go with that 👍

@nethip
Contributor
nethip commented Jun 23, 2015

@larz0 Thanks!.

@busykai Please go ahead with the suggested changes to this PR, so that it can be merged. Let us target this for 1.4.

@abose abose added this to the Release 1.4 milestone Jun 23, 2015
@busykai
Member
busykai commented Jun 25, 2015

@rroshan1, simplicity is the best! :) I should have thought about this layout!..

@busykai
Member
busykai commented Jun 25, 2015

@rroshan1, @larz0 how about this? made the buttons a little bit more spread:
screen shot 2015-06-25 at 11 25 01
or little less spread:
screen shot 2015-06-25 at 11 27 45
I like the second better.

@abose
Contributor
abose commented Jun 25, 2015

I like the first one better. The second one seems a bit out of proportion to me.

@lenovouser

@busykai I think the first one looks better. The second one seems so cluttered.

@busykai
Member
busykai commented Jun 25, 2015

OK. FWIW, the second one is what close to what we used to have (buttons are close to each other).

busykai added some commits Jun 25, 2015
@busykai busykai Change layout to show the update button separately
Added margin of 3px to any extension action button (more spread buttons).
03b1e44
@busykai busykai Make .disabled file rule more concise. d7b6675
@busykai
Member
busykai commented Jun 25, 2015

@rroshan1, ready for re-review. did not address separate functions comment though. let me know if you feel it has to be addressed.

@mackenza
Contributor

@busykai agreed 1st screenshot is much better than 2. Even if 2 is what we have now.

side note: why is remove greyed out on "Proper Indent" extension in both SS?

@busykai
Member
busykai commented Jun 25, 2015

@mackenza, it's in src/extensions/dev (space to develop extensions) -- they are not managed via this dialog (but you still should be able to disable your extension if you screwed up :).

EDIT: I've pushed the first version, so more spacious design with this change.

@larz0
Member
larz0 commented Jun 26, 2015

@busykai great, first screenshot is better but could we shave off 2px to the spacing between Disable and Remove buttons?

@busykai
Member
busykai commented Jun 26, 2015

@larz0, like this?
screen shot 2015-06-26 at 11 23 04

@busykai busykai Change the extension manager layout.
Remove right margin entirely.
1eef654
@busykai
Member
busykai commented Jun 29, 2015

@larz0, so I've pushed the changes to the layout corresponding to the last image.

@rroshan1, this is ready for review.

@larz0
Member
larz0 commented Jun 30, 2015

@busykai looks good here 👍

@rroshan1
Contributor

Thanks @busykai .. 👍
I will get back soon after doing some testing and review.

@busykai
Member
busykai commented Jul 7, 2015

@rroshan1, now fully compliant to the comments.

@rroshan1
Contributor
rroshan1 commented Jul 8, 2015

Thanks @busykai..
I have run unit tests and scenario testings from my side and everything looks good and smooth.
Merging it.

@rroshan1 rroshan1 merged commit 2501136 into master Jul 8, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@busykai busykai deleted the kai/disable-enable-extensions branch Jul 8, 2015
@sprintr sprintr added a commit to sprintr/brackets that referenced this pull request Jul 13, 2015
@busykai @sprintr busykai + sprintr Address review comments.
See #11184.
b27c8ff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment