Skip to content

Commit

Permalink
[LayoutBindings] Fix '[Preserve]' is obsolete warnings (#8529)
Browse files Browse the repository at this point in the history
Fixes: #7480

Context: e604833

When [Layout Bindings][0] are enabled, a set of `partial` classes
will be generated which "mirrors" the `.axml` structure in C#.

Consider:

	dotnet new android -n com.example.codebehind

You enable Code-Behind by:

 1. Setting the `$(AndroidGenerateLayoutBindings)` MSBuild property
    to `true`.
 2. Using `android:id` on elements within `.axml` files.

Consider this patch to the above `dotnet new`:

	diff --git a/Resources/layout/activity_main.xml b/Resources/layout/activity_main.xml
	index f949852..c74521c 100644
	--- a/Resources/layout/activity_main.xml
	+++ b/Resources/layout/activity_main.xml
	@@ -8,6 +8,7 @@
	         android:layout_width="wrap_content"
	         android:layout_height="wrap_content"
	         android:layout_centerInParent="true"
	+        android:id="@+id/text"
	         android:text="@string/app_text"
	     />
	 </RelativeLayout>
	diff --git a/com.example.codebehind.csproj b/com.example.codebehind.csproj
	index 3fdbcb5..8190f84 100644
	--- a/com.example.codebehind.csproj
	+++ b/com.example.codebehind.csproj
	@@ -8,5 +8,6 @@
	     <ApplicationId>com.companyname.com.example.codebehind</ApplicationId>
	     <ApplicationVersion>1</ApplicationVersion>
	     <ApplicationDisplayVersion>1.0</ApplicationDisplayVersion>
	+    <AndroidGenerateLayoutBindings>true</AndroidGenerateLayoutBindings>
	   </PropertyGroup>
	 </Project>

The resulting output contains
`$(IntermediateOutputPath)codebehind\Binding.activity_main.g.cs`,
which declares a type based on the `.axml` file name, and a mirror of
the structure:

	namespace Binding {
	    // typename based on `activity_main.xml` filename
	    sealed partial class activity_main : global::Xamarin.Android.Design.LayoutBinding {
	        [global::Android.Runtime.PreserveAttribute (Conditional=true)]
	        public activity_main (Activity client, …);
	        // …

	        // `text` is via `android:id="@+id/text"`
	        public TextView text => …;
	    }
	}

The problem is the use of `PreserveAttribute` in the generated code;
it's been `[Obsolete]` since e604833 (over two years ago), which
means all projects using Layout Bindings and Layout Code-Behind always
have [CS0618][1] warnings.

Remove the emission of `PreserveAttribute`, so that CS0618 is no
longer generated.

Additionally, within the generated files disable the [CS1591][2] and
[CS8981][3] warnings:

  * CS1591 is around missing XML documentation comments.  These code
    behind files do not have XML documentation comments, so if/when
    `$(DocumentationFile)` is set, these will be emitted for the
    generated code.  We do not currently plan on emitting XML
    documentation comments, so disable this warning.

  * CS8981 is emitted when a type name consists solely of lowercase
    characters.  As the generated Binding types are based on the
    filename -- which is frequently all lowercase -- this warning
    may be emitted.

Finally, add `#nullable enable` to `LayoutBinding.cs` and fix the
nullable reference type warnings.

[0]: https://github.com/xamarin/xamarin-android/blob/2f4e01ec15102dd9cd922cbd833f6482d69512b5/Documentation/guides/LayoutCodeBehind.md
[1]: https://learn.microsoft.com/dotnet/csharp/language-reference/compiler-messages/cs0618
[2]: https://learn.microsoft.com/dotnet/csharp/language-reference/compiler-messages/cs1591
[3]: https://learn.microsoft.com/dotnet/csharp/language-reference/compiler-messages/warning-waves#cs8981---the-type-name-only-contains-lower-cased-ascii-characters
  • Loading branch information
dellis1972 authored Mar 27, 2024
1 parent 150e1d9 commit 751eb96
Show file tree
Hide file tree
Showing 10 changed files with 86 additions and 52 deletions.
46 changes: 22 additions & 24 deletions src/Xamarin.Android.Build.Tasks/Resources/LayoutBinding.cs
Original file line number Diff line number Diff line change
@@ -1,27 +1,29 @@
using System;
#nullable enable

using System;
using System.Diagnostics.CodeAnalysis;
using Android.App;
using Android.Views;

namespace Xamarin.Android.Design
{
public delegate Java.Lang.Object OnLayoutItemNotFoundHandler (int resourceId, Type expectedViewType);
public delegate Java.Lang.Object? OnLayoutItemNotFoundHandler (int resourceId, Type expectedViewType);

abstract class LayoutBinding
{
const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

Activity boundActivity;
View boundView;
OnLayoutItemNotFoundHandler onLayoutItemNotFound;
Activity? boundActivity;
View? boundView;
OnLayoutItemNotFoundHandler? onLayoutItemNotFound;

protected LayoutBinding (Activity activity, OnLayoutItemNotFoundHandler onLayoutItemNotFound = null)
protected LayoutBinding (Activity activity, OnLayoutItemNotFoundHandler? onLayoutItemNotFound = null)
{
boundActivity = activity ?? throw new ArgumentNullException (nameof (activity));
this.onLayoutItemNotFound = onLayoutItemNotFound;
}

protected LayoutBinding (View view, OnLayoutItemNotFoundHandler onLayoutItemNotFound = null)
protected LayoutBinding (View view, OnLayoutItemNotFoundHandler? onLayoutItemNotFound = null)
{
boundView = view ?? throw new ArgumentNullException (nameof (view));
this.onLayoutItemNotFound = onLayoutItemNotFound;
Expand All @@ -38,14 +40,14 @@ protected T FindView <
if (cachedField != null)
return cachedField;

T ret;
T? ret = null;
if (boundActivity != null)
ret = boundActivity.FindViewById <T> (resourceId);
else
else if (boundView != null)
ret = boundView.FindViewById <T> (resourceId);

if (ret == null && onLayoutItemNotFound != null)
ret = (T)onLayoutItemNotFound (resourceId, typeof (T));
ret = (T?)onLayoutItemNotFound (resourceId, typeof (T));

if (ret == null)
throw new global::System.InvalidOperationException ($"View not found (Resource ID: {resourceId})");
Expand All @@ -71,16 +73,16 @@ T __FindFragment<
T
> (
int resourceId,
Func<Activity, T> finder,
ref T cachedField)
Func<Activity, T?> finder,
ref T? cachedField)
where T: Java.Lang.Object
{
if (cachedField != null)
return cachedField;

var ret = finder (EnsureActivity ());
if (ret == null && onLayoutItemNotFound != null)
ret = (T)onLayoutItemNotFound (resourceId, typeof (T));
ret = (T?)onLayoutItemNotFound (resourceId, typeof (T));

if (ret == null)
throw new InvalidOperationException ($"Fragment not found (ID: {resourceId}; Type: {typeof (T)})");
Expand All @@ -89,29 +91,25 @@ T __FindFragment<
return ret;
}
#if __ANDROID_11__
#pragma warning disable CA1422
protected T FindFragment<
[DynamicallyAccessedMembers (Constructors)]
T
> (
int resourceId,
global::Android.App.Fragment __ignoreMe,
ref T cachedField
global::Android.App.Fragment? __ignoreMe,
ref T? cachedField
)
where T: global::Android.App.Fragment
{
return __FindFragment<T> (resourceId, (activity) => activity.FragmentManager.FindFragmentById<T> (resourceId), ref cachedField);
return __FindFragment<T> (resourceId, (activity) => activity.FragmentManager?.FindFragmentById<T> (resourceId), ref cachedField);
}
#pragma warning restore CA1422
#endif // __ANDROID_11__

#if __HAVE_SUPPORT__
protected T FindFragment <T> (int resourceId, global::Android.Support.V4.App.Fragment __ignoreMe, ref T cachedField) where T: global::Android.Support.V4.App.Fragment
{
return __FindFragment<T> (resourceId, (activity) => activity.FragmentManager.FindFragmentById<T> (resourceId), ref cachedField);
}
#endif // __HAVE_SUPPORT__

#if __HAVE_ANDROIDX__
protected T FindFragment<T> (int resourceId, global::AndroidX.Fragment.App.Fragment __ignoreMe, ref T cachedField) where T: global::AndroidX.Fragment.App.Fragment
protected T FindFragment<T> (int resourceId, global::AndroidX.Fragment.App.Fragment? __ignoreMe, ref T? cachedField)
where T : global::AndroidX.Fragment.App.Fragment
{
return __FindFragment(resourceId, (activity) => {
if (activity is AndroidX.Fragment.App.FragmentActivity activity_) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ public sealed class State
// generating code for the partial Activity class
public string BindingClassName { get; }

public bool LinkerPreserveConstructors { get; set; }

public List<string> ExtraImportNamespaces { get; } = new List <string> ();

public string AndroidFragmentType { get; }
Expand Down Expand Up @@ -155,20 +153,18 @@ protected virtual void WriteOnSetContentViewPartials (State state)
WritePartialClassOnSetContentViewPartial_Int (state);
}

public State BeginBindingFile (StreamWriter writer, string layoutResourceId, string classNamespace, string className, string androidFragmentType, bool linkerPreserveConstructors = true)
public State BeginBindingFile (StreamWriter writer, string layoutResourceId, string classNamespace, string className, string androidFragmentType)
{
var state = new State (writer, className, !String.IsNullOrWhiteSpace (classNamespace), androidFragmentType) {
LinkerPreserveConstructors = linkerPreserveConstructors
};
var state = new State (writer, className, !String.IsNullOrWhiteSpace (classNamespace), androidFragmentType);
BeginBindingFile (state, layoutResourceId, classNamespace, className);
WriteBindingConstructors (state, className, state.LinkerPreserveConstructors);
WriteBindingConstructors (state, className);
return state;
}

protected abstract void BeginBindingFile (State state, string layoutResourceId, string classNamespace, string className);
public abstract void EndBindingFile (State state);

protected abstract void WriteBindingConstructors (State state, string className, bool linkerPreserve);
protected abstract void WriteBindingConstructors (State state, string className);
protected abstract void WriteBindingViewProperty (State state, LayoutWidget widget, string resourceNamespace);
protected abstract void WriteBindingFragmentProperty (State state, LayoutWidget widget, string resourceNamespace);
protected abstract void WriteBindingMixedProperty (State state, LayoutWidget widget, string resourceNamespace);
Expand Down Expand Up @@ -287,10 +283,11 @@ protected void WriteBindingPropertyBackingField (State state, LayoutWidget widge
WriteResetLocation (state);
}

public void WriteComment (State state, string text)
public void WriteComment (State state, params string [] text)
{
EnsureArgument (state, nameof (state));
WriteLineIndent (state, $"{LineCommentString}{text}");
foreach (string line in text)
WriteLineIndent (state, $"{LineCommentString}{line}");
}

public void WriteComment (State state, ICollection<string> lines)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,33 @@ void WriteClassClose (State state)
WriteLineIndent (state, "}");
}

void WriteDisableWarnings (State state)
{
state.WriteLine ("#pragma warning disable CS8981");
state.WriteLine ("#pragma warning disable CS1591");
}

void WriteEnableWarnings (State state)
{
state.WriteLine ("#pragma warning restore CS1591");
state.WriteLine ("#pragma warning restore CS8981");
}

void WriteAutoGeneratedComment (State state)
{
state.WriteLine (@"//------------------------------------------------------------------------------
// <auto-generated>
// This code was generated by a tool.
//
// Changes to this file may cause incorrect behavior and will be lost if
// the code is regenerated.
// </auto-generated>
//------------------------------------------------------------------------------");
}

void WriteFilePreamble (State state, string classNamespace)
{
WriteAutoGeneratedComment (state);
WriteUsings (state);
state.WriteLine ();
WriteNamespaceOpen (state, classNamespace);
Expand All @@ -172,13 +197,15 @@ void WriteNamespaceOpen (State state, string classNamespace)
state.WriteLine ($"namespace {classNamespace}");
state.WriteLine ("{");
state.IncreaseIndent ();
WriteDisableWarnings (state);
}

void WriteNamespaceClose (State state)
{
if (!state.IsInNamespace)
return;

WriteEnableWarnings (state);
state.DecreaseIndent ();
state.WriteLine ("}");
}
Expand All @@ -200,15 +227,14 @@ void WriteUsing (State state, string ns)
state.WriteLine ($"using global::{ns};");
}

protected override void WriteBindingConstructors (State state, string className, bool linkerPreserve)
protected override void WriteBindingConstructors (State state, string className)
{
WriteConstructor (state, className, "Android.App.Activity", linkerPreserve);
WriteConstructor (state, className, "Android.Views.View", linkerPreserve);
WriteConstructor (state, className, "Android.App.Activity");
WriteConstructor (state, className, "Android.Views.View");
}

void WriteConstructor (State state, string className, string clientType, bool linkerPreserve)
void WriteConstructor (State state, string className, string clientType)
{
WritePreserveAtribute (state, linkerPreserve);
WriteLineIndent (state, $"public {className} (");
state.IncreaseIndent ();
WriteLineIndent (state, $"global::{clientType} client,");
Expand All @@ -221,14 +247,6 @@ void WriteConstructor (State state, string className, string clientType, bool li
state.WriteLine ();
}

void WritePreserveAtribute (State state, bool linkerPreserve)
{
if (!linkerPreserve)
return;

WriteLineIndent (state, $"[global::Android.Runtime.PreserveAttribute (Conditional=true)]");
}

public override void EndBindingFile (State state)
{
EnsureArgument (state, nameof (state));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ public class GenerateResourceDesignerIntermediateClass : AndroidTask
namespace %NAMESPACE% {
#pragma warning disable IDE0002
/// <summary>
/// Android Resource Designer class.
/// Exposes the Android Resource designer assembly into the project Namespace.
/// </summary>
public partial class Resource : %BASECLASS% {
}
#pragma warning restore IDE0002
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ void SuccessfulBuild_AndroidX (TestProjectInfo testInfo, bool many, bool dtb, Lo

CopyLogs (testInfo, true);
Assert.That (success, Is.True, "Build should have succeeded");
Assert.IsTrue (StringAssertEx.ContainsText (builder.LastBuildOutput, " 0 Warning(s)"), $"{builder.BuildLogFile} should have no MSBuild warnings.");

CopyGeneratedFiles (testInfo);

Expand Down Expand Up @@ -508,7 +509,8 @@ string GetBuildTarget (bool isDTB)
string[] GetBuildProperties (LocalBuilder builder, bool manyBuild, bool dtbBuild, bool referenceAndroidX, params string[] extraConstants)
{
var ret = new List <string> {
"AndroidGenerateLayoutBindings=true"
"AndroidGenerateLayoutBindings=true",
"\"NoWarn=CS0414;CA1416;CS1591;XA1005;XA4225\""
};
if (manyBuild)
ret.Add ("ForceParallelBuild=true");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,6 @@
<ProjectReference Include="..\CommonSampleLibrary\CommonSampleLibrary.NET.csproj" />
</ItemGroup>
<ItemGroup Condition=" '$(ReferenceAndroidX)' == 'True' ">
<PackageReference Include="Xamarin.AndroidX.AppCompat" Version="1.4.1.1" />
<PackageReference Include="Xamarin.AndroidX.AppCompat" Version="1.6.1.5" />
</ItemGroup>
</Project>
2 changes: 2 additions & 0 deletions tests/CodeBehind/BuildTests/MainMergeActivity.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ protected override void OnCreate (Bundle savedInstanceState)

#if NOT_CONFLICTING_FRAGMENT
CommonSampleLibrary.LogFragment log2 = secondary_log_fragment;
#elif __HAVE_ANDROIDX__
global::AndroidX.Fragment.App.Fragment log2 = secondary_log_fragment;
#else
global::Android.App.Fragment log2 = secondary_log_fragment;
#endif
Expand Down
1 change: 0 additions & 1 deletion tests/CodeBehind/BuildTests/Properties/AndroidManifest.xml
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<manifest xmlns:android="http://schemas.android.com/apk/res/android" android:versionCode="1" android:versionName="1.0" package="com.xamarin.CodeBehindBuildTests">
<uses-sdk android:minSdkVersion="21" />
<application android:allowBackup="true" android:icon="@mipmap/icon" android:label="@string/app_name">
</application>
</manifest>
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,10 @@
<AssemblyName>CommonSampleLibrary</AssemblyName>
<AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>
</PropertyGroup>
<PropertyGroup>
<DefineConstants Condition=" '$(ExtraConstants)' != '' ">$(DefineConstants);$(ExtraConstants)</DefineConstants>
</PropertyGroup>
<ItemGroup Condition=" '$(ReferenceAndroidX)' == 'True' ">
<PackageReference Include="Xamarin.AndroidX.AppCompat" Version="1.6.1.5" />
</ItemGroup>
</Project>
12 changes: 10 additions & 2 deletions tests/CodeBehind/CommonSampleLibrary/Logger/LogFragment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,21 @@
using Android.Views;
using Android.Widget;
using Android.Graphics;
using Android.Util;

namespace CommonSampleLibrary
{
#pragma warning disable CA1422
/**
* Simple fraggment which contains a LogView and uses is to output log data it receives
* through the LogNode interface.
*/
public class LogFragment : Fragment
public class LogFragment :
#if __HAVE_ANDROIDX__
AndroidX.Fragment.App.Fragment
#else
Fragment
#endif
{
LogView mLogView;
ScrollView mScrollView;
Expand Down Expand Up @@ -60,7 +67,7 @@ public View InflateViews ()

mLogView.Gravity = GravityFlags.Bottom;
mLogView.SetTextAppearance (Activity, Android.Resource.Style.TextAppearanceMedium);

mScrollView.AddView (mLogView);
return mScrollView;
}
Expand All @@ -77,5 +84,6 @@ public LogView LogView {
get { return mLogView; }
}
}
#pragma warning restore CA1422
}

0 comments on commit 751eb96

Please sign in to comment.