Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Jul 11, 2023

Fixes #7729

We do not currently enumify PackageManager.PackageInfoFlags.Of (long) (source) because it is a long and our tooling is built around enumifying int values.

However, it seems to be a relatively used API so we should try to make it nicer.

Create a new PackageInfoFlagsLong long enum that can be used as a parameter to a new Of overload.

@jpobst jpobst force-pushed the packageinfoflagslong branch from 08b68ca to d95f509 Compare July 25, 2023 20:45
@jpobst jpobst marked this pull request as ready for review July 26, 2023 01:35
@jpobst jpobst requested a review from jonpryor as a code owner July 26, 2023 01:35
// 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
Copy link
Contributor

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?

Copy link
Contributor Author

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Two thoughts/questions:

  1. Instead of hardcoding the values ("from documentation"), could this instead re-use our existing enum for existing non-long values?

    public enum PackageInfoFlagsLong : long {
        GetActivities = PackageInfoFlags.Activitites,
        GetServices = PackageInfoFlags.Services,
        // …
    }
  2. Why the "naming change" between PackageInfoFlags and PackageInfoLongFlags? PackageInfoFlags.Activities is PackageInfoLongFlags.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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Good thought, updated.
  2. 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.

@jpobst
Copy link
Contributor Author

jpobst commented Jul 27, 2023

Also added [ObsoletedOSPlatformAttribute]'s as specified in Android docs.

@jonpryor
Copy link
Contributor

jonpryor commented Aug 7, 2023

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)

@jonpryor jonpryor merged commit 569b7c0 into main Aug 7, 2023
@jonpryor jonpryor deleted the packageinfoflagslong branch August 7, 2023 20:08
grendello added a commit to grendello/xamarin-android that referenced this pull request Aug 8, 2023
* 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)
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 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.

Enumify PackageManager.PackageInfoFlags.Of

3 participants