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

[msbuild] support AndroidManifest.xml placeholders. #342

Merged
merged 1 commit into from Feb 6, 2017

Conversation

@atsushieno
Copy link
Contributor

commented Dec 20, 2016

We only support ${applicationId} so far, but it seems to be used a lot as
https://developer.android.com/studio/build/manifest-build-variables.html

Since we are not using Gradle, we'd need something similar to it, which
is therefore MSBuild property.

@@ -38,6 +38,7 @@ public class GenerateJavaStubs : Task
public bool Debug { get; set; }
public string ApplicationName { get; set; }
public string PackageName { get; set; }
public string ManifestPlaceholders { get; set; }

This comment has been minimized.

Copy link
@jonpryor

jonpryor Dec 28, 2016

Member

This should be a string[], not a string. See also the ReplaceFileContents.Replacements property.

This comment has been minimized.

Copy link
@atsushieno

atsushieno Jan 5, 2017

Author Contributor

OK, that's true. Updating.

@@ -790,6 +791,13 @@ public void Save (System.IO.TextWriter stream)
var s = new StreamReader (ms).ReadToEnd ();
if (ApplicationName != null)
s = s.Replace ("${applicationId}", ApplicationName);
if (Placeholders != null)
foreach (var entry in Placeholders.Split (';').Select (e => e.Split ('='))) {

This comment has been minimized.

Copy link
@jonpryor

jonpryor Dec 28, 2016

Member

Splitting on ; shouldn't be needed after moving to string[]. Additionally, the e.Split('=') should instead use the string.Split(char[], int, StringSplitOptions) overload:

static readonly char[] Separator = new [] { '=' };
...
... e.Split (Separator, 2, StringSplitOptions.RemoveEmptyEntries)

@atsushieno atsushieno force-pushed the atsushieno:manifest-placeholder branch from e1be4ac to 3d41c0b Jan 5, 2017

@@ -790,6 +791,13 @@ public void Save (System.IO.TextWriter stream)
var s = new StreamReader (ms).ReadToEnd ();
if (ApplicationName != null)
s = s.Replace ("${applicationId}", ApplicationName);
if (Placeholders != null)
foreach (var entry in Placeholders.Select (e => e.Split ('='))) {

This comment has been minimized.

Copy link
@jonpryor

jonpryor Jan 9, 2017

Member

This should use string.Split(char[], int, StringSplitOptions):

foreach (var entry in Placeholders.Select (e => e.Split (new[]{'='}, 2, StringSplitOptions.None)) {
    if (entry.Length < 2) {
        log.LogWarning ($"Invalid application placeholders $(AndroidApplicationPlaceholders) value. Use `key1=value1;key2=value2` format. The specified value was: {string.Join (';', Placeholders})");
        continue;
    }
    s = s.Replace ("${" + entry [0] + "}", entry [1]);
}

This overload removes the need to `string.Join ("=", entry.Skip(1)).

@jonpryor

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

We should probably have a test for this as well, perhaps by using src/Mono.Android/Test/Mono.Android-Tests.csproj and src/Mono.Android/Test/Properties/AndroidManifest.xml, and possibly a [Test] that checks the value of...whatever was added to AndroidManifest.xml.

@atsushieno atsushieno force-pushed the atsushieno:manifest-placeholder branch from 3d41c0b to 77fb54a Jan 16, 2017

@atsushieno

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2017

A corresponding test is being added to our msbuild tests (which should become public too).

@atsushieno

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2017

This needs updates from @jonpryor.

[msbuild] support AndroidManifest.xml placeholders.
We only support ${applicationId} so far, but it seems to be used a lot as
https://developer.android.com/studio/build/manifest-build-variables.html

Since we are not using Gradle, we'd need something similar to it, which
is therefore MSBuild property.

@atsushieno atsushieno force-pushed the atsushieno:manifest-placeholder branch from 77fb54a to 8bacb4b Feb 2, 2017

@jonpryor jonpryor merged commit b423ff6 into xamarin:master Feb 6, 2017

1 check passed

Build Build finished. 134 tests run, 1 skipped, 0 failed.
Details
@jzeferino

This comment has been minimized.

Copy link

commented Mar 21, 2017

Is this PR released in any Xamarin android version?

@jonpryor

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

@jzeferino: It's in Xamarin.Android 7.2, currently in the Beta channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.