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
Move ChildActivity handling into base Activity class #16481
Conversation
387340f
to
0b46965
Compare
#16382 was merged. |
Rebased. |
Added a default This is also useful for activities that are just containers for a bunch of childactivities without any logic of their own. |
52d8569
to
3fdb5f6
Compare
Rebased and removed the |
Isn't this only true because we've made an effort to fix the bogus cases that were previously in the code? I'd be a lot happier if we could guarantee that we (or modders) can't accidentally break this again in the future, by keeping the state and hardcoding the |
Just for the record, I don't think this is a good idea. All current instances may have been fixed, but that doesn't mean that someone in the future (or an external modder) won't come along and introduce the mistake in another activity. Also it costs basically nothing at all. |
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.
Yeah, that seems like a valid concern. Reverted and changed to always strictly check instead.
Do we have any cases left where an activity returns something that isn't If not, would it make sense to change |
I've made |
Needs a rebase after #16365. |
ded971b
to
72ee105
Compare
|
||
// Avoid a single tick delay if the childactivity was just queued. | ||
if (ChildActivity != null && ChildActivity.State == ActivityState.Queued) |
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.
Style convention is to add braces around the body in this case - it may still be one logical statement, but there is potential for ambiguity about what the else
belongs to.
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.
Done.
619fcc5
to
e16170a
Compare
Split out the logic changes into separate commits. |
Rebased. |
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
Depends on #16369 and #16382.
With all activities now converted to the
ChildActivity
paradigm, some common patterns can be deduplicated by lettingActivity.TickOuter
handle childactivities.