-
Notifications
You must be signed in to change notification settings - Fork 564
[Xamarin.Android.Build.Tasks] Add Support for CodeBehind for layout files #1238
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
Conversation
2ec27d3 to
a2c6fbd
Compare
a2c6fbd to
cd18b4d
Compare
|
I don't think we should be extending any attributes in |
5fd6521 to
79a460f
Compare
Documentation/LayoutCodeBehind.md
Outdated
|
|
||
| with | ||
|
|
||
| myButton.Click += delegate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code indentation is inconsistent within the file. Here, it's 8 spaces, while later below it's 4 spaces.
Indentation should be consistent.
Documentation/LayoutCodeBehind.md
Outdated
| # How it works | ||
|
|
||
| There are a couple of new MSBuild Tasks which generate the code behind. | ||
| `CalculateLayoutCodeBehind` and `GenerateCodeBehindForLayout`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are tasks, please use <CalculateLayoutCodeBehind/> and <GenerateCodeBehindForLayout/>. If these are targets, please use "the CalculateLayoutCodeBehind target", etc.
a8c67bb to
fc53f57
Compare
fd3a940 to
43a42a9
Compare
|
On the generated code, there is a slight optimization that should be made for the creator function. In the current generated code as I see it in the description, anytime the property for views are called a new instance of 2 options:
|
|
Proposed commit message when merging: |
|
@dellis1972: while reviewing the commit message, a few issues popped up which aren't in the documentation, nor the commit message, nor...make a lot of sense to me. In particular, the What do we want? Presumably this: InitializeContentView();
myButton.Click += delegate {Console.WriteLine ("clicked!");};But if Is this desirable? Similarly, what happens if |
|
The documentation should be improved to at least mention
|
|
OK the |
|
I also think we should change the default for Alternatively provide virtual method which people can override to stop the exception being thrown. |
|
@dellis1972: I think you have it backwards:
The I also don't really see how InitializeComponentView(throwOnMissingView:false);
SetContentView(Resource.Layout.CompletelyWrongLayout); // bwa-ha-ha-ha-ha-ha
myButton.Click += delegate {};Assuming that If If it is overridden...what is it supposed to do? partial void OnLayoutViewNotFound (int resourceId, Type type)
{
// Uh...?
}It could throw an exception, which is arguably the safest thing to do. If it doesn't throw an exception...then it must "somehow" make the There's no straightforward way for the dev to "know" that partial void OnLayoutViewNotFound (int resourceId, Type type)
{
if (resourceId == Resource.Id.myButton) {
__myButton = ....wait, what?
}
}In short, I see no reason for Meaning the only sane approach is to always Meaning we don't need a |
|
ok I'll do that then
…On 7 February 2018 at 19:25, Jonathan Pryor ***@***.***> wrote:
@dellis1972 <https://github.com/dellis1972>: I think you have it backwards
<#1238 (comment)>:
OnLayoutViewNotFound() only needs to be implemented if
InitializeContentView() is called with throwOnMissingView:false. When
throwOnMissingView:true is used, the exception will be thrown, and
OnLayoutViewNotFound() won't be called.
Alternatively provide virtual method which people can override to stop the
exception being thrown.
The partial method is better than a virtual method, as the class might
not be subclassed; why require subclassing?
I also don't really see *how* OnLayoutViewNotFound() *will* be used.
Suppose we have:
InitializeComponentView(throwOnMissingView:false);
SetContentView(Resource.Layout.CompletelyWrongLayout); // bwa-ha-ha-ha-ha-hamyButton.Click += delegate {};
Assuming that Resource.Layout.CompletelyWrongLayout doesn't contain a
myButton id, *there is no myButton id*. What should happen?
If OnLayoutViewNotFound() isn't "overridden", then we get a
NullReferenceException on myButton.Click.
If it *is* overridden...what is it supposed to *do*?
partial void OnLayoutViewNotFound (int resourceId, Type type)
{
// Uh...?
}
It could throw an exception, which is arguably the safest thing to do.
If it *doesn't* throw an exception...then it *must* "somehow" make the
myButton property return a non-null value. If it *doesn't*, then we
*still* have a NullReferenceException!
There's no straightforward way for the dev to "know" that
Resource.id.myButton is backed by the __myButton field, short of looking
at -- and understanding (!) -- the generated CodeBehind file. (Lol?)
partial void OnLayoutViewNotFound (int resourceId, Type type)
{
if (resourceId == Resource.Id.myButton) {
__myButton = ....wait, what?
}
}
In short, I see no reason for OnLayoutViewNotFound() to exist. Assuming
the whole reason it exists is to avoid a later NullReferenceException, I
don't see *how it can do that*.
Meaning the only sane approach is to *always throw*.
Meaning we don't need a InitializeContentView(bool throwOnMissingView),
we just need a InitializeContentView() (no parameters at all!).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1238 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAxeeTqGPKsK0potlFUHBaclB6fw_Lw7ks5tSfi8gaJpZM4Rs_BO>
.
|
That's up to the developer. The idea was what @dellis1972 mentioned - to let the developer handle it in some way. It might be manually locating the view with |
|
@grendello: then there needs to be a sane way to actually do that. The current solution is not what I'd call "sane." Again, what should we expect developers to provide within the body of If we want the flexibility, then I suggest we instead do: partial void OnLayoutViewNotFound<TView> (int resourceId, ref TView value);CodeBehind-side, we'd thus have e.g.: private T @__FindView<T>(global::Android.App.Activity parentView, int resourceId) where T : global::Android.Views.View {
T view = parentView.FindViewById<T>(resourceId)
if ((view == null)) {
if (@__throwOnMissingView_LayoutCodeBehind) {
throw new System.InvalidOperationException($"View not found (ID: {resourceId})");
}
else {
this.OnLayoutViewNotFound(resourceId, ref view);
}
}
return view;
}Developer-override wise, now they can actually set the underlying field without "magically" knowing that partial void OnLayoutViewNotFound<TView> (int resourceId, ref TView value)
{
if (resourceId == Resource.Id.myButton) {
// In yada yada yada, we instead use some other resource:
value = FindViewById<TView>(Resource.Id.myRelatedButton);
}
} |
|
The idea was that this method is part of the same class so it has access to all the fields and can write them or throw if it chooses so. Your idea with a |
|
That implies we should generate: private T @__FindView<T>(global::Android.App.Activity parentView, int resourceId)
where T : global::Android.Views.View
{
T view = parentView.FindViewById<T>(resourceId)
if ((view == null)) {
this.OnLayoutViewNotFound(resourceId, ref view);
}
if ((view == null)) {
throw new System.InvalidOperationException($"View not found (ID: {resourceId})");
}
return view;
}This also means that we don't need the |
|
A small nitpick :) private T @__FindView<T>(global::Android.App.Activity parentView, int resourceId)
where T : global::Android.Views.View
{
T view = parentView.FindViewById<T>(resourceId)
if ((view == null)) {
this.OnLayoutViewNotFound(resourceId, ref view);
if (view != null)
return view;
}
throw new System.InvalidOperationException($"View not found (ID: {resourceId})");
}This looks cleaner IMHO :) |
|
@dellis1972 we could move this code from the latest proposed version of if ((view == null)) {
this.OnLayoutViewNotFound(resourceId, ref view);
if (view != null)
return view;
}
throw new System.InvalidOperationException($"View not found (ID: {resourceId})");
}This way the generated source code would be slightly, but always, smaller and leaner |
grendello
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the OnLayoutViewNotFound changes and it'll be perfect :)
|
We also need to update the documentation about semantics and requirements. I'm currently assuming that Documentation also needs to mention |
|
I have found that if you don't implement partial methods, the compiler will
strip them out. So while I will document that `OnLayoutViewNotFound` method,
it won't be required for the user to get up and running.
…On 8 February 2018 at 14:52, Jonathan Pryor ***@***.***> wrote:
We also need to update the documentation about semantics and requirements.
I'm currently *assuming* that InitializeComponentView() *must* be called
first -- because I can't see how it would work, otherwise -- but that
wasn't explicitly called out *anywhere*.
Documentation also needs to mention OnLayoutViewNotFound().
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1238 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAxeebR79sl7IAKKovxUvWHoLBPtmhr-ks5tSwopgaJpZM4Rs_BO>
.
|
762f3e2 to
135b308
Compare
|
@dellis1972: we'll also want an updated copy of the generated codebehind file. |
bcd7647 to
cbfa8ff
Compare
…iles. This commit is based on work done by @grendello. It adds support for the generation of code behind for layout files. This will make it easier for users to write code since they will no longer need to use `FindViewbyId` manually. The generated code will automatically provide properties for the various UI elements in the layout file. For example if you have a Button with an id of `@id/myButton` you will see a property is available on your activity `myButton`. You can then replace code like var button = FindViewById<Button> (Resource.Id.myButton); button.Click += delegate { }; with myButton.Click += delegate { }; much nicer eh :). There are a couple of caviats. The main one is your layout class MUST be a `partial` class. So public class MainActivity : Activity { } becomes public partial class MainActivity : Activity { } Next is you need to add two items to your root layout of your axml/xml files. xmlns:tools="http://schemas.android.com/tools" tools:class="$(Namespace).$(ClassName)" where `$(Namespace)` and `$(ClassName)` are replaced with the approprite values. Note these MUST match the full namespace/classname of the layout class. The following is a sample of the kind of code which will be generated by this new system. //------------------------------------------------------------------------------ // <auto-generated> // This code was generated by a tool. // Runtime Version:4.0.30319.42000 // // Changes to this file may cause incorrect behavior and will be lost if // the code is regenerated. // </auto-generated> //------------------------------------------------------------------------------ namespace UnnamedProject { using System; using Android.App; using Android.Widget; using Android.Views; using Android.OS; // Generated from layout file 'Resources/layout/Main.axml' public partial class MainActivity { private Func<Button> @__myButtonFunc; #line 1 "/Users/dean/Documents/Sandbox/Xamarin/xamarin-android/bin/TestDebug/temp/CheckCodeBehindIsGenerated/Resources/layout/Main.axml" private Button @__myButton; #line default #line hidden partial void OnLayoutViewNotFound<T> (int resourceId, ref T type) where T : global::Android.Views.View; #line 1 "/Users/dean/Documents/Sandbox/Xamarin/xamarin-android/bin/TestDebug/temp/CheckCodeBehindIsGenerated/Resources/layout/Main.axml" public Button myButton { get { if (@__myButtonFunc == null) { @__myButtonFunc = this.@__Create_myButton; } return this.@__EnsureView<Button>(this.@__myButtonFunc, ref this.@__myButton); } } #line default #line hidden private void InitializeContentView() { this.SetContentView(Resource.Layout.Main); } private T @__FindView<T>(global::Android.Views.View parentView, int resourceId) where T : global::Android.Views.View { T view = parentView.FindViewById<T>(resourceId); if ((view == null)) { this.OnLayoutViewNotFound(resourceId, ref view); } if ((view != null)) { return view; } throw new System.InvalidOperationException($"View not found (ID: {resourceId})"); } private T @__FindView<T>(global::Android.App.Activity parentView, int resourceId) where T : global::Android.Views.View { T view = parentView.FindViewById<T>(resourceId); if ((view == null)) { this.OnLayoutViewNotFound(resourceId, ref view); } if ((view != null)) { return view; } throw new System.InvalidOperationException($"View not found (ID: {resourceId})"); } private T @__FindView<T>(global::Android.App.Fragment parentView, int resourceId) where T : global::Android.Views.View { return this.@__FindView<T>(parentView.Activity, resourceId); } private T @__EnsureView<T>(System.Func<T> creator, ref T field) where T : class { if ((field != null)) { return field; } if ((creator == null)) { throw new System.ArgumentNullException(nameof (creator)); } field = creator(); return field; } private Button @__Create_myButton() { return this.@__FindView<Button>(this, Resource.Id.myButton); } } } ff
cbfa8ff to
33404a0
Compare
|
Build hung deploying to device. Restarted the build. |
Context: https://github.com/actions/virtual-environments/blob/5cdc2e5f50ec21dbac1d3b475ae0fe364f4a2d12/images/linux/Ubuntu2004-Readme.md#environment-variables-3 Context: https://github.com/actions/virtual-environments/blob/5cdc2e5f50ec21dbac1d3b475ae0fe364f4a2d12/images/win/Windows2022-Readme.md#environment-variables-2 Context: https://github.com/actions/virtual-environments/blob/5cdc2e5f50ec21dbac1d3b475ae0fe364f4a2d12/images/macos/macos-11-Readme.md#environment-variables-2 Changes: xamarin/monodroid@91b4ef9...297767f * xamarin/monodroid@297767fcc: [build] Use Azure Pipelines vars for Android SDK/NDK paths (#1238) * xamarin/monodroid@9e83ad1e1: Bump to xamarin/xamarin-android/main@6a177eaa (#1237) Build and test against Android SDK and NDK paths set by Azure Pipelines hosted images, if those paths exist. This should help reduce our disk space requirements on CI.
This commit is based on work done by @grendello. It adds support
for the generation of code behind for layout files. This will
make it easier for users to write code since they will no longer
need to use
FindViewbyIdmanually. The generated code willautomatically provide properties for the various UI elements
in the layout file.
For example if you have a Button with an id of
@id/myButtonyouwill see a property is available on your activity
myButton. Youcan then replace code like
with
much nicer eh :).
There are a couple of caviats. The main one is your layout class
MUST be a
partialclass. Sobecomes
Next is you need to add two items to your root layout of your
axml/xml files.
where
$(Namespace)and$(ClassName)are replaced with theapproprite values. Note these MUST match the full namespace/classname
of the layout class.
The following is a sample of the kind of code which will be
generated by this new system.