Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CB-11023 Add edit-config functionality #449

Closed
wants to merge 1 commit into from
Closed

Conversation

ktop
Copy link
Contributor

@ktop ktop commented Jun 3, 2016

New edit-config tag for plugin.xml will allow users to modify xml attributes of any xml file.

Using AndroidManifest.xml as an example. Assumes your AndroidManifest.xml has a second activity element with attribute android:name="SecondActivity"

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

Note: Using --force to overwrite a conflict will remove the conflicting attribute changes from the file. These changes will not be restorable since the attributes were forced to be overwritten. To get your changes back, you should uninstall then reinstall that plugin.

@codecov-io
Copy link

codecov-io commented Jun 3, 2016

Current coverage is 80.56%

Merging #449 into master will not change coverage

@@             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. Last updated by ca98abf...8c2551d

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');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@riknoll
Copy link
Contributor

riknoll commented Jun 9, 2016

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

@riknoll
Copy link
Contributor

riknoll commented Jun 9, 2016

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.

@ktop
Copy link
Contributor Author

ktop commented Jun 9, 2016

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

@riknoll
Copy link
Contributor

riknoll commented Jun 9, 2016

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

@macdonst
Copy link
Member

macdonst commented Jun 9, 2016

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

@ktop
Copy link
Contributor Author

ktop commented Jun 9, 2016

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.

@ktop
Copy link
Contributor Author

ktop commented Jun 13, 2016

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.

@riknoll
Copy link
Contributor

riknoll commented Jun 14, 2016

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.

@macdonst
Copy link
Member

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.

@ktop
Copy link
Contributor Author

ktop commented Jun 16, 2016

Cool, let me rebase

@ktop
Copy link
Contributor Author

ktop commented Jun 20, 2016

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

@asfgit asfgit closed this in 5fb8dff Jun 20, 2016
@riknoll
Copy link
Contributor

riknoll commented Jun 20, 2016

Done! Sorry for the delay. @ktop can you open a PR documenting this feature in the plugin.xml reference as well?

@ktop
Copy link
Contributor Author

ktop commented Jun 20, 2016

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

@riknoll
Copy link
Contributor

riknoll commented Jun 20, 2016

Yep! The guide for writing xml references is here. The file you need to edit is the one I linked in my other comment in the docs/en/dev path of the cordova-docs repo. You can just add an edit-config section next to the config-file one. Tag me in the PR once you open and I can merge it. Once we do a Cordova common release, we can snap the docs and update the website with your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants