Remove unnecessary "data class" from public API [178659281]

Fixed

Bug

Status Update

No update yet.

Description

ae...@google.com

created issue #1

Jan 28, 2021 03:02AM

With the below command, I see Compose has 46 data classes in our public API. This commits us to supporting APIs like equals() and component1(). It means among other things that changing the order of non-constructor fields in the class would be a breaking API change. We should do a cleanup pass and remove "data" except in the few cases where destructuring style is part of our documented sample code.

frameworks/support/compose$ git grep 'method public.*component1();' -- */current.txt

Comments

si...@google.com <si...@google.com> #2Jan 28, 2021 03:05AM

Hi,

I also see my text cut off when I set maxLines = 2. Is it the same issue?

  Box(
    modifier =
      Modifier.size(
        width = 108dp,
        height = 34dp,
      ),
    contentAlignment = Alignment.Center,
  ) {
    BasicText(
      text = "text text text",
      maxLines = 2,
      autoSize = AutoSize.StepBased(minFontSize = 1.sp, maxFontSize = 13.sp, stepSize = 0.2.sp),
    )
  }

ae...@google.com <ae...@google.com> #3Jan 28, 2021 03:12AM

Project: platform/frameworks/support
Branch: androidx-main
Author: Jossi Wolf <jossiwolf@google.com>
Link: https://android-review.googlesource.com/3450389

Fix TextAutoSize bug with maxLines = 1

Expand for full commit details

Fix TextAutoSize bug with maxLines = 1 
 
We were overcaching the paragraphIntrinsics in MultiParagraphLayoutCache when mutating the style. For `AutoSizeStepBased` instances with biased windows (more values smaller/bigger than the optimal), this could result in performing layout with outdated intrinsics, and thus an outdated style and font size, without surfacing this in the TextLayoutResult. 
 
Test: New MultiParagraphLayoutCacheTests and manual testing 
Relnote: Fixed a bug in BasicText with TextAutoSize and maxLines set to 1. 
Fixes: 376834366 
 
Change-Id: Ic0450c763c5d764492995b44ee1fe570246a9689

Files:

M compose/foundation/foundation/src/androidInstrumentedTest/kotlin/androidx/compose/foundation/text/modifiers/MultiParagraphLayoutCacheTest.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/modifiers/MultiParagraphLayoutCache.kt

Hash: e1b712d78cc60384ed67a56c006148291ba146a6
Date: Tue Jan 07 18:52:26 2025

Message last modified on Jan 28, 2021 03:12AM

lp...@google.com <lp...@google.com> #4Jan 28, 2021 03:17PM

#2, yeah, that's the same issue.

ae...@google.com <ae...@google.com> #5Jan 28, 2021 08:40PM

Thanks @jo...@google.com for fixing this! Do you know when the fix would be available for g3 apps?

an...@google.com <an...@google.com> #6Jan 28, 2021 09:23PM

Moving the internal discussion offline. The bug is fixed and the fix available in snapshot builds. We will comment on this issue when the bug fix is included in a release.

ae...@google.com <ae...@google.com> #7Feb 2, 2021 12:34AM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.compose.foundation:foundation:1.8.0-beta01
androidx.compose.foundation:foundation-android:1.8.0-beta01
androidx.compose.foundation:foundation-jvmstubs:1.8.0-beta01
androidx.compose.foundation:foundation-linuxx64stubs:1.8.0-beta01

si...@google.com <si...@google.com> #8Feb 2, 2021 01:31AM

If i am not wrong you *have* to write hashCode as well. Equals without
hashcode sounds extra dangerous.

ToString is a good utility while debugging our code, and developers
debugging our code.since all 3 methods can be automatically created using
android studio, i dont see a reason/value to create equals but not hashcode
and tostring.

On Mon, Feb 1, 2021, 16:34 aelias <buganizer-system+aelias@google.com>
wrote:

- Show quoted text -

ae...@google.com <ae...@google.com> #9Feb 2, 2021 03:26AM

Makes sense. Also, equals/hashCode/toString are the three methods on Any, so overriding them doesn't affect our public API at all (they don't change any lines in current.txt).

ae...@google.com <ae...@google.com> #10Feb 2, 2021 06:04AM

OK, I did a survey of all the data classes and uploaded a patch to remove a bunch at aosp/1569820 . In the end, I decided the majority of the data classes looked pretty appropriate to use (things like IntRect) and I left them as is, so I changed my mind about the lint rule, it would be too cumbersome to ban it. The API review process can reasonably take care of it in the future.

ap...@google.com <ap...@google.com> #11Feb 2, 2021 11:20PM

Project: platform/frameworks/support
Branch: androidx-main

commit 9b3a65a1038c2da421e0dda0781d2189563e2c6f
Author: Alexandre Elias <aelias@google.com>
Date: Mon Feb 01 21:42:52 2021

Replace data classes with equals/hashCode/toString

This replaces "data" with the Android Studio autogenerated
implementations of equals/hashCode/toString in several classes, in order
to minimize public API surface and improve extensibility.

Note that the majority of the uses of "data" are in dead-simple classes
in graphics, geometry and layout like "IntRect", and I decided it was
appropriate to keep using "data" for those.

Bug: 178659281
Test: Existing tests
Relnote: "Destructuring and copy() methods have been removed from
several classes where they were rarely used."

Change-Id: I267021d3a45314acc9a169f6bbdfbfb4295a448c

M compose/animation/animation-core/api/current.txt
M compose/animation/animation-core/api/public_plus_experimental_current.txt
M compose/animation/animation-core/api/restricted_current.txt
M compose/animation/animation-core/src/commonMain/kotlin/androidx/compose/animation/core/DynamicTargetAnimation.kt
M compose/foundation/foundation/api/current.txt
M compose/foundation/foundation/api/public_plus_experimental_current.txt
M compose/foundation/foundation/api/restricted_current.txt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/BorderStroke.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/animation/FlingConfig.kt
M compose/material/material/api/current.txt
M compose/material/material/api/public_plus_experimental_current.txt
M compose/material/material/api/restricted_current.txt
M compose/material/material/src/commonMain/kotlin/androidx/compose/material/Swipeable.kt
M compose/ui/ui-graphics/api/current.txt
M compose/ui/ui-graphics/api/public_plus_experimental_current.txt
M compose/ui/ui-graphics/api/restricted_current.txt
M compose/ui/ui-graphics/src/commonMain/kotlin/androidx/compose/ui/graphics/Shadow.kt
M compose/ui/ui-graphics/src/commonMain/kotlin/androidx/compose/ui/graphics/drawscope/DrawScope.kt
M compose/ui/ui-graphics/src/commonMain/kotlin/androidx/compose/ui/graphics/painter/ColorPainter.kt
M compose/ui/ui-graphics/src/commonMain/kotlin/androidx/compose/ui/graphics/painter/ImagePainter.kt
M compose/ui/ui/api/current.txt
M compose/ui/ui/api/public_plus_experimental_current.txt
M compose/ui/ui/api/restricted_current.txt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/autofill/Autofill.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/semantics/SemanticsProperties.kt

https://android-review.googlesource.com/1569820

ae...@google.com <ae...@google.com> Feb 4, 2021 09:52PM

Marked as fixed.