-
Notifications
You must be signed in to change notification settings - Fork 561
[Mono.Android] Provide enum overload of PackageManager.PackageInfoFlags.Of.
#8182
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
08b68ca to
d95f509
Compare
| // Manually created "long" version of "PackageInfoFlags" enum, created from documentation: | ||
| // https://developer.android.com/reference/android/content/pm/PackageManager.PackageInfoFlags#of(long) | ||
| [System.Flags] | ||
| public enum PackageInfoLongFlags : long |
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.
Should this instead be named PackageInfoFlagsLong, as IntelliSense code completion for tokens starting with PackageInfoFlags would then show PackageInfoFlagsLong as possible completions?
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 you've already typed out PackageInfoFlags then you aren't really using Intellisense! 😁
Generally you don't have to type any part of an enum type name because Intellisense is already showing it to you because it knows the parameter type.
Regardless, either seems fine, changed.
| { | ||
| // Create overloads that accept PackageInfoLongFlags | ||
| [global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android33.0")] | ||
| public static unsafe Android.Content.PM.PackageManager.PackageInfoFlags Of (PackageInfoLongFlags value) |
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.
Why are Of() and ValueAsFlags are marked as unsafe?
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.
Copy/paste error, removed.
| { | ||
| None = 0, | ||
|
|
||
| GetActivities = 1, |
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.
Two thoughts/questions:
-
Instead of hardcoding the values ("from documentation"), could this instead re-use our existing enum for existing non-
longvalues?public enum PackageInfoFlagsLong : long { GetActivities = PackageInfoFlags.Activitites, GetServices = PackageInfoFlags.Services, // … }
-
Why the "naming change" between
PackageInfoFlagsandPackageInfoLongFlags?PackageInfoFlags.ActivitiesisPackageInfoLongFlags.GetActivities, etc. On the one hand this "new" convention may be "better", but it also means that migrating from the old enum to the new enum will require additional edits; you can't "just" change the enum name.
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.
- Good thought, updated.
- This seemed like a good opportunity to fix a "mistake" from the past.
I don't know that changing GetDisabledComponents to DisabledComponents was ever a good idea, but now that MatchDisabledComponents (and others) have been added, it seems too confusing to have one of them missing the verb.
|
Also added |
Fixes: https://github.com/xamarin/xamarin-android/issues/7729
We do not currently enumify
[`PackageManager.PackageInfoFlags.of(long)`][0] because it is a
`long` and our tooling is built around enumifying `int` values.
However, it seems to be a frequently used API, so we should try to
make it nicer.
Create a new `PackageInfoFlagsLong` enum which has `long` as the
underlying type, which can be used as a parameter to a new
`PackageManager.PackageInfoFlags.Of(PackageInfoFlagsLong)` overload:
var flags = PackageManager.PackageInfoFlags.Of(
PackageInfoFlagsLong.GetActivities |
PackageInfoFlagsLong.GetSignatures
);
// vs previous
var old_flags = PackageManager.PackageInfoFlags.Of(
(long)(PackageInfoFlags.Activities | PackageInfoFlags.Signatures)
);
Note that the enumeration member naming convention changed, e.g.
from `PackageInfoFlags.Activities` to
`PackageInfoFlagsLong.GetActivities`. This was done because the
verbs used by `PackageInfoFlags` members were not consistent; some
members had an "implicit `Get`" e.g. `PackageInfoFlags.Activities`
(which came from `PackageManager.GET_ACTIVITIES`), while others had
an "explicit" verb, e.g. `PackageInfoFlags.MatchUninstalledPackages`
(which came from `PackageManager.MATCH_UNINSTALLED_PACKAGES`).
The new `PackageInfoFlagsLong` convention is to consistently require
the verb.
[0]: https://developer.android.com/reference/android/content/pm/PackageManager.PackageInfoFlags#of(long) |
* main: Bump com.android.tools:r8 from 8.0.40 to 8.1.56 (dotnet#8240) Bump to dotnet/installer@1289a84a55 8.0.100-rc.1.23404.2 (dotnet#8242) [Mono.Android] add "built-in" delegate for _JniMarshal_PPII_L (dotnet#8245) [Mono.Android] Add `PackageInfoFlagsLong` (dotnet#8182)
Fixes #7729
We do not currently enumify
PackageManager.PackageInfoFlags.Of (long)(source) because it is alongand our tooling is built around enumifyingintvalues.However, it seems to be a relatively used API so we should try to make it nicer.
Create a new
PackageInfoFlagsLonglongenum that can be used as a parameter to a newOfoverload.