Pipe Blueprint's environment through during layout and measurement by creating an environment of our own #225

kyleve · 2020-10-27T00:05:28Z

This PR updates List to ensure that we pass BlueprintUI.Environment through to the inner blueprint views within each item and header/footer within the list. This is done by writing BlueprintUI.Environment into our own ListEnvironment – which is now piped through to each item and header/footer within the list, and then, we reach this into environment and pull out the bp environment within BlueprintItemContent and BlueprintHeaderFooterContent to continue to pass the values down.

ListableUI/Sources/ListView/ListView.DataSource.swift

BlueprintUILists/Sources/BlueprintItemContent.swift

BlueprintUILists/Sources/List.swift

CHANGELOG.md

BlueprintUILists/Sources/BlueprintHeaderFooterContent.swift

kylebshr

High level thoughts/questions:

Do we need the env to also exist/get passed through in the ListableUI types? If you think it's useful I'm fine with it, but I'm not sure I see the utility of it?
If the answer to ^ is no, we don't need it, I don't think we should add a ListEnvironment type at all, and just pipe through the BP environment. We can add keys and use the BP environment to pass stuff down in BlueprintUILists

kyleve · 2020-10-27T17:34:40Z

I guess I'm not seeing how we'd get the Blueprint Environment down to each header and footer without some sort of environment-like thing in Listable – the data flow is:

You make a List
It goes into an outer BlueprintView
Blueprint updates the inner ListView
The inner ListView then renders cells by creating their inner BlueprintViews
Those inner views need the outer view's environment when they're getting rendered – their lifetime is not known by the outer List.

The key part being there's an inner extra layer of blueprint views whose lifetime is not known or controlled by users of the List itself.

kyleve · 2020-10-27T17:37:46Z

(I do wonder if there's some way that we can / need to solve this more generally in blueprint at some point – eg this will also break for any nested BP views in Market itself; which is not good for when we let people inject custom content, or we want to leverage the environment ourselves... But that's a whole thing)

zadr · 2020-10-27T18:30:38Z

BlueprintUILists/Tests/ListTests.swift

+                section += TestItemContent(wasCalled: callback)
+            }
+        }.adaptedEnvironment { env in
+            env[TestingKey.self] = true


worth an XCTAssertTrue(env[TestingKey.self]) somewhere at the end of the test (similar to TestHeaderContent and TestItemContent)?

Can you clarify what you mean here? I think this would only be testing Blueprint itself; since this is just a normal element.

I just missed that it was a plain mock element the first time around and asking from a place of "if we set a state in a test, should we assert that it was set anywhere?"

zadr · 2020-10-27T18:35:23Z

ListableUI/Sources/Internal/SupplementaryContainerView.swift

            } else {
                self.content = nil
            }
        }
    }

-    var reuseCache : ReusableViewCache?


when would the reuseCache be nil— if we init something instead of dequeuing it?

if so, something like precondition(reuseCache != nil, "must dequeue a Supplementary Container View") might make it clearer. definitely a nit, though

Because these are UICollectionReusableView; the init method is called by UICollectionView – you can't customize it. Thus, using IUOs to make things explode if these properties haven't been injected, without having to unwrap it at every callsite.

Added an inline comment to clarify this

zadr · 2020-10-27T18:36:47Z

ListableUI/Sources/ListEnvironment.swift

+///    }
+/// }
+/// ```
+public struct ListEnvironment {


maybe something like

public typealias ListEnvironment = BlueprintUI.Environment public typealias ListEnvironmentKey = BlueprintUI.EnvironmentKey

for now? so you own the type but don't have to rebuild the world until you have a use-case to do so

ListableUI itself doesnt depend on BlueprintUI, so, can't do that.

zadr · 2020-10-27T18:49:54Z

(I do wonder if there's some way that we can / need to solve this more generally in blueprint at some point – eg this will also break for any nested BP views in Market itself; which is not good for when we let people inject custom content, or we want to leverage the environment ourselves... But that's a whole thing)

My naive expectation was that EnvironmentReader would keep going up some internal tree until it found an environment to use

So if the tree was

0. UIView
1. - AdaptedEnvironment
2. -- BlueprintElement
3. --- UIView
4. ---- BlueprintElement
5. ----- EnvironmentReader
6. ------ BlueprintElementtent

so instead of the EnvironmentReader at level 5 fizzling out at level 3 because it found a UIView, it would keep traversing the view hierarchy to see if any parent was a view backed by a BlueprintElement and could provide an Environment to use

but on second thought, going up the tree is likely messy implementation (e.g.: would it require the description view to have a back reference to any ElementContent in case it holds an Environment?) and definitely not as nice as explicitly propagating state down the tree

kyleve · 2020-10-27T18:53:10Z

(I do wonder if there's some way that we can / need to solve this more generally in blueprint at some point – eg this will also break for any nested BP views in Market itself; which is not good for when we let people inject custom content, or we want to leverage the environment ourselves... But that's a whole thing)

My naive expectation was that EnvironmentReader would keep going up some internal tree until it found an environment to use

So if the tree was
0. UIView
1. - AdaptedEnvironment
2. -- BlueprintElement
3. --- UIView
4. ---- BlueprintElement
5. ----- EnvironmentReader
6. ------ BlueprintElementtent
so instead of the EnvironmentReader at level 5 fizzling out at level 3 because it found a UIView, it would keep traversing the view hierarchy to see if any parent was a view backed by a BlueprintElement and could provide an Environment to use

but on second thought, going up the tree is likely messy implementation (e.g.: would it require the description view to have a back reference to any ElementContent in case it holds an Environment?) and definitely not as nice as explicitly propagating state down the tree

Yeah, this – and there's different layout and measurement passes; so you can't just pass it down or "reach up" to find the current status easily.

zadr · 2020-10-27T18:57:14Z

Yeah, this – and there's different layout and measurement passes; so you can't just pass it down or "reach up" to find the current status easily.

Yeah, that makes a lot of sense, thanks.

I was thinking that the Environment is more independent of the layout/measurement passes so it might be okay. But now that you mention it, I can definitely see people wanting to store per-view information in an environment and propagate it down (e.g. the view size influencing size classes as you get further down the tree is super believable)

… creating an environment of our own

kyleve · 2020-10-29T06:37:13Z

@kylebshr any more thoughts here? I can set up some time to run thorough why this is needed (I think?) which might be easier than going back and forth here

kylebshr · 2020-10-29T17:18:58Z

I didn't realize at first that it had to pass through ListableUI, so needs it's own environment to not depend on BlueprintUI - makes sense now!