Replace CodeListener with RedwoodView.Root #2374

squarejesse · 2024-10-08T22:59:52Z

This moves loading/error/ready tracking to live beside the content, rather than as something that side-effects the content.

This is not compatible with the old API and there is no shim.

Closes: #2174

CHANGELOG.md's "Unreleased" section has been updated, if applicable.

This moves loading/error/ready tracking to live beside the content, rather than as something that side-effects the content. This is not compatible with the old API and there is no shim.

...ost-composeui/src/commonMain/kotlin/app/cash/redwood/treehouse/composeui/TreehouseContent.kt

squarejesse · 2024-10-08T23:02:57Z

redwood-treehouse-host/src/commonTest/kotlin/app/cash/redwood/treehouse/FakeRoot.kt

+    // Canonicalize "java.lang.Exception(boom!)" to "kotlin.Exception(boom!)".
+    val exceptionString = uncaughtException?.toString()?.replace("java.lang.", "kotlin.")
+
+    // TODO(jwilson): this is a backwards-compatibility shim. Emit a simpler event.


For follow-up

squarejesse · 2024-10-08T23:03:38Z

redwood-widget/src/androidMain/kotlin/app/cash/redwood/widget/RedwoodLayout.kt

@@ -37,17 +37,16 @@ import kotlinx.coroutines.flow.StateFlow
 @SuppressLint("ViewConstructor")
 public open class RedwoodLayout(
  context: Context,
+  public final override val root: RedwoodView.Root<View> = ViewRoot(context),


On Android, this accepts the interface type, RedwoodView.Root ...

squarejesse · 2024-10-08T23:04:27Z

redwood-widget/src/iosMain/kotlin/app/cash/redwood/widget/RedwoodUIView.kt

@@ -38,17 +38,16 @@ import platform.UIKit.UIUserInterfaceStyle
 import platform.UIKit.UIView

 public open class RedwoodUIView(
-  public val view: UIView,
+  final override val root: UIViewRoot,


But on iOS, this accepts our concrete base implementation. This is because I haven’t yet figured out how to add a bounds change listener to a view I didn’t subclass

squarejesse · 2024-10-08T23:05:33Z

samples/counter/ios-uikit/CounterApp/CounterViewController.swift

@@ -6,19 +6,18 @@ class CounterViewController : UIViewController {
    private var delegate: CounterViewControllerDelegate!

    override func viewDidLoad() {
-        let container = UIStackView()


This is a behavior change. For a follow-up I need to use a Row in the Compose-side code for the counter.

squarejesse · 2024-10-08T23:06:43Z

...d-views/src/main/kotlin/com/example/redwood/emojisearch/android/views/EmojiSearchActivity.kt

+        }
+      }
+
+      for (child in viewGroupChildren) {


This still isn’t that great . . . I prefer the way this works in Compose

squarejesse · 2024-10-08T23:07:05Z

...h/android-views/src/main/kotlin/com/example/redwood/emojisearch/android/views/LoadingView.kt

+import androidx.appcompat.view.ContextThemeWrapper
+
+@SuppressLint("ViewConstructor")
+internal class LoadingView(


New: now there’s a spinner when launching emoji search

squarejesse · 2024-10-15T17:30:39Z

@JakeWharton this is ready for review.

I did an in-place swap from the old type to the new type rather than a graceful migration, which makes the PR larger than I generally like.

JakeWharton · 2024-10-15T23:32:26Z

Yeah it's... Big. I started today but had to leave early. Will finish tomorrow morning.

JakeWharton

I reviewed as much as I could. Don't want to hold it up any more.

JakeWharton · 2024-10-16T16:44:44Z

...d-views/src/main/kotlin/com/example/redwood/emojisearch/android/views/EmojiSearchActivity.kt

+
+      for (child in viewGroupChildren) {
+        if (child is ExceptionView || child is LoadingView) {
+          removeView(child)


It is not safe to remove a view like this while iterating. However, the iterator is actually a MutableIterator that you can call remove() on: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:core/core-ktx/src/main/java/androidx/core/view/ViewGroup.kt;l=89-99;drc=1e02caf7787e6fc64ec1913bf6e5cbbab1724a13. Unfortunately that means you can't use a nice for loop anymore.

Oh hell yes. I was exploiting the fact that this will only contain one child element if its either of those types. But I prefer your fix with an iterator.

JakeWharton · 2024-10-16T16:46:43Z

...d-views/src/main/kotlin/com/example/redwood/emojisearch/android/views/EmojiSearchActivity.kt

+      if (loadCount == 0) {
+        addView(LoadingView(context))
+      }
+      if (uncaughtException != null) {
+        addView(ExceptionView(context, uncaughtException))
      }


Seems a bit suspect that the loop will leave both LoadingView and ExceptionView instances, but then also possible add new ones as well.

Agreed. In practice it’s unlikely to transition from loading to loading again, or exception to exception again.

...d-views/src/main/kotlin/com/example/redwood/emojisearch/android/views/EmojiSearchActivity.kt

JakeWharton · 2024-10-16T17:22:27Z

redwood-widget/src/commonMain/kotlin/app/cash/redwood/widget/RedwoodView.kt

-   */
-  public fun reset()
+  @ObjCName("Root", exact = true)
+  public interface Root<W : Any> : Widget<W> {


This seems like it exists at the wrong layer of abstraction since Redwood has no concept of loading or errors–just content. Eliminating CodeListener is a big enough win for now. We can figure out how to move it later.

I don't see why this extends Widget. Do we use that capability anywhere? I haven't seen any usage that relies on it, but perhaps that's just because the diff is pretty big.

We’re using the value field to get a component to place in the parent container. But that doesn’t need to be polymorphic.

…/redwood/emojisearch/android/views/EmojiSearchActivity.kt Co-authored-by: Jake Wharton <[email protected]>

Replace CodeListener with RedwoodView.Root

2fbfdad

This moves loading/error/ready tracking to live beside the content, rather than as something that side-effects the content. This is not compatible with the old API and there is no shim.

squarejesse requested a review from JakeWharton October 8, 2024 23:00

squarejesse commented Oct 8, 2024

View reviewed changes

...ost-composeui/src/commonMain/kotlin/app/cash/redwood/treehouse/composeui/TreehouseContent.kt Outdated Show resolved Hide resolved

squarejesse commented Oct 8, 2024

View reviewed changes

squarejesse added 5 commits October 8, 2024 16:23

Self-review updates

8a35eae

Fix some build problems

daa92af

Track some API changes

acb8f5d

Fix CI problems

61b58b7

Call super.Render()

07a6426

JakeWharton approved these changes Oct 16, 2024

View reviewed changes

squarejesse and others added 2 commits October 16, 2024 13:45

Update samples/emoji-search/android-views/src/main/kotlin/com/example…

556e040

…/redwood/emojisearch/android/views/EmojiSearchActivity.kt Co-authored-by: Jake Wharton <[email protected]>

Code review feedback

bdb9855

squarejesse enabled auto-merge (squash) October 16, 2024 17:52

squarejesse merged commit d32766e into trunk Oct 16, 2024
11 checks passed

squarejesse deleted the jwilson.1004.redwoodview_root branch October 16, 2024 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace CodeListener with RedwoodView.Root #2374

Replace CodeListener with RedwoodView.Root #2374

squarejesse commented Oct 8, 2024 •

edited

Loading

squarejesse Oct 8, 2024

squarejesse Oct 8, 2024

squarejesse Oct 8, 2024

squarejesse Oct 8, 2024

squarejesse Oct 8, 2024

squarejesse Oct 8, 2024

squarejesse commented Oct 15, 2024

JakeWharton commented Oct 15, 2024

JakeWharton left a comment

JakeWharton Oct 16, 2024

squarejesse Oct 16, 2024

JakeWharton Oct 16, 2024

squarejesse Oct 16, 2024

JakeWharton Oct 16, 2024

JakeWharton Oct 16, 2024

squarejesse Oct 16, 2024

Replace CodeListener with RedwoodView.Root #2374

Replace CodeListener with RedwoodView.Root #2374

Conversation

squarejesse commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squarejesse commented Oct 15, 2024

JakeWharton commented Oct 15, 2024

JakeWharton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squarejesse commented Oct 8, 2024 •

edited

Loading