You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by ktop <gi...@git.apache.org> on 2016/04/22 16:24:12 UTC
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
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 <kt...@gmail.com>
Date: 2016-04-22T14:05:12Z
CB-11023 Add attribute through config-file tag
Add back comment and fix format
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by ktop <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on the pull request:
https://github.com/apache/cordova-lib/pull/432#issuecomment-218623029
How close is this to being able to merge in? @macdonst
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by macdonst <gi...@git.apache.org>.
Github user macdonst commented on the pull request:
https://github.com/apache/cordova-lib/pull/432#issuecomment-219075301
@ktop, @stevengill my environment seems to be in a bad state. I've got problems with cordova-fetch and the new telemetry stuff I'm working through. Right now when I try to test this PR the lines get added multiple times instead of merged. I don't think that is a problem with the PR but rather my env right now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by macdonst <gi...@git.apache.org>.
Github user macdonst commented on the pull request:
https://github.com/apache/cordova-lib/pull/432#issuecomment-218824969
@stevengill it was working pretty good last I checked. With the config.xml stuff in it should be ready to go. I've earmarked some time tomorrow to do my testing. cc: @ktop
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by riknoll <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by vladimir-kotikov <gi...@git.apache.org>.
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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by riknoll <gi...@git.apache.org>.
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`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by macdonst <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by ktop <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by purplecabbage <gi...@git.apache.org>.
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>
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by ktop <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by macdonst <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by ktop <gi...@git.apache.org>.
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) {
var browserify = require('../plugman/browserify');
return browserify(project, platformApi);
}
+ })
+ .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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by ktop <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by ktop <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-lib/pull/432#issuecomment-219188296
Yeah, same here. I think anyone who is working from master and using npm
link(s) is bunked for a bit.
@purplecabbage
risingj.com
On Fri, May 13, 2016 at 8:22 AM, Simon MacDonald <no...@github.com>
wrote:
> @ktop <https://github.com/ktop>, @stevengill
> <https://github.com/stevengill> my environment seems to be in a bad
> state. I've got problems with cordova-fetch and the new telemetry stuff I'm
> working through. Right now when I try to test this PR the lines get added
> multiple times instead of merged. I don't think that is a problem with the
> PR but rather my env right now.
>
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/cordova-lib/pull/432#issuecomment-219075301>
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by nikhilkh <gi...@git.apache.org>.
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) {
var browserify = require('../plugman/browserify');
return browserify(project, platformApi);
}
+ })
+ .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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by macdonst <gi...@git.apache.org>.
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
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by purplecabbage <gi...@git.apache.org>.
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
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request #432: CB-11023 Add attribute through config-file ta...
Posted by ktop <gi...@git.apache.org>.
Github user ktop closed the pull request at:
https://github.com/apache/cordova-lib/pull/432
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by ktop <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by riknoll <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by riknoll <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by ktop <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by riknoll <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the pull request:
https://github.com/apache/cordova-lib/pull/432#issuecomment-215478064
## [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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by macdonst <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib issue #432: CB-11023 Add attribute through config-file tag
Posted by ktop <gi...@git.apache.org>.
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....
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-11023 Add attribute through config-fi...
Posted by purplecabbage <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org