Fixed

Feature

Status Update

No update yet.

Description

mg...@google.com

created issue #1

Feb 28, 2025 11:36AM

JetBrains has asked if we could support a get*OrNull() variant for our methods. The current get*OrElse() does not allow returning null, and checking isNull beforehand is both unnecessary and inefficient.

With AOSP/3500556 change to reduce excessive inlining, we plan to move from inline to non-inline functions, as benchmarks show minimal performance impact. This works well for get* functions, but get*OrElse() would allocate lambdas if we don't use inline, which we don't want to.

Here are the options before the beta:

Option 1: Do not support get*OrNull().
- Downsides: (1) checking for isNull and contains before a get runs at 60ns instead of 20ns and (2) wouldn’t be able to remove inline from get*OrElse() due to lambda allocations.
Option 2a: Support get*OrNull(), which also allows migrating get*OrElse() out of inline functions.
- The final API would look like this:

fun getOrNull(key): T?  
fun get(key): T = getOrNull() ?: throw  
fun inline getOrElse(key, else): T = getOrNull() ?: else() // avoids lambda allocation

Option 2b: Same as option 2a, but we remove get*OrElse() since users can simply call getOrNull() ?: else().
- Downside: JetBrains has already migrated their navigation fork to use get*OrElse(), so this would require extra work on their side.

getOrNull() seems useful, and Bundle always boxes primitives anyway, since everything is stored in a Map<K, V>.

Edit 03.03.25:

Option 3: Same as Option 2b (including downsides), but without the non-nullable variant. We keep only get*(), and change to return a @Nullable value.
- Advantage: (1) Consistent with Kotlin's generic collections get function (Map). (2) Reduces API surface by half.
- Downside: (1) It's disruptive, requiring changes to all usages. (2) SavedState is a specialized Map, not a general-purpose one, so if an ACTION_FOO expects EXTRA_FOO_ARG1 and EXTRA_FOO_ARG2, their absence means execution can't proceed. Throwing an error for a missing key is (arguably) reasonable, while get*OrNull()/get*OrElse() provide an explicit opt-out mechanism for cases where a null or default value is acceptable.

Comments

ku...@google.com <ku...@google.com> #2Feb 28, 2025 12:02PM

I highly support this, this way every current FormGroup subclass can define only the functions it needs (Everything besides Address can get rid of the AutofillType API and of the app_locale parameters, many classes could also get rid of the VerificationStatus API, etc.)

AutofillProfile could then implement a GetDataForType(FieldType) -> absl::variant<Address, NameInfo, ...> and provide the current API with visit calls instead of polymorphism.

Message last modified on Feb 28, 2025 02:16PM

ku...@google.com <ku...@google.com> #3Feb 28, 2025 12:17PM

Relevant prior discussion: https://chromium-review.googlesource.com/c/chromium/src/+/6197978/comments/3ac57e7a_d168059a

Message last modified on Feb 28, 2025 02:16PM

ku...@google.com <ku...@google.com> #4Mar 3, 2025 10:19AM

Re the discussion from #comment3:

It's true that the polymorphism isn't used, but isn't desirable to have a consistent interface among data model classes regardless?

I agree an interface has some appeal as a tool to define a contract even if the polymorphism isn't needed. But I'd say in C++ it's not worth the cost that comes with inheritance. By cost I mean the general sketchiness, the code-search complexity, and very concretely that we can't default operator==() safely (which is what led me to file this bug). In the case of the SingleFieldFormFiller interface, removing an "unused" interface was I think positive in every way (issue 374266445).

I'd rather use a concept plus static_assert to express that multiple classes define the same API, if we don't need run-time polymorphism.

FormGroup gives us a consistent way to read/write data from/to all Autofill objects.

In my view*, FormGroup is only on the surface a "consistent way to read/write data from/to all Autofill objects". I think it fails at defining a clear contract that's uniformly met by the implementations because there are so many similarly named functions, some of which may be useless because they don't use some parameters. As a result, in my experience, I have to look at every implementation anyway to figure out what it does. And the implementation inheritance IMO makes this a lot annoying because I'm always afraid that I'm missing some override.

This is useful by itself, but also enables easy composition of FormGroups.

It looks to me like we can do the same easily with absl::variant, but I haven't tried it.

*: Admittedly I haven't used FormGroup that much yet, but I've found myself looking at it on code search a lot of times over the past couple of weeks.

ko...@jetbrains.com <ko...@jetbrains.com> #5Mar 3, 2025 12:54PM

From the discussion in comment #3:

If we've already gathered some attention around this important topic, I will create a design document to discuss possible solutions. I think we agree that the FormGroup class is not well designed, but we have different ideas about what to do with it.

Piotr, did you already have a chance to create such a doc?

Message last modified on Mar 3, 2025 12:54PM

mg...@google.com <mg...@google.com> #6Mar 3, 2025 10:13PM

Accepted by mg...@google.com.

I haven't had a chance to create the design document yet, but I still intend to do so. Given that this topic is still actively being discussed and raised, I'll increase the priority of creating it.

Message last modified on Mar 4, 2025 12:18PM

ap...@google.com <ap...@google.com> #7Mar 5, 2025 11:06PM

Project: platform/frameworks/support
Branch: androidx-main
Author: Marcello Galhardo <mgalhardo@google.com>
Link: https://android-review.googlesource.com/3523154

Combine keyNotFoundError and valueNotFoundError

Expand for full commit details

Combine `keyNotFoundError` and `valueNotFoundError` 
 
RelNote: "N/A" 
Bug: 399820614 
Fixes: 399818282 
Test: SavedStateCodecTest 
Test: SavedStateTest 
Test: SavedStateAndroidTest 
Change-Id: I518f38e4caa3063cb1e38fd763db5d030580f338

Files:

M savedstate/savedstate/api/restricted_current.txt
M savedstate/savedstate/bcv/native/current.txt
M savedstate/savedstate/src/androidMain/kotlin/androidx/savedstate/SavedStateReader.android.kt
M savedstate/savedstate/src/androidUnitTest/kotlin/androidx/savedstate/SavedStateAndroidTest.android.kt
M savedstate/savedstate/src/commonMain/kotlin/androidx/savedstate/SavedStateReader.kt
M savedstate/savedstate/src/commonTest/kotlin/androidx/savedstate/SavedStateCodecTest.kt
M savedstate/savedstate/src/commonTest/kotlin/androidx/savedstate/SavedStateTest.kt
M savedstate/savedstate/src/nonAndroidMain/kotlin/androidx/savedstate/SavedStateReader.nonAndroid.kt

Hash: 40ca7f18520404f04bfded6477a45685fc157078
Date: Tue Mar 04 00:02:23 2025

ap...@google.com <ap...@google.com> #8Mar 5, 2025 11:06PM

Project: platform/frameworks/support
Branch: androidx-main
Author: Marcello Galhardo <mgalhardo@google.com>
Link: https://android-review.googlesource.com/3527932

Fix missing get*OrElse in navigation.common

Expand for full commit details

Fix missing `get*OrElse` in `navigation.common` 
 
- Pre-submit runs against ToT (max_version). 
- The new SavedState does not include a `get*OrElse`, causing navigation to break. 
 
Bug: 399820614 
Test: N/A 
Change-Id: Ife25463e2881342df4eae4221e9ef59b1e19d095

Files:

M navigation/navigation-common/build.gradle
M navigation/navigation-runtime/src/androidMain/kotlin/androidx/navigation/NavController.android.kt

Hash: d62bb24b2575eb79280e67746e8c1ceb5a9d53c6
Date: Tue Mar 04 14:54:20 2025

ap...@google.com <ap...@google.com> #9Mar 5, 2025 11:06PM

Project: platform/frameworks/support
Branch: androidx-main
Author: Marcello Galhardo <mgalhardo@google.com>
Link: https://android-review.googlesource.com/3523151

Add getOrNull in SavedStateReader

Expand for full commit details

Add `getOrNull` in `SavedStateReader` 
 
- Introduces a `getOrNull` variant behaves as: 
  a. Key exists & type match == returns value. 
  b. Key doesn't exist == returns null. 
  c. Value is null (putNull) == returns null. 
- Please note that `getOrNull` for primitives will cause auto-boxing. 
 
RelNote: "Add `getOrNull` alternative methods for `SavedStateReader.get` variants." 
Bug: 399820614 
Test: SavedStateTest 
Test: SavedStateAndroidTest 
Change-Id: I6228c173c45e6137de7d4885550285de5e50ae15

Files:

M savedstate/savedstate/api/current.txt
M savedstate/savedstate/api/restricted_current.txt
M savedstate/savedstate/bcv/native/current.txt
M savedstate/savedstate/src/androidMain/kotlin/androidx/savedstate/SavedStateReader.android.kt
M savedstate/savedstate/src/androidUnitTest/kotlin/androidx/savedstate/SavedStateAndroidTest.android.kt
M savedstate/savedstate/src/commonMain/kotlin/androidx/savedstate/SavedStateReader.kt
M savedstate/savedstate/src/commonTest/kotlin/androidx/savedstate/SavedStateTest.kt
M savedstate/savedstate/src/nonAndroidMain/kotlin/androidx/savedstate/SavedStateReader.nonAndroid.kt

Hash: cd71b1c721a8cf9733aed3bea5361f528011641c
Date: Mon Mar 03 23:39:02 2025

ap...@google.com <ap...@google.com> #10Mar 5, 2025 11:06PM

Project: platform/frameworks/support
Branch: androidx-main
Author: Marcello Galhardo <mgalhardo@google.com>
Link: https://android-review.googlesource.com/3523153

Remove inline from SavedState

Expand for full commit details

Remove `inline` from `SavedState` 
 
- The changes in aosp/3500556 increased the amount of inlined code, raising concerns about excessive inlining in client code. 
- Benchmarking showed no significant performance difference between using `inline` and non-`inline` method calls. 
- Based on these findings, we decided to remove `inline` from `SavedStateReader` to reduce inlining and allow logic sharing without relying on `@PublishedApi`, which remains public for backward compatibility. 
 
RelNote: "Remove `inline` from `SavedStateReader` and `SavedStateWriter` methods." 
Bug: 399820614 
Test: SavedStateCodecTest 
Change-Id: If2a0264a865f5d1d0a1298903c83fa02cbd6a99e

Files:

M savedstate/savedstate/api/current.txt
M savedstate/savedstate/api/restricted_current.txt
M savedstate/savedstate/bcv/native/current.txt
M savedstate/savedstate/src/androidMain/kotlin/androidx/savedstate/SavedStateReader.android.kt
M savedstate/savedstate/src/androidMain/kotlin/androidx/savedstate/SavedStateWriter.android.kt
M savedstate/savedstate/src/androidUnitTest/kotlin/androidx/savedstate/SavedStateCodecAndroidTest.android.kt
M savedstate/savedstate/src/commonMain/kotlin/androidx/savedstate/SavedStateReader.kt
M savedstate/savedstate/src/commonMain/kotlin/androidx/savedstate/SavedStateWriter.kt
M savedstate/savedstate/src/commonTest/kotlin/androidx/savedstate/SavedStateCodecTest.kt
M savedstate/savedstate/src/nonAndroidMain/kotlin/androidx/savedstate/SavedStateReader.nonAndroid.kt
M savedstate/savedstate/src/nonAndroidMain/kotlin/androidx/savedstate/SavedStateWriter.nonAndroid.kt

Hash: f09d0e201cae0bbbc77e28b0ecf6245425d65da6
Date: Mon Mar 03 23:47:41 2025

ap...@google.com <ap...@google.com> #11Mar 5, 2025 11:06PM

Project: platform/frameworks/support
Branch: androidx-main
Author: Marcello Galhardo <mgalhardo@google.com>
Link: https://android-review.googlesource.com/3523152

Remove getOrElse in SavedStateReader

Expand for full commit details

Remove `getOrElse` in `SavedStateReader` 
 
- Because `getOrNull()` is now available, users can achieve the same behavior with `getOrNull() ?: else()`, allowing us to reduce the public API surface and the number of inline methods (as `getOrElse` takes a lambda, it cannot be removed). 
 
RelNote: "Remove `getOrElse` from `SavedStateReader` in favor of `getOrNull() ?: else()`." 
Bug: 399820614 
Test: SavedStateTest 
Change-Id: I87317b0eaa0f6d0fad334cc3f81bfcc8a90e6313

Files:

M lifecycle/lifecycle-viewmodel-savedstate/src/commonMain/kotlin/androidx/lifecycle/SavedStateHandleSupport.kt
M savedstate/savedstate/api/current.txt
M savedstate/savedstate/api/restricted_current.txt
M savedstate/savedstate/bcv/native/current.txt
M savedstate/savedstate/src/androidMain/kotlin/androidx/savedstate/Recreator.android.kt
M savedstate/savedstate/src/androidMain/kotlin/androidx/savedstate/SavedStateReader.android.kt
M savedstate/savedstate/src/androidUnitTest/kotlin/androidx/savedstate/SavedStateAndroidTest.android.kt
M savedstate/savedstate/src/commonMain/kotlin/androidx/savedstate/SavedStateReader.kt
M savedstate/savedstate/src/commonTest/kotlin/androidx/savedstate/SavedStateTest.kt
M savedstate/savedstate/src/nonAndroidMain/kotlin/androidx/savedstate/SavedStateReader.nonAndroid.kt

Hash: e04b610835745492aa66aa45be1426d57c1401b6
Date: Mon Mar 03 23:42:23 2025

ap...@google.com <ap...@google.com> #12Mar 5, 2025 11:06PM

Project: platform/frameworks/support
Branch: androidx-main
Author: Marcello Galhardo <mgalhardo@google.com>
Link: https://android-review.googlesource.com/3523155

Add non-reified get collections in SavedStateReader

Expand for full commit details

Add non-reified `get` collections in `SavedStateReader` 
 
- Previously, retrieving a typed collection required a `reified` method, restricting its usage at runtime. 
- Introducing a non-reified variant allows clients to call it with a dynamic type. 
- This change also enables code encapsulation by reducing inlined code, with the reified method simply delegating to the non-reified one. 
 
RelNote: "Add non-reified methods for `get` collections in `SavedStateReader`." 
Bug: 399820614 
Test: N/A 
Change-Id: I0b641cba421761ee41bb8dc386cbc5db7cb67066

Files:

M savedstate/savedstate/api/current.txt
M savedstate/savedstate/api/restricted_current.txt
M savedstate/savedstate/src/androidMain/kotlin/androidx/savedstate/SavedStateReader.android.kt
M savedstate/savedstate/src/androidUnitTest/kotlin/androidx/savedstate/SavedStateAndroidTest.android.kt
M savedstate/savedstate/src/nonAndroidMain/kotlin/androidx/savedstate/SavedStateReader.nonAndroid.kt

Hash: 481a2c575f9c2c3a71e99cf61f093d079d69e1bc
Date: Tue Mar 04 10:55:25 2025

ap...@google.com <ap...@google.com> #13Mar 5, 2025 11:18PM

Android API Change

This bug was referenced by a recent CL that changed the Android API surface area.

The Android API Council regularly reviews API changes for consistency and sustainability, and we've just added this bug to our hotlist of pending reviews.

We'll wait until you mark this bug as 'Fixed' before starting our review, but please reach out if you'd like us to review it sooner.

Changes to savedstate/savedstate/api/current.txt

androidx.savedstate
androidx.savedstate.SavedStateReader

http://go/agw/platform/frameworks/support/+/481a2c575f9c2c3a71e99cf61f093d079d69e1bc%5E%21
https://android-review.googlesource.com/3523155

[Gerrit:https://android-review.googlesource.com/3523155]
[API-Approvers:ilake@google.com]
[Branch:androidx-main]

[LIBRARY_API_REVIEW_TAG:savedstate/savedstate/api/current.txt]

ap...@google.com <ap...@google.com> #14Mar 5, 2025 11:18PM

Android API Change

This bug was referenced by a recent CL that changed the Android API surface area.

The Android API Council regularly reviews API changes for consistency and sustainability, and we've just added this bug to our hotlist of pending reviews.

We'll wait until you mark this bug as 'Fixed' before starting our review, but please reach out if you'd like us to review it sooner.

Changes to savedstate/savedstate/api/current.txt

androidx.savedstate
androidx.savedstate.SavedStateReader
androidx.savedstate.SavedStateWriter

http://go/agw/platform/frameworks/support/+/f09d0e201cae0bbbc77e28b0ecf6245425d65da6%5E%21
https://android-review.googlesource.com/3523153

[Gerrit:https://android-review.googlesource.com/3523153]
[API-Approvers:ilake@google.com]
[Branch:androidx-main]

[LIBRARY_API_REVIEW_TAG:savedstate/savedstate/api/current.txt]

ap...@google.com <ap...@google.com> #15Mar 5, 2025 11:18PM

Android API Change

This bug was referenced by a recent CL that changed the Android API surface area.

The Android API Council regularly reviews API changes for consistency and sustainability, and we've just added this bug to our hotlist of pending reviews.

We'll wait until you mark this bug as 'Fixed' before starting our review, but please reach out if you'd like us to review it sooner.

Changes to savedstate/savedstate/api/current.txt

androidx.savedstate
androidx.savedstate.SavedStateReader

http://go/agw/platform/frameworks/support/+/e04b610835745492aa66aa45be1426d57c1401b6%5E%21
https://android-review.googlesource.com/3523152

[Gerrit:https://android-review.googlesource.com/3523152]
[API-Approvers:ilake@google.com]
[Branch:androidx-main]

[LIBRARY_API_REVIEW_TAG:savedstate/savedstate/api/current.txt]

ap...@google.com <ap...@google.com> #16Mar 5, 2025 11:18PM

Android API Change

This bug was referenced by a recent CL that changed the Android API surface area.

The Android API Council regularly reviews API changes for consistency and sustainability, and we've just added this bug to our hotlist of pending reviews.

We'll wait until you mark this bug as 'Fixed' before starting our review, but please reach out if you'd like us to review it sooner.

Changes to savedstate/savedstate/api/current.txt

androidx.savedstate
androidx.savedstate.SavedStateReader

http://go/agw/platform/frameworks/support/+/cd71b1c721a8cf9733aed3bea5361f528011641c%5E%21
https://android-review.googlesource.com/3523151

[Gerrit:https://android-review.googlesource.com/3523151]
[API-Approvers:ilake@google.com]
[Branch:androidx-main]

[LIBRARY_API_REVIEW_TAG:savedstate/savedstate/api/current.txt]

mg...@google.com <mg...@google.com> Mar 6, 2025 02:56PM

Marked as fixed.

IssueTracker

Add `get*OrNull()` to `SavedStateReader`

Status Update

Description

Comments

ku...@google.com <ku...@google.com> #2Feb 28, 2025 12:02PM

ku...@google.com <ku...@google.com> #3Feb 28, 2025 12:17PM

ku...@google.com <ku...@google.com> #4Mar 3, 2025 10:19AM

ko...@jetbrains.com <ko...@jetbrains.com> #5Mar 3, 2025 12:54PM

mg...@google.com <mg...@google.com> #6Mar 3, 2025 10:13PM

ap...@google.com <ap...@google.com> #7Mar 5, 2025 11:06PM

ap...@google.com <ap...@google.com> #8Mar 5, 2025 11:06PM

ap...@google.com <ap...@google.com> #9Mar 5, 2025 11:06PM

ap...@google.com <ap...@google.com> #10Mar 5, 2025 11:06PM

ap...@google.com <ap...@google.com> #11Mar 5, 2025 11:06PM

ap...@google.com <ap...@google.com> #12Mar 5, 2025 11:06PM

ap...@google.com <ap...@google.com> #13Mar 5, 2025 11:18PM

Android API Change

ap...@google.com <ap...@google.com> #14Mar 5, 2025 11:18PM

Android API Change

ap...@google.com <ap...@google.com> #15Mar 5, 2025 11:18PM

Android API Change

ap...@google.com <ap...@google.com> #16Mar 5, 2025 11:18PM

Android API Change

mg...@google.com <mg...@google.com> Mar 6, 2025 02:56PM

Issue 399820614

Description

Issue summary

Comments

ku...@google.com <ku...@google.com> #2Feb 28, 2025 12:02PM

ku...@google.com <ku...@google.com> #3Feb 28, 2025 12:17PM

ku...@google.com <ku...@google.com> #4Mar 3, 2025 10:19AM

ko...@jetbrains.com <ko...@jetbrains.com> #5Mar 3, 2025 12:54PM

mg...@google.com <mg...@google.com> #6Mar 3, 2025 10:13PM

ap...@google.com <ap...@google.com> #7Mar 5, 2025 11:06PM

ap...@google.com <ap...@google.com> #8Mar 5, 2025 11:06PM

ap...@google.com <ap...@google.com> #9Mar 5, 2025 11:06PM

ap...@google.com <ap...@google.com> #10Mar 5, 2025 11:06PM

ap...@google.com <ap...@google.com> #11Mar 5, 2025 11:06PM

ap...@google.com <ap...@google.com> #12Mar 5, 2025 11:06PM

ap...@google.com <ap...@google.com> #13Mar 5, 2025 11:18PM

Android API Change

ap...@google.com <ap...@google.com> #14Mar 5, 2025 11:18PM

Android API Change

ap...@google.com <ap...@google.com> #15Mar 5, 2025 11:18PM

Android API Change

ap...@google.com <ap...@google.com> #16Mar 5, 2025 11:18PM

Android API Change

mg...@google.com <mg...@google.com> Mar 6, 2025 02:56PM

Add comment

Issue metadata