Add inspection support for ViewThatFits. #255

rafael-assis · 2023-08-22T18:42:54Z

Changes

Similar to the issue fixed by #228, this change aims to fix blocking inspection for Views that are nested inside a ViewThatFits View.

ViewThatFits has a structure similar to a TreeView and needs to be inspected for its content property for ViewInspector to find its children.

Discussion

This is a first step that unblocks the inspection of children Views of ViewThatFits.

However, the documented behavior of this View states that only one of its children will be provided (rendered) based on horizontal space. Implementing inspection in a way that all children Views are returned may be problematic as all children are likely going to be similar in content, labels and accessibility traits.

This can potentially cause multiple Views returned in an inspection done with a findAll(where:) call.

I will investigate the possibility of hosting the ViewThatFits View inside a ViewHosting View and validate whether only one of the children view is available. That will be the scenario that is the closest to SwiftUI running on a real simulator/device.

Another suggestion we're considering is the addition of a capability that enables the test code to pick an index of the child view wanted. Alternatively we can just always default to the first child in the ViewThatFits.

@nalexn we'd love to have your thoughts and recommendations on this.

Testing

I added a test to explicitly find 2 children Views inside a ViewThatFits, which enforces the behavior being proposed in this PR.

Review

@bachand
@nalexn

rafael-assis · 2023-08-22T18:44:42Z

Sources/ViewInspector/SwiftUI/ViewThatFits.swift

+    static func child(_ content: Content) throws -> Content {
+        let view: Any = try {
+            guard let rootContent = try? Inspector.attribute(path: "_tree|content", value: content.view) else {
+                // A ViewThatFits View renders only one of its child Views based on the available horizontal space.


This comment was added to explicitly document that we currently return all children of a ViewThatFits View.

cc: @bachand

Inspector.attribute(path: "_tree|content", value: content.view) is what will return all children right? If so I wonder if this comment should go above that line instead of in the else of the guard statement.

rafael-assis · 2023-08-22T18:45:32Z

Tests/ViewInspectorTests/SwiftUI/ViewThatFitsTests.swift

+        let shortText = try sut.inspect().find(text: "Short text")
+
+        XCTAssertEqual(try longText.string(), "Very long text that will only get picked if there is available space.")
+        XCTAssertEqual(try shortText.string(), "Short text")


The test intentionally validates that both children Views are inspectable even though in a real runtime scenario only one of them would be provided.

bachand

Nice! Thank you for unblocking inspection of ViewThatFits, which is used in some critical views within our codebase. Will be curious to see what @nalexn thinks about this change and about whether it makes sense to provide further customization options during inspection (e.g. an option to return the first child of ViewThatFits, instead of all children).

bachand · 2023-08-23T17:29:44Z

Sources/ViewInspector/SwiftUI/ViewThatFits.swift

+import SwiftUI
+
+@available(iOS 16.0, macOS 13.0, tvOS 16.0, *)
+internal extension ViewType {


Should this be public? For comparison:

ViewInspector/Sources/ViewInspector/SwiftUI/VStack.swift

Lines 4 to 9 in 0f872b5

public extension ViewType {

struct VStack: KnownViewType {

public static let typePrefix: String = "VStack"

}

}

bachand · 2023-08-23T17:39:23Z

Sources/ViewInspector/SwiftUI/ViewThatFits.swift

+    static func child(_ content: Content) throws -> Content {
+        let view: Any = try {
+            guard let rootContent = try? Inspector.attribute(path: "_tree|content", value: content.view) else {
+                // A ViewThatFits View renders only one of its child Views based on the available horizontal space.


Suggested change

// A ViewThatFits View renders only one of its child Views based on the available horizontal space.

// A ViewThatFits View renders only one of its child Views based on the available space.

The view accepts an axis on initialization.

bachand · 2023-08-23T17:41:23Z

Sources/ViewInspector/SwiftUI/ViewThatFits.swift

+    static func child(_ content: Content) throws -> Content {
+        let view: Any = try {
+            guard let rootContent = try? Inspector.attribute(path: "_tree|content", value: content.view) else {
+                // A ViewThatFits View renders only one of its child Views based on the available horizontal space.


Inspector.attribute(path: "_tree|content", value: content.view) is what will return all children right? If so I wonder if this comment should go above that line instead of in the else of the guard statement.

bachand · 2023-08-23T17:41:44Z

Sources/ViewInspector/SwiftUI/ViewThatFits.swift

+                // A ViewThatFits View renders only one of its child Views based on the available horizontal space.
+                // This inspection returns all children that can potentially be picked by `ViewThatFits`.
+                // https://developer.apple.com/documentation/swiftui/viewthatfits
+                return try Inspector.attribute(path: "content", value: content.view)


Can you help me understand this fallback? When would we expect this to be encountered?

I see we had similar code in #228 but I want to make sure it's necessary here.

bachand · 2023-08-23T17:49:07Z

Sources/ViewInspector/SwiftUI/ViewThatFits.swift

+
+            return rootContent
+        }()
+        return try Inspector.unwrap(view: view, medium: content.medium)


@nalexn can you help me better understand what it's appropriate to invoke this unwrap(…) method? I am trying to understand if we should always be unwrapping when extracting content from views.

unwrap should be called when you get to a user-provided view that might be "wrapped" in modifiers. For example, AnyView(x). This x can be a standard view, a custom user-defined view, or either of these but "wrapped" in a modifier, such as x.padding(10). "unwrap" makes sure the root view of "x" is revealed for ease of further inspection (but modifiers are remembered as well, just stored separately).
On the other hand, when you inspect an internal child of a standard SwiftUI view, there is no need to "unwrap" them, if you can see from the code there is no generic parameter that can alter the view and turn it from Text into a ModifiedContent<Text>

Thank you for the explanation!

bachand · 2023-08-23T17:50:14Z

Tests/ViewInspectorTests/SwiftUI/ViewThatFitsTests.swift

@@ -0,0 +1,26 @@
+import XCTest
+import SwiftUI


Suggested change

import SwiftUI

import SwiftUI

@testable import ViewInspector

nalexn · 2023-08-24T06:19:33Z

Thanks! I'll review the PR as soon as possible. As a side note, I feel like I've already given you this tip, but instead of po view you can use po Inspector.print(view) as AnyObject. It's identical to po view for primitive views, but gives much better results on complex hierarchies.

rafael-assis · 2023-08-25T17:55:01Z

Thanks! I'll review the PR as soon as possible. As a side note, I feel like I've already given you this tip, but instead of po view you can use po Inspector.print(view) as AnyObject. It's identical to po view for primitive views, but gives much better results on complex hierarchies.

Thank you @nalexn for making time to review it. We appreciate your efforts here!

Add inspection support for ViewThatFits.

0f872b5

rafael-assis commented Aug 22, 2023

View reviewed changes

bachand approved these changes Aug 23, 2023

View reviewed changes

nalexn pushed a commit that referenced this pull request Aug 26, 2023

#255: Slight refactoring

0def023

nalexn pushed a commit that referenced this pull request Aug 26, 2023

#255: Reuse HStack code for ViewThatFits

161c14c

nalexn pushed a commit that referenced this pull request Aug 26, 2023

#255: Include ViewThatFits in readiness

e1935eb

nalexn merged commit 0f872b5 into nalexn:0.9.8 Aug 26, 2023

nalexn added pending release A fixed issue that'll be released in an upcoming update and removed pending release A fixed issue that'll be released in an upcoming update labels Aug 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add inspection support for ViewThatFits. #255

Add inspection support for ViewThatFits. #255

rafael-assis commented Aug 22, 2023

rafael-assis Aug 22, 2023

bachand Aug 23, 2023

rafael-assis Aug 22, 2023

bachand left a comment

bachand Aug 23, 2023

bachand Aug 23, 2023

bachand Aug 23, 2023

bachand Aug 23, 2023

bachand Aug 23, 2023

bachand Aug 23, 2023

bachand Aug 23, 2023

nalexn Aug 24, 2023 •

edited

Loading

bachand Aug 25, 2023

bachand Aug 23, 2023

nalexn commented Aug 24, 2023

rafael-assis commented Aug 25, 2023

	public extension ViewType {

	struct VStack: KnownViewType {
	public static let typePrefix: String = "VStack"
	}
	}

	// A ViewThatFits View renders only one of its child Views based on the available horizontal space.
	// A ViewThatFits View renders only one of its child Views based on the available space.

Add inspection support for ViewThatFits. #255

Add inspection support for ViewThatFits. #255

Conversation

rafael-assis commented Aug 22, 2023

Changes

Discussion

Testing

Review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachand 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

nalexn Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalexn commented Aug 24, 2023

rafael-assis commented Aug 25, 2023

nalexn Aug 24, 2023 •

edited

Loading