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] Fix WindowManagerLayoutParams.SystemUiVisibility enumification #7730

Merged
merged 3 commits into from
Jan 27, 2023

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Jan 23, 2023

Fixes #4290

The following API were incorrectly enumified as StatusBarVisibility instead of SystemUiFlags:

  • android.views.View.[get|set]SystemUiVisibility (int) (source)
  • android.views.WindowManager.LayoutParams.SystemUiVisibility (source)

As these are properties, we cannot create an overload. Instead create a new property called SystemUiFlags and obsolete the incorrect property. Additionally, this whole thing was deprecated in API-30, so make sure we reflect that on the new method. It's still worth fixing this, as people will be using this for pre-API-30 device support for years to come.

API Reference Results:

WindowManagerLayoutParams
image

View
image

@jpobst jpobst marked this pull request as ready for review January 24, 2023 21:29
@jpobst jpobst requested a review from jonpryor as a code owner January 24, 2023 21:29
{
partial class WindowManagerLayoutParams
{
#if NET
Copy link
Member

Choose a reason for hiding this comment

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

Given that we'll eventually do a net7.0-android34.0 release, requiring cherry-picks to a release/7.0* branch, should this instead be:

#if NET && ANDROID_34

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guarded just in case. However, I think we should expect net7.0-android34.0 API may not exactly match net8.0-android34.0 and we shouldn't backport these fixes.

I don't suspect a user upgrading to .NET 8, using new API, and then wanting to downgrade their app to .NET 7 is a common scenario.

Copy link
Member

Choose a reason for hiding this comment

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

@jpobst: I'm more concerned about the opposite: customer starts with net7.0-android34.0 -- probably released in August or September! -- then migrating to net8.0-android34.0 in November.

Then there is ABI compatibility: libraries built against net7.0-android34.0 should work as-is in a net8.0-android34.0 app. Will it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should not be an issue with this PR, as it only adds API to newer versions, but it is good to keep that in mind in general.

@jonpryor
Copy link
Member

Re-reading this, and I feel like this is incomplete?

The PR description states:

WindowManagerLayoutParams.SystemUiVisibiliy (source) was incorrectly enumified as StatusBarVisibility instead of SystemUiFlags.

However, the source link is to View.setSystemUiVisibility(), while it mentions WindowManagerLayoutParams.SystemUiVisibiliy. These are not the same.

Similarly, the PR contents adds a WindowManagerLayoutParams.SystemUiFlags property, but issue #4290 was about View.SystemUiVisibiliy.

Shouldn't this change hit both WindowManagerLayoutParams and View?

@jonpryor
Copy link
Member

jonpryor commented Jan 26, 2023

Fixes: https://github.com/xamarin/xamarin-android/issues/4290

The [`View.getSystemUiVisibility()` method][0], bound as the
[`View.SystemUiVisibility` property][1], and the
[`WindowManager.LayoutParams.systemUiVisibility` field][2], bound as
the [`WindowManagerLayoutParams.SystemUiVisibility` property][3],
were incorrectly enumified to use the `StatusBarVisibility` enum when
they should have used the `SystemUiFlags` enum.

We cannot change the existing members, as that would be an ABI and
API break.  Furthermore, as these are properties, we cannot overload
them either.

"Fix" this by:

 1. Marking the existing properties as `[Obsolete]`, and
 2. Introduce *new* `SystemUiFlags` properties.

Additionally, these members were deprecated in API-30, so make sure
we reflect that on the new method.  It's still worth fixing this, as
people will be using this for pre-API-30 device support for years to
come.

These changes will only be present in API-34+.
The resulting API is:

	partial class View {
	    // Original property
	    [Obsolete ("This property has an incorrect enumeration type. Use the SystemUiFlags property instead.")]
	    public StatusBarVisibility SystemUiVisibility {get; set;}

	    // New hawtness
	    [ObsoletedOSPlatform ("android30.0", "…")]
	    public SystemUiFlags SystemUiFlags {get; set;}
	}
	partial class WindowManagerLayoutParams {
	    // Original property
	    [Obsolete ("This property has an incorrect enumeration type. Use the SystemUiFlags property instead.")]
	    public StatusBarVisibility SystemUiVisibility {get; set;}

	    // New hawtness
	    [ObsoletedOSPlatform ("android30.0", "…")]
	    public SystemUiFlags SystemUiFlags {get; set;}
	}

[0]: https://developer.android.com/reference/android/view/View#getSystemUiVisibility()
[1]: https://learn.microsoft.com/en-us/dotnet/api/android.views.view.systemuivisibility?view=xamarin-android-sdk-12
[2]: https://developer.android.com/reference/android/view/WindowManager.LayoutParams#systemUiVisibility
[3]: https://learn.microsoft.com/en-us/dotnet/api/android.views.windowmanagerlayoutparams.systemuivisibility?view=xamarin-android-sdk-12

@jonpryor jonpryor merged commit c1752ca into main Jan 27, 2023
@jonpryor jonpryor deleted the system-ui-flags branch January 27, 2023 17:02
grendello added a commit to grendello/xamarin-android that referenced this pull request Jan 30, 2023
* main:
  [Xamarin.Android.Build.Tasks] Fix issue where app will not install (dotnet#7719)
  Bump to dotnet/installer@779a644 8.0.100-alpha.1.23070.23 (dotnet#7728)
  LEGO: Merge pull request 7751
  [Mono.Android] Wrap connection exceptions in HttpRequestException (dotnet#7661)
  [Mono.Android] Fix View.SystemUiVisibility enumification (dotnet#7730)
grendello added a commit to grendello/xamarin-android that referenced this pull request Jan 30, 2023
* main:
  [Xamarin.Android.Build.Tasks] Fix issue where app will not install (dotnet#7719)
  Bump to dotnet/installer@779a644 8.0.100-alpha.1.23070.23 (dotnet#7728)
  LEGO: Merge pull request 7751
  [Mono.Android] Wrap connection exceptions in HttpRequestException (dotnet#7661)
  [Mono.Android] Fix View.SystemUiVisibility enumification (dotnet#7730)
  Bump r8 from 3.3.75 to 4.0.48 (dotnet#7700)
  [monodroid] Prevent overlapped decompression of embedded assemblies (dotnet#7732)
  [xaprepare] Support arm64 emulator components (dotnet#7743)
  Bump SQLite to 3.40.1 (dotnet#7733)
  Bump to xamarin/xamarin-android-binutils/L_15.0.7-5.0.3@6721af4b (dotnet#7742)
  [monodroid] Replace `exit()` with `abort()` in native code (dotnet#7734)
  Bump to xamarin/Java.Interop/main@8a1ae57 (dotnet#7738)
  [build] bump `$(AndroidNet7Version)` (dotnet#7737)
  Bump to xamarin/Java.Interop/main@1366d99 (dotnet#7718)
  [Xamarin.Android.Build.Tasks] fix AndroidGenerateResourceDesigner (dotnet#7721)
  Bump to xamarin/monodroid@50faac94 (dotnet#7725)
grendello added a commit to grendello/xamarin-android that referenced this pull request Feb 9, 2023
* main: (112 commits)
  [ci] Remove classic Mono.Android-Tests runs (dotnet#7778)
  $(AndroidPackVersionSuffix)=preview.2; net8 is 34.0.0-preview.2 (dotnet#7761)
  Localized file check-in by OneLocBuild Task (dotnet#7758)
  Bump to dotnet/installer@dec1209 8.0.100-alpha.1.23080.11 (dotnet#7755)
  LEGO: Merge pull request 7756
  Localized file check-in by OneLocBuild (dotnet#7752)
  [Xamarin.Android.Build.Tasks] Fix issue where app will not install (dotnet#7719)
  Bump to dotnet/installer@779a644 8.0.100-alpha.1.23070.23 (dotnet#7728)
  LEGO: Merge pull request 7751
  [Mono.Android] Wrap connection exceptions in HttpRequestException (dotnet#7661)
  [Mono.Android] Fix View.SystemUiVisibility enumification (dotnet#7730)
  Bump r8 from 3.3.75 to 4.0.48 (dotnet#7700)
  [monodroid] Prevent overlapped decompression of embedded assemblies (dotnet#7732)
  [xaprepare] Support arm64 emulator components (dotnet#7743)
  Bump SQLite to 3.40.1 (dotnet#7733)
  Bump to xamarin/xamarin-android-binutils/L_15.0.7-5.0.3@6721af4b (dotnet#7742)
  [monodroid] Replace `exit()` with `abort()` in native code (dotnet#7734)
  Bump to xamarin/Java.Interop/main@8a1ae57 (dotnet#7738)
  [build] bump `$(AndroidNet7Version)` (dotnet#7737)
  Bump to xamarin/Java.Interop/main@1366d99 (dotnet#7718)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 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.

Is View.SystemUiVisibility bound to View.StatusBarVisibility incorrectly?
2 participants