Status Update
Comments
lp...@google.com <lp...@google.com> #2
Branch: androidx-master-dev
commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019
Change the way to detect mismatch between POJO and query
This fixes cursor mismatch warnings with expandProjection.
Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt
di...@google.com <di...@google.com> #3
lp...@google.com <lp...@google.com> #4
Branch: androidx-master-dev
commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020
Re-factor how errors are dismissed when query is re-written
This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically (
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.
This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.
Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f
M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
lp...@google.com <lp...@google.com> #5
This works above R, but below R it crashes with:
java.lang.IllegalArgumentException: Context android.app.Application@20b1a86 is not a UiContext
di...@google.com <di...@google.com> #6
An application Context is not a UI Context so it isn't guaranteed to work. We could add stricter enforcement but that would make it so that the method throws when using an application context.
di...@google.com <di...@google.com> #7
An application Context is not a UI Context because it is not associated with a Display. Therefore creating UI with it is not recommended.
lp...@google.com <lp...@google.com> #8
Sorry if I wasn't clear, but we don't control the callers here, it is common (and supported) for developers to create views with an application context. The question is what should the window size we provide in Compose be in this case. And whatever logic we decide to do there (e.g root view size, default display size) probably makes more sense to implement inside window metrics calculator instead of crashing, so we can be consistent in the fallback behavior across applications and libraries.
Otherwise we would have to basically rely on the implementation detail that the WM library tries to unwrap the context, so we would do it first to see if we can do it safely, and only if so call the WM API
Curious if you have any thoughts as to this, as there is still a 'window' somewhere in the app that is meaningful for UI decisions based on window size
di...@google.com <di...@google.com> #9
There are no correct window metrics that you can get from the Application Context. There is some parts leftover for compatibility reasons but for new code paths one should not use the Application Context to create Views. For example, there is no default display size because Context#getDisplay
will crash. You will not get a correct result going forward when using Context.getSystemService(WindowManager::class.java).getDefaultDisplay()
.
as there is still a 'window' somewhere in the app that is meaningful for UI decisions based on window size
Where is the window
?
Also, in your example above why not use the Activity
as the Context
?
lp...@google.com <lp...@google.com> #10
There are no correct window metrics that you can get from the Application Context
Above R it seems to work fine, since it uses WindowManager#getCurrentWindowMetrics, but I guess there is nothing else we can use below R
You will not get a correct result going forward when using Context.getSystemService(WindowManager::class.java).getDefaultDisplay().
Right, but it's a better approximation than crashing, we need to provide our consumers with some value here. We would only need to do this below R, if the context is not a UI context anyway.
Where is the window?
The same entity that WindowManager#getCurrentWindowMetrics returns above R for an applicationContext, not sure how this is calculated but it seems to map to the current activity inside the application.
Also, in your example above why not use the Activity as the Context?
My example is a bit contrived, in reality this is a common pattern internally. We tried to land a change that calculates default window metrics from the context, and this crashed in multiple tests because the applicationContext was being used, and it's not feasible to update all these existing applicationContext usages that work fine outside of calculating window size.
In any case if there is no desire to avoid this crash at the window library layer, we'll have to land some workaround instead for Compose
di...@google.com <di...@google.com> #11
Above R it seems to work fine, since it uses WindowManager#getCurrentWindowMetrics, but I guess there is nothing else we can use below R
Not crashing isn't the same as working fine. WindowManager#getCurrentWindowMetrics
would be incorrect in a multi display environment.
Right, but it's a better approximation than crashing, we need to provide our consumers with some value here. We would only need to do this below R, if the context is not a UI context anyway.
I don't agree since he default display is the phone's display which can be different from a 42" 4k monitor. And like I said, a non UI context should not be used.
The same entity that WindowManager#getCurrentWindowMetrics returns above R for an applicationContext, not sure how this is calculated but it seems to map to the current activity inside the application.
The problem is when you are in a multi display environment and that is when it would break.
My example is a bit contrived, in reality this is a common pattern internally. We tried to land a change that calculates default window metrics from the context, and this crashed in multiple tests because the applicationContext was being used, and it's not feasible to update all these existing applicationContext usages that work fine outside of calculating window size.
Why are you all using the Application Context instead of a proper UI Context?
lp...@google.com <lp...@google.com> #12
Not crashing isn't the same as working fine. WindowManager#getCurrentWindowMetrics would be incorrect in a multi display environment.
Isn't this what is normally used by WindowMetricsCalculator above R?
di...@google.com <di...@google.com> #13
On a UI Context it is correct, on a non-UI Context it is incorrect.
lp...@google.com <lp...@google.com> #14
Discussed more offline, filed
lp...@google.com <lp...@google.com> #15
Note we'll also need to address the performance regression with the original change:
lp...@google.com <lp...@google.com> #16
Filed a bug for the performance issue, which was rootcaused to insets calculation in WindowMetricsCalculator - since we don't need insets in what we are trying to provide, for now we can just fork the bounds logic, and unify with window library when these blocking issues are resolved
ap...@google.com <ap...@google.com> #17
Project: platform/frameworks/support
Branch: androidx-main
Author: Louis Pullen-Freilich <
Link:
Re-land adding WindowInfo#containerSize
Expand for full commit details
Re-land adding WindowInfo#containerSize
Temporarily using a forked subset of WindowMetricsCalculator to avoid performance issues with inset calculations (that we don't need).
Bug: b/369334429
Fixes: b/360343819
Test: ComposeViewTest
Test: WindowInfoCompositionLocalTest
Relnote: "Adds WindowInfo#containerSize to provide the current window's content container size. This can be retrieved using LocalWindowInfo."
Change-Id: I277673b5576f15f60bc82eeb9d9424c5144a3c25
Files:
- M
compose/ui/ui/api/current.txt
- M
compose/ui/ui/api/restricted_current.txt
- M
compose/ui/ui/build.gradle
- M
compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/platform/ComposeViewTest.kt
- M
compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/platform/WindowInfoCompositionLocalTest.kt
- M
compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/platform/AndroidComposeView.android.kt
- A
compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/platform/AndroidWindowInfo.android.kt
- M
compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/platform/WindowInfo.kt
Hash: 7ca3950e4bd0a608b1570f07b75c824912860ed6
Date: Tue Sep 24 14:14:22 2024
ap...@google.com <ap...@google.com> #18
Project: platform/frameworks/support
Branch: androidx-main
Author: Louis Pullen-Freilich <
Link:
Re-re-land adding WindowInfo#containerSize
Expand for full commit details
Re-re-land adding WindowInfo#containerSize
Temporarily using a forked subset of WindowMetricsCalculator to avoid some extra performance issues with inset calculations (that we don't need).
Bug: b/369334429
Fixes: b/360343819
Test: ComposeViewTest
Test: WindowInfoCompositionLocalTest
Relnote: "Adds WindowInfo#containerSize to provide the current window's content container size. This can be retrieved using LocalWindowInfo."
Change-Id: Idc38c705655c9181022161927275318fba86bed8
Files:
- M
compose/ui/ui/api/current.txt
- M
compose/ui/ui/api/restricted_current.txt
- M
compose/ui/ui/build.gradle
- M
compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/platform/ComposeViewTest.kt
- M
compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/platform/WindowInfoCompositionLocalTest.kt
- M
compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/platform/AndroidComposeView.android.kt
- A
compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/platform/AndroidWindowInfo.android.kt
- M
compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/platform/WindowInfo.kt
Hash: 2b13d0743f3dc7f38550d0bcfe30dbc5cfd2ecfe
Date: Tue Sep 24 14:14:22 2024
na...@google.com <na...@google.com> #19
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.compose.ui:ui:1.8.0-alpha04
androidx.compose.ui:ui-android:1.8.0-alpha04
androidx.compose.ui:ui-jvmstubs:1.8.0-alpha04
androidx.compose.ui:ui-linuxx64stubs:1.8.0-alpha04
Description
To re-land this we need to figure out what the behavior should be for containerSize in this case.