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

[Mono.Android] API-compatible CellInfo.CellIdentity #4391

Merged
merged 1 commit into from
Mar 13, 2020

Conversation

jonpryor
Copy link
Member

Add Documentation/workflow/mono-android-api-compatibility.md, which
describes the scenarios we need to be familiar with regarding Java and
C# API compatibility differences and how to deal with them, and update
Mono.Android.dll for API-R so that e.g.
Android.Telephony.CellInfoGsm.CellIdentity doesn't change types vs.
API-29 (the previous version).

@jonpryor jonpryor requested review from gugavaro and jpobst March 13, 2020 01:40
@jonpryor jonpryor force-pushed the jonp-doc-api-compat branch 2 times, most recently from 7690c41 to 5755259 Compare March 13, 2020 14:40
// Metadata.xml XPath method reference: path="/api/package[@name='android.telephony']/class[@name='CellInfo']/method[@name='getCellIdentity' and count(parameter)=0]"
[Register ("getCellIdentity", "()Landroid/telephony/CellIdentity;", "GetGetCellIdentityHandler", ApiSince = 30)]
get {
const string __id = "getCellIdentity.()Landroid/telephony/CellIdentity;";
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very interesting solution to solve this break. One thing that comes to my mind is if this code inside the get will ever be executed? In which condition this path would run? Would be safe to actually assert inside this get?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it can be executed. Consider:

CellInfo     cell     = new CellInfoCdma ();
CellIdentity identity = cell.CellIdentity;

CellInfoCdma does not override the CellInfo.CellIdentity property. It can't, because in order to do so it would need to change the property type, and changing the property type would be a compatibility break.

Now, a downside to this approach is that if you build your app against API-R and use the CellInfo.CellIdentity property, then run the app on e.g. an API-29 device, cell.CellIdentity will throw, because API-29 doesn't have a CellInfo.getCellIdentity() method.

What I don't know is if this behavior is consistent with Java. I assume it would be, but haven't tested. (Could someone please test this? :-)

Even if this behavior is consistent with Java, it's kinda "less than ideal", because there is a CellInfoCdma.CellIdentity property which does work on API-29, so why should this throw?

If we think that cell.CellIdentity should work on API-29, then we need an indirection:

partial class CellIdentity {
    public virtual Android.Telephony.CellIdentity CellIdentity {
        [Register (...)]
        get => _GetCellIdentityCore ();

    internal virtual Android.Telephony.CellIdentity _GetCellIdentityCore ()
    {
        const string __id = "getCellIdentity.()Landroid/telephony/CellIdentity;";
        try {
            var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, null);
            return global::Java.Lang.Object.GetObject<Android.Telephony.CellIdentity> (__rm.Handle, JniHandleOwnership.TransferLocalRef);
        }
        catch (Java.Lang.NoSuchMethodError) {
            throw new Java.Lang.AbstractMethodError (__id);
        }
    }
}

sealed partial class CellIdentityCdma {
    public new Android.Telephony.CellIdentityCdma CellIdentity { get; }

    internal override Android.Telephony.CellIdentity _GetCellIdentityCore () => CellIdentity;
}

@@ -1539,16 +1539,8 @@
<attr api-since="30" path="/api/package[@name='android.view.inline']" name="managedName">Android.Views.Inline</attr>

<!-- Google has added getCellIdentity and getCellSignalStrength as abstract method on the base class -->
Copy link
Contributor

@jpobst jpobst Mar 13, 2020

Choose a reason for hiding this comment

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

Suggested change
<!-- Google has added getCellIdentity and getCellSignalStrength as abstract method on the base class -->
<!-- Google has added getCellIdentity and getCellSignalStrength as abstract method on the base class and changed return type.
Remove these here and manually bind to preserve compatibility. -->

Add `Documentation/workflow/mono-android-api-compatibility.md`, which
describes the scenarios we need to be familiar with regarding Java and
C# API compatibility differences and how to deal with them, and update
`Mono.Android.dll` for API-R so that e.g.
`Android.Telephony.CellInfoGsm.CellIdentity` doesn't change types vs.
API-29 (the previous version).
@jonpryor jonpryor force-pushed the jonp-doc-api-compat branch from 5755259 to e869685 Compare March 13, 2020 16:03
@jonpryor jonpryor merged commit 1f4c8be into dotnet:master Mar 13, 2020
jonpryor added a commit that referenced this pull request Mar 15, 2020
Add `Documentation/workflow/mono-android-api-compatibility.md`, which
describes the scenarios we need to be familiar with regarding Java and
C# API compatibility differences and how to deal with them, and update
`Mono.Android.dll` for API-R so that e.g.
`Android.Telephony.CellInfoGsm.CellIdentity` doesn't change types vs.
API-29 (the previous version).
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants