Drag and drop local zip file install for ExtensionManager #8166
Conversation
| promise; | ||
|
|
||
| // TODO validate version? | ||
| if (isUpdate) { |
There was a problem hiding this comment.
@dangoor I wasn't sure if I should be checking versions here to block users from installing older versions. Compatibility with brackets API version is handled elsewhere right?
There was a problem hiding this comment.
I don't think we currently do any version/compatibility checking when using the manual "Install from URL" workflow. Maybe we should, though :-) @jasonsanjose do you think it's something that's critical for our use case here?
There was a problem hiding this comment.
I just checked in ExtensionDomain.js. We do check compatibility on installation:
if (validationResult.metadata && validationResult.metadata.engines &&
validationResult.metadata.engines.brackets) {
var compatible = semver.satisfies(options.apiVersion,
validationResult.metadata.engines.brackets);
if (!compatible) {
installDirectory = path.join(options.disabledDirectory, extensionName);
validationResult.installationStatus = Statuses.DISABLED;
validationResult.disabledReason = Errors.API_NOT_COMPATIBLE;
_removeAndInstall(packagePath, installDirectory, validationResult, deleteTempAndCallback);
return;
}
}There was a problem hiding this comment.
That's API compatibility that we check. I don't think we block installation of older versions anywhere. If someone wants to install an older version of an extension and it's compatible, we don't stop them.
There was a problem hiding this comment.
Great! That works for us.
|
It would be great to see this land in the next sprint to help us as we move towards a prerelease. |
| ExtensionManagerView.prototype._installUsingDragAndDrop = function () { | ||
| var self = this; | ||
|
|
||
| brackets.app.getDroppedFiles(function (err, files) { |
There was a problem hiding this comment.
At first I was a bit confused as to why we can't just keep looking at event.originalEvent.dataTransfer.files (we do it this way in brackets.js for the general drag/drop case too)... but upon further inspection I think it's because the W3C event never provides you the full, local path (since you can't normally do anything useful with it). @RaymondLim is that right?
There was a problem hiding this comment.
Correct. This is why we have to rely on shell code to provide the file paths.
|
Marking triage complete. I added a few questions/comments, but the overall approach looks solid to me. And this will be useful for users who are stuck behind a firewall, etc. too (better than our existing manual install workflow). |
|
Oh, one other question: should we add anything to the UI indicating that Extension Manager is a 'drop zone'? Otherwise this is totally undiscoverable... though otoh I can't think of anything offhand that doesn't clutter up the UI. Thoughts, @larz0? |
…d flag to keep local zip files.
|
Implemented UI from @larz0:
|
|
@peterflynn ready for review |
| * @return {$.Promise} A promise that's resolved when the extension is updated or | ||
| * rejected with an error if there's a problem with the update. | ||
| */ | ||
| function update(id, packagePath) { | ||
| return Package.installUpdate(packagePath, id); | ||
| function update(id, packagePath, keepFile) { |
There was a problem hiding this comment.
Had to add a keepFile flag so that we don't automatically cleanup local zip files from drag-and-drop.
|
@jasonsanjose UI looks good! |
| var extension = FileUtils.getFileExtension(path); | ||
|
|
||
| if (err || !file.isFile || (extension !== "zip")) { | ||
| result.reject(); |
There was a problem hiding this comment.
I think it's considered bad style to reject without a proper error cause.
…nddrop Conflicts: src/styles/brackets.less
| // new install or an update. | ||
| Package.validate(path).then(function (info) { | ||
| if (info.errors.length) { | ||
| result.reject(); |
There was a problem hiding this comment.
Ditto what @ingorichter says above. If the user drops an invalid package here, we should tell them what's wrong.
|
I worry a little about the lack of tests for this because I wouldn't want to see the |
|
One thing I noticed in testing: if I drag in an image file and place it where the zip should go, Brackets ends up opening the image behind the Extension Manager dialog. |
|
Works pretty well when you drop in a valid zip file. For multiple zip files, the experience is not quite as good because the install dialog isn't really set up to install multiple files nicely. Ideal case would be something more like a download manager that lists everything that's installing and shows the status of each. Even listing which extension "installed successfully" may be an improvement when installing multiple. But, installing multiple is something of an edge case, so I don't think we need to do anything as far as that goes right now. |
| * localized error message. | ||
| * | ||
| * @param {string} path Absolute path to the package zip file | ||
| * @param {?string} nameHint Hint for the extension folder's name (used in favor of |
There was a problem hiding this comment.
nit: jsdoc argument name doesn't match the actual one filenameHint
|
Review done (finally! I've been hopping back and forth between things all day). I did test with a package that had an invalid value in package.json and this just fails silently. Again, that's an edge case... but a quick blink of drop zone is all I saw, and I can see someone being given a broken test package and then filing a bug saying "package installation doesn't work!" To summarize:
This is a very useful feature! Thanks for building this, Jason! |
|
@dangoor @jasonsanjose Would it be simpler to turn off support for dropping multiple zip files? I'm not sure how ugly it currently looks when you do that, or how much complexity it adds to the code, but we could easily just no-op or show an error when the number of dropped files != 1... And I don't think we have an urgent use case for dropping multiple files at this point... |
|
@peterflynn It's not super ugly. It's basically just one modal install dialog after another, and it goes quickly since the files are local. There's a little complexity added, but it's mostly just a couple of calls to Async.doSequentially. I'm overall pretty neutral on the support of multiple files. |
|
Thanks for the review. I'm off today but I can respond on Monday. When is this sprint locking down? |
|
Monday would be the earliest. We can shift by a day or two if needed. |
|
Here's the new error dialog when validation errors occur. Notes:
|
|
Is "Unknown internal error" just a placeholder? Or does that error come up a lot? If so, should we think about not showing the error message? |
It's only possible in a few cases where we don't have error strings, see https://github.com/adobe/brackets/blob/master/src/extensibility/node/package-validator.js#L42. This is not new behavior. I just happened to be testing a weird case where package.json was missing (which we don't have a string for). |
|
Cool, sounds good! |
…github.com/adobe/brackets into jasonsanjose/extension-install-draganddrop
|
From @dangoor's list
Fixed. I'll have unit tests updated soon. |
|
Well, I guess it's reasonable to have an error message for extensions missing a package.json, right? |
|
@SAplayer In the usual case -- installing from extension registry -- that's not possible, so it's sort of an edge case. That said, I'm sure the extension registry itself has strings for package format errors like that, so it shouldn't be hard to copy them over if we want... just doesn't seem that urgent. |
|
Well, we've got an "main.js missing" string too, which the registry won't allow either, I guess. |
|
@SAplayer Do you want to file a bug so we can track adding more error strings? (I don't think it should block this PR) |
|
@jasonsanjose This looks good. I think we can merge this and add the test in a follow up. |
…aganddrop Drag and drop local zip file install for ExtensionManager
|
Merging but keeping the branch open for the test. |
|
Actually. I'll just delete this branch and sync to master anyway. Thanks! |
|
@jasonsanjose Thank you! This is bound to be a helpful feature. :D I just noticed one thing, not sure if it was intentional or not. When you hover over the link, the cursor remains at |
|
Sweet! Thanks to everyone for getting this merged. Can't wait to be able to use this. |
|
Thanks for catching the cursor problem. I'll fix that tonight. |
Adds drag and drop support to the
ExtensionManagerDialogto allow users to install extensions via local zip files from the native file system. The rough workflow is below:InstallExtensionDialogFYI I noticed #8165 while debugging this branch. It also appears on master.
This new workflow allows users to experiment with extensions not yet delivered via the Extension Registry and that are also not available via a public URL.
Here's a related trello story I found https://trello.com/c/fIGcd4V3.