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

scheduling from Command.end? #6532

Open
truher opened this issue Apr 23, 2024 · 7 comments
Open

scheduling from Command.end? #6532

truher opened this issue Apr 23, 2024 · 7 comments

Comments

@truher
Copy link
Contributor

truher commented Apr 23, 2024

in Command.end(), theres a comment that advises against using this method to schedule another command, suggesting andThen(), which I think would only work for a static sequence. I had in mind to use the end() method to do ad hoc command sequencing, and it seems like it should work fine. is there a reason not to?

@spacey-sooty
Copy link
Contributor

What is scheduling from end going to gain you compared to using factory methods? You might be looking for select or either

@truher
Copy link
Contributor Author

truher commented Apr 23, 2024

maybe? how do I use select() or either() to assemble an indefinite series of arbitrary commands?

@spacey-sooty
Copy link
Contributor

Something like .run(select(...)) could work?

@truher
Copy link
Contributor Author

truher commented Apr 23, 2024

i see, you mean to put the selection logic in the command selector. how do you trap the command completion to select the next one? I think my main question is actually the reasoning behind the comment. why say that chaining commands through end() is a bad idea if it seems to work fine?

@spacey-sooty
Copy link
Contributor

I presume that it isn't recommended as it adds a side effect to the command that is essentially hidden. When you use a factory command you explicitly state that after this command ends this command should run. Compared to end where you are implicitly moving on to it which could cause unintuitive behaviour using something like factory methods as well.

@truher
Copy link
Contributor Author

truher commented Apr 23, 2024

ok thanks. maybe this is more of a question about canon rather than the specific Command.end idea. I'll ask over on CD.

@KangarooKoala
Copy link
Contributor

KangarooKoala commented Apr 23, 2024

Looks like that line was originally added in #2450 (to resolve #2441). However, the behavior that needed to be fixed (scheduling a command with shared requirements from end() would cause the original command to be canceled, calling end() again in an infinite loop) has been fixed in #5470, so we probably don't need that comment anymore.

Regarding https://www.chiefdelphi.com/t/canonical-command-chaining/464156 (which I'm guessing is where you asked on CD):

  • Overriding Command.end() (or equivalently, wrapping in onEnd()) to get a callback when the command finishes should work, though if you run into bugs a) please file a bug report to help us improve! and b) try polling Command.isScheduled() periodically to detect when it is no longer scheduled.
  • Depending on your framework, you also might be able to use cmd.andThen(Commands.defer(() -> getNextCommand(), Set.of(<requirements>))) instead of wrapping in cmd.onEnd(() -> scheduleNextCommand()) or overriding cmd.end() to schedule the next command, but I suspect that would run into memory issues since it would create subcommands but not be able to garbage collect old commands (since those are wrappers around the subcommands). bovlb's CD suggestion of andThen(Commands.runOnce(() -> scheduleNextCommand())) wouldn't run into those memory issues, though since onEnd() should work, there wouldn't be an advantage compared to onEnd(() -> scheduleNextCommand()).
  • I would advise against using onCommandFinish() and/or onCommandInterrupt(), since even if you get both the finish and interrupt combination, you still will not be notified of a command ending within a composition (since the callbacks are managed by CommandScheduler, while command compositions manage the composed commands themselves).

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

No branches or pull requests

3 participants