-
Notifications
You must be signed in to change notification settings - Fork 101
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
Brings ViewStarter to the UI update. #620
Conversation
Fixes a problem where `launchWhenAttached` would crash if it failed to fetch a resource name when trying to make a cute scope name. I happen to know of some view systems that break our assumption that every id is named.
View.launchWhenAttached tolerates strange ids
…moving the cursor to the ends of the string being edited.
Fixes index out of bounds crash in hello-todo-terminal sample
Fixes #597. It used to be the case that every `DecorativeViewFactory` called `showRendering()` twice (#397). We fixed that (we thought) by introducing the `initializeView` lambda to `ViewRegistry.buildView` and `DecorativeViewFactory` (#408). Unfortunately, that fix botched recursion. Individual `DecorativeViewFactory` instances work fine, but if you wrap them you still get one `showRendering` call from each. Worse, upstream `initializeView` lambdas are clobbered by immediately downstream ones. e.g., when a `WorkflowViewStub` shows a `DecorativeViewFactory`, the `WorkflowLifecycleRunner.installOn` call in the former is clobbered. The fix is to completely decouple building a view from from this kind of initialization. `ViewRegistry.buildView` and its wrappers no longer try to call `showRendering` at all. Instead the caller of `buildView` (mostly `WorkflowViewStub`) is reponsible for immediately calling `View.start` on the new `View`. `View.start` makes the initial `showRendering` call that formerly was the job of `ViewFactory.buildView` -- the factory builds the view, and the container turns the key. Since `View.start` is called only after all wrapped `ViewFactory.buildView` functions have executed, we're certain it will only happen once. Of course we still need the ability to customize view initialization via wrapping, especially to invoke `WorkflowLifecycleOwner.installOn`. To accomodate that, the function that `View.start` executes can be wrapped via the new `viewStarter` argument to `ViewRegistry.buildView` and `DecorativeViewFactory`, which replaces `initializeView`. This required a pretty thorough overhaul of `ViewShowRendering.kt` The `ViewShowRenderingTag` that it hangs off of a view tag is renamed `WorkflowViewState`, and extracted to a separate file. `WorkflowViewState` is a sealed class with two implementations (`New` and `Started`) to help us enforce the order of the `ViewRegistry.buildView`, `View.bindShowRendering`, `View.start` and `View.showRendering` calls.
Replaces *.initializeView with *.viewStarter
Because the [releases page](https://github.com/square/workflow-kotlin/releases) is so much nicer.
End CHANGELOG.md
When moving Workers to being wrapped by WorkerWorkflow we did not need to compare types included in the KType signature but that was not clear from the kdoc. Update that to make it clear.
…-output 608: Update TypedWorker kdoc
Gradle 7.3.2 adds dependency constraints to the _build_ classpath to reject known-bad versions of log4j. See also https://blog.gradle.org/log4j-vulnerability.
Use Gradle 7.3.2. Log4shell mitigation.
Upgrade the compile SDK to 31.
Moves us to s01.oss.sonatype.org
* origin/main: Upgrade the compile SDK to 31. Moves us to s01.oss.sonatype.org Fixes version name after 1.3.0 release, forgot to bump it. Use Gradle 7.3.2. Log4shell mitigation. 608: Update TypedWorker kdoc Releasing v1.3.0 End CHANGELOG.md Replaces *.initializeView with *.viewStarter Adds (failing) DecorativeViewFactory double update test Fixes index out of bounds crash when attempting to add characters or moving the cursor to the ends of the string being edited. View.launchWhenAttached tolerates strange ids
847de16
to
8c31040
Compare
8c31040
to
f0dce22
Compare
Having trouble seeing why the espresso tests are timing out. Not consistently happening at the same point, and they pass locally in a reasonable amount of time. Maybe infra? Rolling the dice again this morning. |
Reviewed last commit and looks good. Are you still waiting for green before taking this out of draft? |
Yes. Seems a lot more likely that I've broken something than that CI is actually being bad. |
Cool cool cool, turned on debugging and now it's green. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is entirely a merge PR, except for the last commit. That one replicates the work from #602 , and should be reviewed please.