GitHub user ktop opened a pull request:

https://github.com/apache/cordova-lib/pull/432

CB-11023 Add attribute through config-file tag

Add functionality to be able to add attributes to xml files through config-file tag.

Syntax:
Able to add multiple attributes to multiple elements at once (only to direct children of the parent).
```
<config-file target="AndroidManifest.xml" parent="/manifest" attr="true">
<application android:name="MyApplication" android:isGame="true" />
<uses-sdk android:maxSdkVersion="22" />
</config-file>
```

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ktop/cordova-lib cb-11023

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cordova-lib/pull/432.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #432

commit aa3427ce38ef0bd2e2ac71e5d56d7d9d0dc6b15c
Author: Karen Tran <ktop500@gmail.com>
Date: 2016-04-22T14:05:12Z

CB-11023 Add attribute through config-file tag

Add back comment and fix format

Github user macdonst commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-213519895

@ktop I did a quick test of this with one of my existing apps and it does add the following lines to AndroidManifest.xml:

```
<application android:isGame="true" android:name="MyApplication" />
<uses-sdk android:maxSdkVersion="22" />
```

but unfortunately it already had the following lines:

```
<application android:hardwareAccelerated="true" android:icon="@drawable/icon" android:label="@string/app_name" android:supportsRtl="true">
<uses-sdk android:minSdkVersion="14" android:targetSdkVersion="23" />
```

So in order for this to get merged it has to be able to modify existing lines instead of adding new ones.

Github user ktop commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-213530432

If we can modify existing attributes, and then the plugin gets removed, the modified attribute will also get removed. I don't think there is a way to revert it back to what it was before. Is that ok?

Github user codecov-io commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-215478064

1. 1. [Current coverage][cc-pull] is *79.60%*
      > Merging 432[cc-pull] into [master][cc-base-branch] will increase coverage by *-0.49%*

```diff
@@ master #432 diff @@
========================================
Files 69 69
Lines 5029 5398 +369
Methods 0 865 +865
Branches 972 1017 +45
========================================
+ Hits 4029 4297 +268

• Misses 995 1101 +106
  + Partials 5 0 -5
  ```

1. 4 files in `...dova-lib/src/cordova` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/352d71395621245f691ec6143290ccd7774cbae9/changes?src=pr#636F72646F76612D6C69622F7372632F636F72646F7661)

• Misses `+1`
• Hits `+13`
  1. 2 files (not in diff) in `...lib/src/util/windows` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/352d71395621245f691ec6143290ccd7774cbae9/changes?src=pr#636F72646F76612D6C69622F7372632F7574696C2F77696E646F7773)
• Misses `+6`
• Partials `-1`
• Hits `+24`
  1. 2 files (not in diff) in `cordova-lib/src/util` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/352d71395621245f691ec6143290ccd7774cbae9/changes?src=pr#636F72646F76612D6C69622F7372632F7574696C)
• Misses `+4`
• Hits `+1`
  1. 5 files (not in diff) in `...lib/src/plugman/util` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/352d71395621245f691ec6143290ccd7774cbae9/changes?src=pr#636F72646F76612D6C69622F7372632F706C75676D616E2F7574696C)
• Misses `-1`
• Hits `+17`
  1. 7 files (not in diff) in `...rc/plugman/platforms` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/352d71395621245f691ec6143290ccd7774cbae9/changes?src=pr#636F72646F76612D6C69622F7372632F706C75676D616E2F706C6174666F726D73)
• Misses `+32`
• Hits `+63`
  1. 7 files (not in diff) in `...dova-lib/src/plugman` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/352d71395621245f691ec6143290ccd7774cbae9/changes?src=pr#636F72646F76612D6C69622F7372632F706C75676D616E)
• Misses `+22`
• Hits `+36`
  1. 3 files (not in diff) in `...ordova-lib/src/hooks` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/352d71395621245f691ec6143290ccd7774cbae9/changes?src=pr#636F72646F76612D6C69622F7372632F686F6F6B73)
• Misses `+2`
• Hits `+2`
  1. 9 files (not in diff) in `...src/cordova/metadata` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/352d71395621245f691ec6143290ccd7774cbae9/changes?src=pr#636F72646F76612D6C69622F7372632F636F72646F76612F6D65746164617461)
• Misses `+3`
• Hits `+12`
  1. 10 files (not in diff) in `...dova-lib/src/cordova` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/352d71395621245f691ec6143290ccd7774cbae9/changes?src=pr#636F72646F76612D6C69622F7372632F636F72646F7661)
• Misses `+31`
• Partials `-2`
• Hits `+78`
  1. 2 files (not in diff) in `cordova-lib/src` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/352d71395621245f691ec6143290ccd7774cbae9/changes?src=pr#636F72646F76612D6C69622F737263)
• Misses `+6`
• Partials `-2`
• Hits `+18`

> Powered by [Codecov](https://codecov.io?src=pr). Last updated by 352d713
[cc-base-branch]: https://codecov.io/gh/apache/cordova-lib/branch/master?src=pr
[cc-pull]: https://codecov.io/gh/apache/cordova-lib/pull/432?src=pr

Github user macdonst commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-215811240

@ktop Yeah, I have the `attr="true"` in my plugin.xml and it still gets added instead of merged.

```
<config-file target="AndroidManifest.xml" parent="/manifest" attr="true">
<application android:name="MyApplication" android:isGame="true" />
<uses-sdk android:maxSdkVersion="22" />
</config-file>
```

I'm not sure why it's not working for me 😢

Regardless, I have some other comments. What if I put config-file statements in config.xml? Which change takes precedence?

Also, instead of using `attr="true"` would it make more sense to use a more granular approach like `mode="add|replace|merge|delete"` where:

• add will append to the inner xml of the parent
• replace will completely overwrite the parent's inner xml with your declaration
• merge will attempt to find elements of the same name and merge their attributes
• delete will search for elements matching the specified name and attributes and delete them

Github user ktop commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-215857823

@macdonst
I had Edna test the first time around before I opened the PR and she was able to get it working, so that is weird that it is adding a new element for you.

Was config-file functional in config.xml before? Or was that something to be done as part of this jira issue? I was only focused on getting it working in plugin.xml, so I can work on it if it isn't implemented yet. Should config.xml handle config-file during prepare?

The mode suggestion sounds good. It will give users a lot of utility for modifying xml files.
I have just few clarification questions:

• If no mode is specified, is the default to add?
• For replace, if there is nothing to replace, should it still try to add?
• And for delete, should it delete an element even if the element has children?

Github user riknoll commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-215866756

@ktop config-file wasn't in config.xml before, but we wanted to add it because the number of config.xml attributes that just edit various native xml files is getting unsustainable. I think that config.xml's config-file modifications should happen during the same step as the plugins' modifications do, but they should definitely be applied after all of the plugin.xml ones. That way, the app developer always has the option to override whatever the plugins are doing.

I think the mode flag is a good idea, but I worry about allowing it in plugin.xml. If plugins are able to do things other than add elements, does that mean we have the opportunity for plugins to overwrite each other's modifications depending on the order in which their config-file attributes are resolved? That might get weird. Definitely makes sense to have it in config.xml though.

Github user purplecabbage commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-215892244

I would put the mode flag as out-of-scope for now, we can re-asses later if need be.

I could not get this to work, can you please post instructions for verifying it?
My steps were:
```
cordova create newproject
cordova platform add android
// modified the root config.xml with the fragment from above
cordova prepare
// checked androidmanifest.xml for changes, none seen
```

Github user macdonst commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-215915464

@ktop @riknoll yeah, config.xml should be part of this. Anything in config.xml should trump whatever is specified in the plugin.xml. If we do modes then default mode would be `add`, if nothing to `replace` then it becomes an `add` and `delete` could take out the entire element. We can work on flags later but getting attribute level manipulation into config.xml and plugin.xml parsing would be amazing.

@purplecabbage you need to add:

```
<config-file target="AndroidManifest.xml" parent="/manifest" attr="true">
<application android:name="MyApplication" android:isGame="true" />
<uses-sdk android:maxSdkVersion="22" />
</config-file>
```

into the plugin.xml of a plugin you are including in the project. So...

```
cordova create newproject
cordova platform add android
cordova plugin add cordova-plugin-one-I-just-made-up
// modified the plugin.xml with the fragment from above
cordova prepare
// checked androidmanifest.xml for changes
```

Github user purplecabbage commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-216386608

I too am seeing @macdonst 's issue.
```
...
</application>
<uses-sdk android:minSdkVersion="14" android:targetSdkVersion="23" />
<application android:isGame="true" android:name="MyApplication" />
<uses-sdk android:maxSdkVersion="22" />
</manifest>
```

Github user purplecabbage commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-216424469

@ktop Can you provide the steps you used to verify that this is working please?

What I did was create a new project, and added the cordova-plugin-device
I then modified the plugin.xml to include your additions, and then added android and looked for the changes to show up in androidmanifest.xml

Github user ktop commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-216543951

@purplecabbage
My workspace is working off of the master branches with the tools npm linked.

I modified the plugin.xml of cordova-plugin-device and added:
```
<config-file target="AndroidManifest.xml" parent="/manifest" attr="true">
<application android:name="MyApplication" android:isGame="true" />
<uses-sdk android:maxSdkVersion="22" />
</config-file>
```

Then for the project:
```
cordova create myApp
cd myApp
cordova platform add ../cordova-android
cordova plugin add ../cordova-plugin-device
```
I check AndroidManifest.xml and the new attributes are there as expect, without a new tag being added.

I'll try testing this PR on another machine and see what result I get.

As for getting config-file tag into config.xml, I've been caught up in some other work so will get to this sometime later in the week.

Github user ktop commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-216629805

@macdonst @purplecabbage
On my other machine (Mac), I see the same behavior that you both were seeing. I was able to fix it by doing a `cordova-coho/coho npm-link` and re-adding the android platform to the project. What was happening was the cordova-common module inside of cordova-android wasn't being linked to the one in cordova-lib, so it was using a cordova-common without my changes in it, thus using the default functionality of config-file.

On a side note (this only happens for me on Mac, works fine on Windows), when coho replaces the cordova-common module with a symlink, this was treated as deleting the cordova-common module, then when I go to add the android platform, I get an error saying `Cannot find module 'cordova-common'` and when I checked the node modules folder, there's only a symlink file of cordova-common. I'm not really sure how symlinks are supposed to work on Mac since I mainly use a Windows machine. The project still runs and cordova-common is actually linked. I'm not sure if this is a Mac issue or something else.

Github user macdonst commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-217228255

@ktop thanks for the hint about running `cordova-coho/coho npm-link`. Once I did I was able to get your code to run. It seems to be working pretty good to me. So I would love to get this into cordova ASAP.

Folks do you think we should merge this PR then look at changes to config.xml or land it all in one PR?

Github user ktop commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-217972581

@macdonst I've added the implementation for config-file to config.xml just to get it working, but I do think it should be refactored (cordova-common/src/ConfigChanges/ConfigChanges.js). I really just wanted to rename `add_plugin_changes` to something more general but that would mean I have to do refactoring in several other repos. I'll take some suggestions on how to refactor, but for now, I just made a similar function that does almost the same thing, `add_config_changes`.

This config-file will be handled right after platform prepare happens, which is called on every prepare and plugin install/uninstall. I think this should handle the case for config.xml overriding plugin.xml.

Github user nikhilkh commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/432#discussion_r63604254

— Diff: cordova-lib/src/cordova/prepare.js —
@@ -116,6 +117,13 @@ function preparePlatforms (platformList, projectRoot, options)

+ })
+ .then(function () {
+ // Handle config-file in config.xml
+ var platformRoot = path.join(projectRoot, 'platforms', platform);
+ var platformJson = PlatformJson.load(platformRoot, platform);
+ var munger = new PlatformMunger(platform, platformRoot, platformJson);
+ munger.add_config_changes(project.projectConfig, /should_increment=/true).save_all();
— End diff –

This API cannot be used until cordova-common is released. This needs to go in a separate PR.

Github user riknoll commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-220179183

@ktop I have a couple questions. First, do we have a plan for dealing with plugins that have conflicting attribute edits? For example, if plugin A has this in its plugin.xml:

```xml
<config-file target="AndroidManifest.xml" parent="/manifest" attr="true">
<uses-sdk android:maxSdkVersion="22" />
</config-file>
```

and plugin B has this in its plugin.xml:

```xml
<config-file target="AndroidManifest.xml" parent="/manifest" attr="true">
<uses-sdk android:maxSdkVersion="17" />
</config-file>
```

Will the user get a build error?

Second, is it possible to reference a sibling of the same tag type? For example, given this in my AndroidManifest.xml:

```xml
<application android:icon="@drawable/icon" android:label="@string/app_name">
<activity
android:label="@string/activity_name"
android:name="MainActivity" />
<activity
android:label="@string/second_activity"
android:name="SecondActivity" />
</application>
```

If I wanted to add an attribute in the second Activity, how would I do so? This isn't a real-life example, but you get the idea.

Github user vladimir-kotikov commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-220254686

I think there is no mechanism for resolving conflicts, as there was no need for it before.

One idea on how to deal w/ conflicts is to fail by default in case of conflicting attributes changes made by plugins, because overwriting the attributes possibly would lead to unexpected errors at build and run time. However, the user once notified with appropriate error message still could install the conflicting plugin using `--force` flag.

Also overriding attributes from `config.xml` is under user control, and should always be allowed (we might want to show warning in this case though).

Github user riknoll commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-220386776

@vladimir-kotikov yep, my thought's exactly! We definitely need to handle plugins conflicting with each other because any bugs caused by those conflicts would likely be extremely difficult to debug/trace. +1 to the config.xml warning as well

Github user ktop commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/432#discussion_r63922339

— Diff: cordova-lib/src/cordova/prepare.js —
@@ -116,6 +117,13 @@ function preparePlatforms (platformList, projectRoot, options)

+ })
+ .then(function () {
+ // Handle config-file in config.xml
+ var platformRoot = path.join(projectRoot, 'platforms', platform);
+ var platformJson = PlatformJson.load(platformRoot, platform);
+ var munger = new PlatformMunger(platform, platformRoot, platformJson);
+ munger.add_config_changes(project.projectConfig, /should_increment=/true).save_all();
— End diff –

Thanks for reviewing, I'll make the change.

Github user ktop commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-220440531

@riknoll Vladimir is correct, there is no mechanism for either yet.

I think warnings are good, but does that mean the plugins that get installed first get first dibs to modifying xml elements? How do we go about keeping track of whether or not an attribute/element was modified or not?

For the second question, that has actually crossed my mind a few times while testing (how I couldn't modify the second activity tag or add children to it). I'm not really sure the best way to handle it. I've thought about adding an identifier attribute to the config-file tag, but that would limit config-file to modify only one element.

```xml
<config-file target="AndroidManifest.xml" parent="/manifest/application" identifierName="android:name" identifierValue="SecondActivity" mode="merge" >
<activity android:configChanges="orientation|keyboardHidden|screenSize" />
</config-file>
```

End goal would be to be able to modify both activity tags in the same config-file tag, but that may be a stretch.

Github user riknoll commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-220467601

I think that the issue might be that we need to separate inserting child elements and editing existing ones. Something like this:

```xml
<!-- Editing attributes in existing elements -->
<edit-config target="xpath/nonsense" mode="merge|overwrite">
<attribute name="attr" new-value="edited" />
</edit-config>

<!-- Existing config-file insert behavior -->
<insert-config parent="xpath/nonsense">
<child-element />
</insert-config>
```

This is a bit more verbose if you have a lot of edits that need to be performed, but much more flexible because the xpath selector can be used to reference specific elements, even in lists. It might also help with detecting plugin conflicts. I don't care about the names of those tags (we can keep using config-file if we want).

For plugin conflicts, detection should probably happen at "plugin add" time. The add should fail on conflict and allow the user to override it using `--force` if they want to. As for which plugin gets dibs in the forced add scenario, it doesn't really matter as long as it is consistent. Hopefully the user can determine the correct value on conflict and fix it by editing the config from the project's `config.xml`.

Github user ktop commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-220647997

I'd like to clear up some confusion I have on adding attributes.
Before, we had decided on going with this syntax where the child elements of config-file tag would point to the child tags of the parent we want to add attributes to, so that we could add attributes to more than one child in a single config-file tag.

With the following syntax, I wouldn't be able to at add to the second activity.
```xml
<config-file target="AndroidManifest.xml" parent="/manifest/application" mode="merge" >
<activity android:enabled="true" />
</config-file>
```

Now with your suggestion, the tag would look like this:
Here, I would be able to specify the second activity, but this contradicts with what we had before since activity tag isn't a child of the parent.
```xml
<config-file target="AndroidManifest.xml" parent="/manifest/application/activity[@android:name='SecondActivity']" mode="merge" >
<activity android:enabled="true" />
</config-file>
```

Would we lose the ability to add attributes to more than one child? Or should we try to do both? Detect that we want to add to the tag that is the same as the parent as well as child tags. (Not sure if this makes sense)
```xml
<config-file target="AndroidManifest.xml" parent="/manifest/application/activity[@android:name='SecondActivity']" mode="merge" >
<activity android:enabled="true" />
<intent-filter android:priority="1" />
</config-file>
```
Or any other ideas?

Github user riknoll commented on the pull request:

https://github.com/apache/cordova-lib/pull/432#issuecomment-220658702

My suggestion was to separate editing and inserting altogether. Continue to have a `config-file` tag that works exactly like before for adding things to XML, and have a new differently named tag for editing existing tags (like `edit-config`). The new tag wouldn't specify a parent, it would use the xpath selector to specify the element to be edited directly. We do lose the ability to edit a bunch of tags all at once like you say, but I don't think it would be too cumbersome to add a separate element for each edit we want to make.

Github user ktop commented on the issue:

https://github.com/apache/cordova-lib/pull/432

I'm gonna open a new pull request with new changes. Closing this one....

Github user ktop closed the pull request at:

https://github.com/apache/cordova-lib/pull/432

GitHub user ktop opened a pull request:

https://github.com/apache/cordova-lib/pull/449

CB-11023 Add edit-config functionality

New edit-config tag for plugin.xml will allow users to modify xml attributes.
Ex. Assumes your AndroidManifest.xml has a second activity element with attribute android:name="SecondActivity"
```xml
<!-- Will modify first activity -->
<edit-config file="AndroidManifest.xml" target="/manifest/application/activity" mode="merge">
<activity android:enabled="true" android:configChanges="orientation|keyboardHidden" />
</edit-config>
<!-- Will modify second activity -->
<edit-config file="AndroidManifest.xml" target="/manifest/application/activity[@android:name='SecondActivity']" mode="overwrite">
<activity android:enabled="true" android:name="SecondActivity" />
</edit-config>
```
file: specifies the file location
target: specifies an xpath to the element that you want to modify
modes:

• merge: add attributes in the target with the ones specified by edit-config and will replace if the attribute names are the same
• overwrite: replace all of the attributes at the target with the one specified by edit-config

children: will only modify one element per edit-config tag

There is conflict checking now....if a plugin wants to modify an attribute another plugin has already modified, an error will be thrown and plugin install will fail. The user must fix the conflict or they can use --force to force add the plugin and overwrite the conflict.

Lastly, on plugin uninstall, the plugin should restore the attributes to the state it was before installing.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ktop/cordova-lib editconfig

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cordova-lib/pull/449.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #449

commit a3864929400f61fc31d03c3bab1d91a9467a3fbf
Author: unknown <ktop500@gmail.com>
Date: 2016-05-26T21:36:23Z

CB-11023 Add edit-config functionality

Github user codecov-io commented on the issue:

https://github.com/apache/cordova-lib/pull/449

1. 1. [Current coverage][cc-pull] is *80.56%*
      > Merging 449[cc-pull] into [master][cc-base-branch] will not change coverage

```diff
@@ master #449 diff @@
==========================================
Files 68 68
Lines 5387 5387
Methods 851 851
Messages 0 0
Branches 1042 1042
==========================================
Hits 4340 4340
Misses 1047 1047
Partials 0 0
```

> Powered by [Codecov](https://codecov.io?src=pr). Last updated by [ca98abf...8c2551d][cc-compare]
[cc-base-branch]: https://codecov.io/gh/apache/cordova-lib/branch/master?src=pr
[cc-compare]: https://codecov.io/gh/apache/cordova-lib/compare/ca98abf66ec573873ef0609b42926df7c804f564...8c2551d96d509419dd2dc780b37e0aaf8f2a1529
[cc-pull]: https://codecov.io/gh/apache/cordova-lib/pull/449?src=pr

Github user riknoll commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/449#discussion_r66486389

— Diff: cordova-common/src/ConfigChanges/ConfigChanges.js —
@@ -125,12 +128,32 @@ function remove_plugin_changes(pluginInfo, is_top_level) {

PlatformMunger.prototype.add_plugin_changes = add_plugin_changes;
-function add_plugin_changes(pluginInfo, plugin_vars, is_top_level, should_increment) {
+function add_plugin_changes(pluginInfo, plugin_vars, is_top_level, should_increment, plugin_force) {
var self = this;
var platform_config = self.platformJson.root;
+ var editConfigChanges = pluginInfo.getEditConfigs(self.platform);
+ var config_munge;

• // get config munge, aka how should this plugin change various config files
• var config_munge = self.generate_plugin_config_munge(pluginInfo, plugin_vars);
  + if (!editConfigChanges || editConfigChanges.length === 0) { + // get config munge, aka how should this plugin change various config files + config_munge = self.generate_plugin_config_munge(pluginInfo, plugin_vars); + }

  + else if (plugin_force) {
  + CordovaLogger.get().log(CordovaLogger.WARN, '--force is used. edit-config will overwrite any conflicts');

  • • End diff –

I would add "conflicting plugins may not work as expected" to the end of this warning (or something to that effect)

Github user riknoll commented on the issue:

https://github.com/apache/cordova-lib/pull/449

@ktop the author name on the commit is "unknown"

Github user riknoll commented on the issue:

https://github.com/apache/cordova-lib/pull/449

I've tested it out and I think there needs to be a slight tweak to the `--force` behavior. When a plugin is force added, any conflicting edit-config changes it has should not be applied. Otherwise, you can get into a weird situation where adding conflicting plugins and removing them in a bad order results in the config file being different than when you started.

For example, If I have two plugins that conflict and I execute the following commands:
```
cordova plugin add plugin-1
cordova plugin add plugin-2 --force
cordova plugin rm plugin-1
cordova plugin rm plugin-2
```
I will be left with a config file that has the changes that `plugin-1` made despite `plugin-1` no longer being in my project. I think it's a good idea to always make it so that removing all plugins will get you back to where you started.

Github user ktop commented on the issue:

https://github.com/apache/cordova-lib/pull/449

@riknoll
Where does it say "unknown"? I don't see it.

If we have plugin-1, plugin-2 --force, and then add plugin-3 with --force, plugin-1 and plugin-2 should not be applied correct?

Github user riknoll commented on the issue:

https://github.com/apache/cordova-lib/pull/449

@ktop Run `git log` and check out the author field. Your email shows up, but the name is unknown for some reason.

As for the force add thing, that sounds fine to me. There are two possible behaviors:

1. When you force add a plugin, remove any existing changes (i.e. the changes of plugin 1) and apply the new changes (from plugin 2)
2. When you force add a plugin, ignore any conflicting changes (i.e. the changes of plugin 2) and leave the existing ones in place (from plugin 1)

I believe you are describing the first behavior. The difference between that and the current implementation is that the current implementation shouldn't store both sets of changes in platform.json. If the changes of plugin 1 get overwritten by plugin 2, then they should be undone before the changes of plugin 2 are made. Otherwise, the platform.json gets into a weird state.

Github user macdonst commented on the issue:

https://github.com/apache/cordova-lib/pull/449

@ktop I also see "unknown" for your name in `git log`.

Github user ktop commented on the issue:

https://github.com/apache/cordova-lib/pull/449

ahh must be because I didn't set up my git config after my computer got re-imaged. I have it set now and hopefully it will appear after I rebase.

Github user ktop commented on the issue:

https://github.com/apache/cordova-lib/pull/449

My latest commit will find all conflicts when --force is used and remove them before adding the new plugin. @riknoll or @macdonst can you try it out and let me know if you have any issues?

Also looks like my git name got fixed in the latest commit, so I think after I rebase and squash, it'll fix the first commit.

Github user riknoll commented on the issue:

https://github.com/apache/cordova-lib/pull/449

This is working fine for me! When this feature gets documented, we need to make sure to document how to get your project back in order if you mess it up by adding a plugin with `--force`. I think removing all plugins and re-adding them (minus the conflicting ones) should do the trick.

I'll wait for @macdonst to take a look before I merge this. This change will require a platform release as well since it is in cordova-common.

Github user macdonst commented on the issue:

https://github.com/apache/cordova-lib/pull/449

LGTM! Sorry for taking so long. My workspace is in a bad way and I think I need to kill it with fire and setup the whole thing from scratch.

Once this gets into a release I have about 4 plugins I will modify to use this approach.

Github user ktop commented on the issue:

https://github.com/apache/cordova-lib/pull/449

Cool, let me rebase

Github user ktop commented on the issue:

https://github.com/apache/cordova-lib/pull/449

@riknoll I've rebased so this should be ready for merge.

Commit 5fb8dffb2dc92e4d08d286432f23299da0e81249 in cordova-lib's branch refs/heads/master from Karen Tran
[ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=5fb8dff ]

CB-11023 Add edit-config functionality

This closes #449

Github user asfgit closed the pull request at:

https://github.com/apache/cordova-lib/pull/449

Github user riknoll commented on the issue:

https://github.com/apache/cordova-lib/pull/449

Done! Sorry for the delay. @ktop can you open a PR documenting this feature in the [plugin.xml reference](https://github.com/apache/cordova-docs/blob/master/www/docs/en/dev/plugin_ref/spec.md) as well?

Github user ktop commented on the issue:

https://github.com/apache/cordova-lib/pull/449

@riknoll yea sure. Are there instructions on how/where documentation goes?

GitHub user ktop opened a pull request:

https://github.com/apache/cordova-docs/pull/614

CB-11023 New edit-config tag in plugin.xml

Documenting the new edit-config tag in plugin.xml.
@riknoll can you review? I tried to keep the examples short, but explain enough to get the point across. Let me know if I need to fix anything.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ktop/cordova-docs cb-11023

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cordova-docs/pull/614.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #614

commit 1d8ec834eb5d157ce779cf7517b6670c1ad36a16
Author: ktop <ktop500@gmail.com>
Date: 2016-06-23T14:24:09Z

CB-11023 New edit-config tag in plugin.xml

Github user macdonst commented on the issue:

https://github.com/apache/cordova-docs/pull/614

@ktop looks good but my personal preference would be to show a before and after of the Android Manifest.xml you use in your example to really hammer home how it works.

Github user riknoll commented on the issue:

https://github.com/apache/cordova-docs/pull/614

For the examples, I would just give a snippet of the AndroidMainfest and not the whole thing. There's a lot that doesn't change in the before and after. Secondly, the example of force adding plugins is pretty good, but I wonder if this is the right place for it. Perhaps it is better suited for the CLI guide as a sub-heading under the [plugin command](https://github.com/apache/cordova-cli/blob/master/doc/readme.md#cordova-plugin-command). People who get CLI errors might be more likely to look there since they are probably not plugin authors themselves.

Github user ktop commented on the issue:

https://github.com/apache/cordova-docs/pull/614

I shortened the AndroidManifest to snippets.

I could see the force add content going into both locations, but in the CLI guide it should be more describing the error about conflicting edit-config and how it is caused. Maybe this section can link to the plugin.xml guide about edit-config for how to resolve the error. It's probably worth it for users to learn about edit-config in the plugin.xml guide before making changes.

Github user riknoll commented on a diff in the pull request:

https://github.com/apache/cordova-docs/pull/614#discussion_r70540748

— Diff: www/docs/en/dev/plugin_ref/spec.md —
@@ -365,6 +365,111 @@ For windows-specific attributes:
```
The above example will set pre-8.1 platforms (Windows 8, specifically) to require the `webcam` device capability and the `picturesLibrary` general capability, and apply the `webcam` device capability only to Windows 8.1 projects that build for Windows Phone. Windows desktop 8.1 systems are unmodified.

+### edit-config
+Similar to `config-file`, `edit-config` identifies an XML-based configuration file to be modified, where in that document the modification should take place, and what should be modified. Instead of appending new children to an XML document tree, `edit-config` makes modifications to attributes of XML elements. There are two modes which will determine what type of attribute modification will be made, `merge` or `overwrite`. `edit-config` has one child and that child will contain the attributes to be added.
+
+Attributes(type) <br/> <span class="sub-header">Only for platform:</span> | Description
+---------------- | ------------
+file(string) | The file to be modified, and the path relative to the root of the Cordova project. If the specified file does not exist, the tool ignores the configuration change and continues installation. <br/> The target can include wildcard (`*`) elements. In this case, the CLI recursively searches through the project directory structure and uses the first match. <br/> On iOS, the location of configuration files relative to the project directory root is not known, so specifying a target of `config.xml` resolves to `cordova-ios-project/MyAppName/config.xml`.
+target(string) | An XPath selector referencing the target element to make attribute modifications to. If you use absolute selectors, you can use a wildcard (``) to specify the root element, e.g., `//plugins`. If the selector does not resolve to a child of the specified document, the tool stops and reverses the installation process, issues a warning, and exits with a non-zero code.
+mode(string) | The mode that determines what type of attribute modifications will be made. <br/> `merge` - Adds the specified attributes to the target element. Will replace the attribute values if the specified attributes already exist in the target element. <br/> `overwrite` - Replaces all attributes in the target element with the attributes specified.
+
+Example:
+
+```xml
+<!-- plugin-1 -->
+<edit-config file="AndroidManifest.xml" target="/manifest/uses-sdk" mode="merge">
+ <uses-sdk android:minSdkVersion="16" android:maxSdkVersion="23" />
+</edit-config>
+<edit-config file="AndroidManifest.xml" target="/manifest/application/activity[@android:name='MainActivity']" mode="overwrite">
+ <activity android:name="MainActivity" android:label="NewLabel" android:configChanges="orientation|keyboardHidden" />
+</edit-config>
+```
+
+AndroidManifest.xml before adding plugin-1:
+```xml
+<manifest android:hardwareAccelerated="true" android:versionCode="1" android:versionName="0.0.1" package="io.cordova.hellocordova" xmlns:android="http://schemas.android.com/apk/res/android">
+ ...
+ <activity android:configChanges="orientation|keyboardHidden|keyboard|screenSize|locale" android:label="@string/activity_name" android:launchMode="singleTop" android:name="MainActivity" android:theme="@android:style/Theme.DeviceDefault.NoActionBar" android:windowSoftInputMode="adjustResize">
+ ...
+ </activity>
+ ...
+ <uses-sdk android:minSdkVersion="14" android:targetSdkVersion="23" />
+</manifest>
+```
+
+AndroidManifest.xml after adding plugin-1:
+```xml
+<manifest android:hardwareAccelerated="true" android:versionCode="1" android:versionName="0.0.1" package="io.cordova.hellocordova" xmlns:android="http://schemas.android.com/apk/res/android">
+ ...
+ <activity android:configChanges="orientation|keyboardHidden" android:label="NewLabel" android:name="MainActivity">
+ ...
+ </activity>
+ ...
+ <uses-sdk android:maxSdkVersion="23" android:minSdkVersion="16" android:targetSdkVersion="23" />
+</manifest>
+```
+
+Multiple plugins will not be able to to modify the same attributes because it may cause problems with the application. An error will be thrown and plugin install will fail. The conflicting `edit-config` tags must be resolved before the plugin can be added. Make modifications to conflicting tags to resolve the conflict, then remove and re-add those updated plugins.
— End diff –

"Multiple plugins will not be able to to modify " => "Multiple plugins can not modify"

"then remove and re-add those updated plugins" => "then remove and re-add the updated plugins"

Github user riknoll commented on a diff in the pull request:

https://github.com/apache/cordova-docs/pull/614#discussion_r70540822

— Diff: www/docs/en/dev/plugin_ref/spec.md —
@@ -365,6 +365,111 @@ For windows-specific attributes:
```
The above example will set pre-8.1 platforms (Windows 8, specifically) to require the `webcam` device capability and the `picturesLibrary` general capability, and apply the `webcam` device capability only to Windows 8.1 projects that build for Windows Phone. Windows desktop 8.1 systems are unmodified.

+### edit-config
+Similar to `config-file`, `edit-config` identifies an XML-based configuration file to be modified, where in that document the modification should take place, and what should be modified. Instead of appending new children to an XML document tree, `edit-config` makes modifications to attributes of XML elements. There are two modes which will determine what type of attribute modification will be made, `merge` or `overwrite`. `edit-config` has one child and that child will contain the attributes to be added.
+
+Attributes(type) <br/> <span class="sub-header">Only for platform:</span> | Description
+---------------- | ------------
+file(string) | The file to be modified, and the path relative to the root of the Cordova project. If the specified file does not exist, the tool ignores the configuration change and continues installation. <br/> The target can include wildcard (`*`) elements. In this case, the CLI recursively searches through the project directory structure and uses the first match. <br/> On iOS, the location of configuration files relative to the project directory root is not known, so specifying a target of `config.xml` resolves to `cordova-ios-project/MyAppName/config.xml`.
+target(string) | An XPath selector referencing the target element to make attribute modifications to. If you use absolute selectors, you can use a wildcard (``) to specify the root element, e.g., `//plugins`. If the selector does not resolve to a child of the specified document, the tool stops and reverses the installation process, issues a warning, and exits with a non-zero code.
+mode(string) | The mode that determines what type of attribute modifications will be made. <br/> `merge` - Adds the specified attributes to the target element. Will replace the attribute values if the specified attributes already exist in the target element. <br/> `overwrite` - Replaces all attributes in the target element with the attributes specified.
+
+Example:
+
+```xml
+<!-- plugin-1 -->
+<edit-config file="AndroidManifest.xml" target="/manifest/uses-sdk" mode="merge">
+ <uses-sdk android:minSdkVersion="16" android:maxSdkVersion="23" />
+</edit-config>
+<edit-config file="AndroidManifest.xml" target="/manifest/application/activity[@android:name='MainActivity']" mode="overwrite">
+ <activity android:name="MainActivity" android:label="NewLabel" android:configChanges="orientation|keyboardHidden" />
+</edit-config>
+```
+
+AndroidManifest.xml before adding plugin-1:
+```xml
+<manifest android:hardwareAccelerated="true" android:versionCode="1" android:versionName="0.0.1" package="io.cordova.hellocordova" xmlns:android="http://schemas.android.com/apk/res/android">
+ ...
+ <activity android:configChanges="orientation|keyboardHidden|keyboard|screenSize|locale" android:label="@string/activity_name" android:launchMode="singleTop" android:name="MainActivity" android:theme="@android:style/Theme.DeviceDefault.NoActionBar" android:windowSoftInputMode="adjustResize">
+ ...
+ </activity>
+ ...
+ <uses-sdk android:minSdkVersion="14" android:targetSdkVersion="23" />
+</manifest>
+```
+
+AndroidManifest.xml after adding plugin-1:
+```xml
+<manifest android:hardwareAccelerated="true" android:versionCode="1" android:versionName="0.0.1" package="io.cordova.hellocordova" xmlns:android="http://schemas.android.com/apk/res/android">
+ ...
+ <activity android:configChanges="orientation|keyboardHidden" android:label="NewLabel" android:name="MainActivity">
+ ...
+ </activity>
+ ...
+ <uses-sdk android:maxSdkVersion="23" android:minSdkVersion="16" android:targetSdkVersion="23" />
+</manifest>
+```
+
+Multiple plugins will not be able to to modify the same attributes because it may cause problems with the application. An error will be thrown and plugin install will fail. The conflicting `edit-config` tags must be resolved before the plugin can be added. Make modifications to conflicting tags to resolve the conflict, then remove and re-add those updated plugins.
+
+There is an option for those who are certain that the plugin should be installed despite the conflicts. The `-force` flag can be used with `cordova plugin add`. Force adding the plugin will revert conflicting changes of other plugins so that it can be added without issues. `-force` should be used with caution as reverting changes of other plugins may cause the application to not work as expected.
+
+If the plugins ever get in a weird state, remove all plugins and re-add them.
+
+Example:
— End diff –

Maybe make this a sub-header?

Github user riknoll commented on a diff in the pull request:

https://github.com/apache/cordova-docs/pull/614#discussion_r70540948

— Diff: www/docs/en/dev/plugin_ref/spec.md —
@@ -365,6 +365,111 @@ For windows-specific attributes:
```
The above example will set pre-8.1 platforms (Windows 8, specifically) to require the `webcam` device capability and the `picturesLibrary` general capability, and apply the `webcam` device capability only to Windows 8.1 projects that build for Windows Phone. Windows desktop 8.1 systems are unmodified.

+### edit-config
+Similar to `config-file`, `edit-config` identifies an XML-based configuration file to be modified, where in that document the modification should take place, and what should be modified. Instead of appending new children to an XML document tree, `edit-config` makes modifications to attributes of XML elements. There are two modes which will determine what type of attribute modification will be made, `merge` or `overwrite`. `edit-config` has one child and that child will contain the attributes to be added.
+
+Attributes(type) <br/> <span class="sub-header">Only for platform:</span> | Description
+---------------- | ------------
+file(string) | The file to be modified, and the path relative to the root of the Cordova project. If the specified file does not exist, the tool ignores the configuration change and continues installation. <br/> The target can include wildcard (`*`) elements. In this case, the CLI recursively searches through the project directory structure and uses the first match. <br/> On iOS, the location of configuration files relative to the project directory root is not known, so specifying a target of `config.xml` resolves to `cordova-ios-project/MyAppName/config.xml`.
+target(string) | An XPath selector referencing the target element to make attribute modifications to. If you use absolute selectors, you can use a wildcard (``) to specify the root element, e.g., `//plugins`. If the selector does not resolve to a child of the specified document, the tool stops and reverses the installation process, issues a warning, and exits with a non-zero code.
+mode(string) | The mode that determines what type of attribute modifications will be made. <br/> `merge` - Adds the specified attributes to the target element. Will replace the attribute values if the specified attributes already exist in the target element. <br/> `overwrite` - Replaces all attributes in the target element with the attributes specified.
+
+Example:
+
+```xml
+<!-- plugin-1 -->
+<edit-config file="AndroidManifest.xml" target="/manifest/uses-sdk" mode="merge">
+ <uses-sdk android:minSdkVersion="16" android:maxSdkVersion="23" />
+</edit-config>
+<edit-config file="AndroidManifest.xml" target="/manifest/application/activity[@android:name='MainActivity']" mode="overwrite">
+ <activity android:name="MainActivity" android:label="NewLabel" android:configChanges="orientation|keyboardHidden" />
+</edit-config>
+```
+
+AndroidManifest.xml before adding plugin-1:
+```xml
+<manifest android:hardwareAccelerated="true" android:versionCode="1" android:versionName="0.0.1" package="io.cordova.hellocordova" xmlns:android="http://schemas.android.com/apk/res/android">
+ ...
+ <activity android:configChanges="orientation|keyboardHidden|keyboard|screenSize|locale" android:label="@string/activity_name" android:launchMode="singleTop" android:name="MainActivity" android:theme="@android:style/Theme.DeviceDefault.NoActionBar" android:windowSoftInputMode="adjustResize">
+ ...
+ </activity>
+ ...
+ <uses-sdk android:minSdkVersion="14" android:targetSdkVersion="23" />
+</manifest>
+```
+
+AndroidManifest.xml after adding plugin-1:
+```xml
+<manifest android:hardwareAccelerated="true" android:versionCode="1" android:versionName="0.0.1" package="io.cordova.hellocordova" xmlns:android="http://schemas.android.com/apk/res/android">
+ ...
+ <activity android:configChanges="orientation|keyboardHidden" android:label="NewLabel" android:name="MainActivity">
+ ...
+ </activity>
+ ...
+ <uses-sdk android:maxSdkVersion="23" android:minSdkVersion="16" android:targetSdkVersion="23" />
+</manifest>
+```
+
+Multiple plugins will not be able to to modify the same attributes because it may cause problems with the application. An error will be thrown and plugin install will fail. The conflicting `edit-config` tags must be resolved before the plugin can be added. Make modifications to conflicting tags to resolve the conflict, then remove and re-add those updated plugins.
+
+There is an option for those who are certain that the plugin should be installed despite the conflicts. The `-force` flag can be used with `cordova plugin add`. Force adding the plugin will revert conflicting changes of other plugins so that it can be added without issues. `-force` should be used with caution as reverting changes of other plugins may cause the application to not work as expected.
+
+If the plugins ever get in a weird state, remove all plugins and re-add them.
+
+Example:
+
+Assume plugin-1 from above is already installed. Trying to install plugin-2 below will cause an error because plugin-1 has modified `uses-sdk` element in AndroidManifest.xml already.
+
+```xml
+<!-- plugin-2 -->
+<edit-config file="AndroidManifest.xml" target="/manifest/uses-sdk" mode="merge">
+ <uses-sdk android:minSdkVersion="15" />
+</edit-config>
+```
+
+There are a couple ways plugin-2 can be added:
+
+One possible resolution of the conflict is to remove the `edit-config` tag from plugin-2 and merge it into plugin-1's `edit-config` tag (assuming plugin-1 has no issue with this change). Remove both plugins and re-add them with these changes. plugin-2 should be added cleanly this time.
+
+Removing `edit-config` from plugin-2 and merging it into plugin-1:
+```xml
+<!-- plugin-1 -->
+<edit-config file="AndroidManifest.xml" target="/manifest/uses-sdk" mode="merge">
+ <uses-sdk android:minSdkVersion="15" android:maxSdkVersion="23" />
+</edit-config>
+<edit-config file="AndroidManifest.xml" target="/manifest/application/activity[@android:name='MainActivity']" mode="overwrite">
+ <activity android:name="MainActivity" android:label="NewLabel" android:configChanges="orientation|keyboardHidden" />
+</edit-config>
+```
+
+The resulting AndroidManifest.xml after removing and re-adding both plugins:
+```xml
+<manifest android:hardwareAccelerated="true" android:versionCode="1" android:versionName="0.0.1" package="io.cordova.hellocordova" xmlns:android="http://schemas.android.com/apk/res/android">
+ ...
+ <activity android:configChanges="orientation|keyboardHidden" android:label="NewLabel" android:name="MainActivity">
+ ...
+ </activity>
+ ...
+ <uses-sdk android:maxSdkVersion="23" android:minSdkVersion="15" android:targetSdkVersion="23" />
+</manifest>
+```
+
+The second way to add plugin-2 involves adding the plugin with `--force`. The conflicting `edit-config` change from plugin-1 will be reverted and plugin-2's change will be applied. The resulting AndroidManifest.xml will have the `uses-sdk` change from plugin-2 and the `activity` change from plugin-1. Notice only the `uses-sdk` change from plugin-1 is gone since it was the only conflicting change.
+
+The resulting AndroidManifest.xml after force adding plugin-2:
+```xml
+<manifest android:hardwareAccelerated="true" android:versionCode="1" android:versionName="0.0.1" package="io.cordova.hellocordova" xmlns:android="http://schemas.android.com/apk/res/android">
+ ...
+ <activity android:configChanges="orientation|keyboardHidden" android:label="NewLabel" android:name="MainActivity">
+ ...
+ </activity>
+ ...
+ <uses-sdk android:minSdkVersion="15" android:targetSdkVersion="23" />
+</manifest>
+```
+
+Note: Reverted changes from `--force` are gone for good. They will not reappear after removing the plugin that was force added. If the reverted changes are needed, the associated plugins should be removed and re-added.
— End diff –

"the associated plugins" => "all associated plugins"

Github user riknoll commented on the issue:

https://github.com/apache/cordova-docs/pull/614

That make sense to me. I added a few more comments, but otherwise LGTM.

Github user ktop commented on the issue:

https://github.com/apache/cordova-docs/pull/614

@riknoll I've made the changes here and will open a PR in the CLI doc to mention the plugin conflicts and link to the plugin.xml doc for reference.

GitHub user ktop opened a pull request:

https://github.com/apache/cordova-cli/pull/258

CB-11023 Add doc for conflicting plugins

Added a section under plugins for dealing with plugin conflicts involving the new edit-config tag. References the plugin.xml guide for more information and examples.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ktop/cordova-cli cb-11023

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cordova-cli/pull/258.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #258

commit 83c4c6bcc04bd5b4d7c22971010b8ba7e9ac7893
Author: ktop <ktop500@gmail.com>
Date: 2016-07-18T18:29:00Z

CB-11023 Add doc for conflicting plugins

Github user riknoll commented on the issue:

https://github.com/apache/cordova-docs/pull/614

@macdonst any chance you can take over merging this? Been sick and swamped this week and I don't think I'll have a chance to test out the site before merging it in.

Commit 83c4c6bcc04bd5b4d7c22971010b8ba7e9ac7893 in cordova-cli's branch refs/heads/master from Karen Tran
[ https://git-wip-us.apache.org/repos/asf?p=cordova-cli.git;h=83c4c6b ]

CB-11023 Add doc for conflicting plugins

Github user asfgit closed the pull request at:

https://github.com/apache/cordova-cli/pull/258

Commit af2ede15180d202e59d576f8f44e967018a268d1 in cordova-docs's branch refs/heads/master from Karen Tran
[ https://git-wip-us.apache.org/repos/asf?p=cordova-docs.git;h=af2ede1 ]

CB-11023 New edit-config tag in plugin.xml

This closes #614

Github user asfgit closed the pull request at:

https://github.com/apache/cordova-docs/pull/614