Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the double showRendering call when initializing DecorativeViewFactory views. #276

Closed
wants to merge 1 commit into from

Conversation

zach-klippenstein
Copy link
Collaborator

Introduces ViewInitializer which gets executed by bindShowRendering between setting
the tag and actually invoking showRendering, which gives the DecorativeViewFactory
logic the chance to cancel the original showRendering call since it wraps it.

This initializer concept will also be used in a follow-up PR to allow ViewStateCache
to correctly initialize AndroidX view tree owners.

@zach-klippenstein zach-klippenstein requested a review from a team as a code owner January 20, 2021 09:31
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/fix-double-showrendering branch 3 times, most recently from 13b5e7d to fd8fd30 Compare January 20, 2021 09:34
Comment on lines 5 to 44
/**
* TODO kdoc
*/
@WorkflowUiExperimentalApi
public fun interface ViewInitializer {
/**
* TODO kdoc
*
* Implementations of this function _must_ call [showRendering].
*/
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/fix-double-showrendering branch 2 times, most recently from a754051 to c2a3628 Compare January 20, 2021 09:39
*
* Implementations of this function _must_ call [showRendering].
*/
public fun onViewCreated(view: View)
Copy link
Collaborator Author

@zach-klippenstein zach-klippenstein Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: If it's important that ViewStateCache only call restoreSavedInstanceState after the view's initial showRendering, then this will either have to be changed to allow initializers to run code both before and after the showRendering call, or we'll need two update functions that BackStackContainers will be required to call.

I'm leaning towards the latter – it keeps this API much simpler and more focused, and while it makes the ViewStateCacheAPI more complex, the extra verbosity might actually make the code more clear.

That said, adding a proceed function parameter here would mean bindShowRendering could drop it's magic "if the tag changed, don't call showRendering" logic, and instead leave it up to the ViewInitializer to skip calling proceed if it wants to. ViewStateCache also needs to track some local state before and after the initial showRendering, which would be easier to do if it could control that call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this different than the existing initView and doShowRendering arguments to DecorativeViewFactory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initView is called in the same point in the initialization process, so they are related.
But it's different than initView in that it's not tied to a particular view factory, but to a particular invocation of buildView.

doShowRendering is unrelated though, that gets called on every update.

*
* Implementations of this function _must_ call [showRendering].
*/
public fun onViewCreated(view: View)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this different than the existing initView and doShowRendering arguments to DecorativeViewFactory?

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/backstack-tests branch 2 times, most recently from 0eebbdc to 9c020a3 Compare January 20, 2021 22:45
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/fix-double-showrendering branch 2 times, most recently from ee289ed to 93a0802 Compare January 20, 2021 23:27
Base automatically changed from zachklipp/backstack-tests to main January 21, 2021 21:33
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/fix-double-showrendering branch 3 times, most recently from adc7223 to a6bd9b5 Compare January 21, 2021 22:04
@zach-klippenstein zach-klippenstein marked this pull request as draft January 21, 2021 22:04
*
* @param view The [View] that was just created by the [ViewFactory].
*/
public fun onViewCreated(view: View)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @laurakelly i removed that parameter like we discussed.

…tory views.

Introduces `ViewInitializer` which gets executed by `bindShowRendering` between setting
the tag and actually invoking `showRendering`, which gives the `DecorativeViewFactory`
logic the chance to cancel the original `showRendering` call since it wraps it.

This initializer concept will also be used in a follow-up PR to allow `ViewStateCache`
to correctly initialize AndroidX view tree owners.
@zach-klippenstein
Copy link
Collaborator Author

So I thought this would be required to implement AndroidX ViewTreeOwners support, but I sketched out an idea in #280 that doesn't actually require this at all. I haven't tested that yet though so it might need this after all. But if it doesn't, then I think this is probably too over-engineered just to solve this one case, and we should rethink how to fix the double-render.

@rjrjr
Copy link
Contributor

rjrjr commented Jun 8, 2021

This was made obsolete by #397, I believe.

@rjrjr rjrjr closed this Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants