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

Cherry picks #549

Closed
wants to merge 2 commits into from
Closed

Cherry picks #549

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08e148d
GH-540 fix for source-file with app target-dir
Nov 11, 2018
22ccf4b
GH-539 fix destination path fallback
Nov 11, 2018
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
19 changes: 7 additions & 12 deletions bin/templates/cordova/lib/pluginHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,7 @@ var handlers = {
if (!obj.src) throw new CordovaError(generateAttributeError('src', 'source-file', plugin.id));
if (!obj.targetDir) throw new CordovaError(generateAttributeError('target-dir', 'source-file', plugin.id));

var dest = path.join(obj.targetDir, path.basename(obj.src));

// TODO: This code needs to be replaced, since the core plugins need to be re-mapped to a different location in
// a later plugins release. This is for legacy plugins to work with Cordova.

if (options && options.android_studio === true) {
dest = studioPathRemap(obj);
}
var dest = getInstallDestination(obj);
Copy link

Choose a reason for hiding this comment

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

I would downvote this change in a patch release, due to the potential to cause an upgrade issue. I would make a much simpler change here:

@@ -34,7 +34,7 @@ var handlers = {
             // a later plugins release.  This is for legacy plugins to work with Cordova.
 
             if (options && options.android_studio === true) {
-                dest = studioPathRemap(obj);
+                dest = getInstallDestination(obj);
             }
 
             if (options && options.force) {


if (options && options.force) {
copyFile(plugin.dir, obj.src, project.projectDir, dest, !!(options && options.link));
Expand All @@ -44,11 +37,8 @@ var handlers = {
}
},
uninstall: function (obj, plugin, project, options) {
var dest = path.join(obj.targetDir, path.basename(obj.src));

if (options && options.android_studio === true) {
dest = studioPathRemap(obj);
}
var dest = getInstallDestination(obj);
Copy link

Choose a reason for hiding this comment

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

Same comment as I made on line 31. Here is a much simpler change:

@@ -47,7 +47,7 @@ var handlers = {
             var dest = path.join(obj.targetDir, path.basename(obj.src));
 
             if (options && options.android_studio === true) {
-                dest = studioPathRemap(obj);
+                dest = getInstallDestination(obj);
             }
 
             // TODO: Add Koltin extension to uninstall, since they are handled like Java files

Copy link
Member Author

Choose a reason for hiding this comment

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

if (options && options.android_studio === true) is going to be true always, so there is no point on keeping it


// TODO: Add Koltin extension to uninstall, since they are handled like Java files
if (obj.src.endsWith('java')) {
Expand Down Expand Up @@ -317,6 +307,11 @@ function generateAttributeError (attribute, element, id) {
return 'Required attribute "' + attribute + '" not specified in <' + element + '> element from plugin: ' + id;
}

function getInstallDestination (obj) {
return studioPathRemap(obj) ||
path.join(obj.targetDir, path.basename(obj.src));
}

function studioPathRemap (obj) {
// If any source file is using the app new directory structure,
// don't penalize it
Expand Down