By default `ConvertResourcesCases` ignores the assembly name when replacing fully qualified managed type names in `class` or `android:name` attributes on `fragment` elements

[brendanzagaeski]

Background context: 0dee27d

In short, the approach from the earlier fix removes slightly too much information in situations where the acw-map also contains another, longer match. One idea to improve this would be to loop repeatedly, removing one comma-delimited chunk at a time from the attribute value until the longest available match from the acw-map is found. Another idea would be to change attr.Value.Substring (0, attr.Value.IndexOf (',')) so that it takes the substring up to the second comma rather than the first.

The first idea would allow class='ClassLibrary1.CustomView, ClassLibrary3' to be replaced successfully with the example acmap.txt from the Steps to Reproduce (below) even though that file doesn't mention a ClassLibrary3 assembly. That might be an advantage or disadvantage. It might be useful in some cases, but since Type.GetType ("ClassLibrary1.CustomView, ClassLibrary3") would return null, it might be unexpected.

The second idea makes an assumption that the assembly-qualified name will always be present in the acw-map. That is currently the case:
https://github.com/xamarin/xamarin-android/blob/b61941ed62c108f54e680b805bce1efa8f18e4e8/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs#L176

Steps to Reproduce

Add the following test to ConvertResourcesCasesTests and run it:

		[Test]
		public void FullyQualifiedCustomFragmentClassAttributes ()
		{
			var path = Path.Combine (Root, "temp", "FullyQualifiedCustomFragmentClassAttributes");
			Directory.CreateDirectory (path);
			var resPath = Path.Combine (path, "res");
			Directory.CreateDirectory (Path.Combine (resPath, "layout"));
			File.WriteAllText (Path.Combine (resPath, "layout", "main.xml"), @"<?xml version='1.0' ?>
<LinearLayout xmlns:android='http://schemas.android.com/apk/res/android'>
<fragment android:name='ClassLibrary1.CustomView, ClassLibrary1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' />
<fragment class='ClassLibrary1.CustomView, ClassLibrary1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' />
<fragment android:name='ClassLibrary1.CustomView, ClassLibrary2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' />
<fragment class='ClassLibrary1.CustomView, ClassLibrary2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' />
</LinearLayout>
");
			var errors = new List<BuildErrorEventArgs> ();
			IBuildEngine engine = new MockBuildEngine (TestContext.Out, errors);
			var task = new ConvertResourcesCases {
				BuildEngine = engine
			};
			task.ResourceDirectories = new ITaskItem [] {
				new TaskItem (resPath),
			};
			task.AcwMapFile = Path.Combine (path, "acwmap.txt");
			task.CustomViewMapFile = Path.Combine (path, "classmap.txt");
			File.WriteAllLines (task.AcwMapFile, new string [] {
				"ClassLibrary1.CustomView, ClassLibrary1;md5d6f7135293df7527c983d45d07471c5e.CustomView",
				"ClassLibrary1.CustomView;md5d6f7135293df7527c983d45d07471c5e.CustomView",
				"classLibrary1.CustomView;md5d6f7135293df7527c983d45d07471c5e.CustomView",
				"ClassLibrary1.CustomView, ClassLibrary2;md58090b0504f2fbdbfb93028c14dfe0429.CustomView",
				"ClassLibrary1.CustomView;md58090b0504f2fbdbfb93028c14dfe0429.CustomView",
				"classLibrary1.CustomView;md58090b0504f2fbdbfb93028c14dfe0429.CustomView",
			});
			Assert.IsTrue (task.Execute (), "Task should have executed successfully");
			var custom = new ConvertCustomView () {
				BuildEngine = engine,
				CustomViewMapFile = task.CustomViewMapFile,
				AcwMapFile = task.AcwMapFile,
				ResourceDirectories = new ITaskItem [] {
					new TaskItem (resPath),
				},
			};
			Assert.IsTrue (custom.Execute (), "Task should have executed successfully");
			var output = File.ReadAllText (Path.Combine (resPath, "layout", "main.xml"));
			StringAssert.Contains ("md5d6f7135293df7527c983d45d07471c5e.CustomView", output, "md5d6f7135293df7527c983d45d07471c5e.CustomView should exist in the main.xml");
			StringAssert.Contains ("md58090b0504f2fbdbfb93028c14dfe0429.CustomView", output, "md58090b0504f2fbdbfb93028c14dfe0429.CustomView should exist in the main.xml");
			StringAssert.DoesNotContain ("ClassLibrary1.CustomView", output, "ClassLibrary1.CustomView should have been replaced in all cases.");
			Directory.Delete (path, recursive: true);
		}

Expected Behavior

The test passes. This would mean that ConvertCustomView successfully used the assembly-qualified name for each of the managed types in the input layout .xml file when mapping each one to a Java Callable Wrapper name from the acwmap.txt file.

The expected main.xml file contents before the Directory.Delete() step would be:

<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android">
  <fragment android:name="md5d6f7135293df7527c983d45d07471c5e.CustomView" />
  <fragment class="md5d6f7135293df7527c983d45d07471c5e.CustomView" />
  <fragment android:name="md58090b0504f2fbdbfb93028c14dfe0429.CustomView" />
  <fragment class="md58090b0504f2fbdbfb93028c14dfe0429.CustomView" />
</LinearLayout>

Actual Behavior

The test fails because all of the managed type names are replaced with the Java Callable Wrapper name "md5d6f71..." that corresponds to the type from the ClassLibrary1 assembly.

<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
  <fragment android:name="md5d6f7135293df7527c983d45d07471c5e.CustomView" />
  <fragment class="md5d6f7135293df7527c983d45d07471c5e.CustomView" />
  <fragment android:name="md5d6f7135293df7527c983d45d07471c5e.CustomView" />
  <fragment class="md5d6f7135293df7527c983d45d07471c5e.CustomView" />
</LinearLayout>

If you change the class attribute to the shorter, partially assembly-qualified name ClassLibrary1.CustomView, ClassLibrary2, then the replacement works as desired because that exact string exists in the acwmap.txt file.

[brendanzagaeski]

To jot down a possible alternative to fixing this, it might also be acceptable to take gradual steps to remove the current incomplete support for fully assembly-qualified names. For example, a future feature release could start emitting a build warning for fully assembly-qualified names, and then a later release could remove the warning (or change it to an error) and remove the string-splitting logic in TryFixFragment() to go back to the old behavior where only names with an exact match in the acw-map would be replaced.

That approach would have the advantage that it would remove a tiny bit of complexity in how the Android resource files are processed.

[jonpryor]

https://github.com/xamarin/xamarin-android/blob/307355f8d9bc9fc49f152e67efafd728c0ec0e2f/src/Xamarin.Android.Build.Tasks/Tasks/ConvertCustomView.cs#L133-L141

[jonpryor]

Given:

<fragment android:name='ClassLibrary1.CustomView, ClassLibrary1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' />

Then ConvertCustomView.TryFixFragment() needs to lookup the following keys within acwMap:

1. ClassLibrary1.CustomView, ClassLibrary1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null (which will fail)
2. ClassLibrary1.CustomView, ClassLibrary1 (assembly-qualified)
3. ClassLibrary1.CustomView (simple-name)

Currently, only (1) and (3) are done, which is the bug. To fix this, we also need to check (2).

This could be done by doing .LastIndexOf(',') in a loop, trying each value, until no ,s are left, which would also try the (won't match) values of ClassLibrary1.CustomView, ClassLibrary1, Version=1.0.0.0, Culture=neutral and ClassLibrary1.CustomView, ClassLibrary1, Version=1.0.0.0, but that's fine.