If we do this we should make sure to re-open #14883 so we don't forget to re-evaluate the issue @Luminoth is facing :)

If we do this we should make sure to re-open #14883 so we don't forget to re-evaluate the issue @Luminoth is facing :)

The issue he is facing is something that has been solved in sickle_ui by inverting the dependency and anyone can take the abstraction from it. It is a limitation of ChildBuilder to begin with, since that doesn't let you take its commands for fancy chains. It is also not relevant since an Entity can be captured by a variable outside of closures and new commands can be issues on the next line.

There are situations that cannot be resolved per se, which is again the reason sickle_ui has the abstraction (and I think .reborrow already solves those cases without it). Also, I somehow feel that his example won't be resolved by making it consume self. Just makes commands entangled with the closure, making it even less workable in a larger code block.

NOTE: I am fine to leave it as-is, just worried about the astronomical impact for ??? gains. I mean if it would bring performance benefits or enable something mission critical we couldn't do before, yeah why not...

Edit: .insert_if is welcome to stay tho.

Yeah, I feel like we should revert this change before the release candidate and keep insert_if.

I'm willing to do the partial revert

Yes please :)

Also, I somehow feel that his example won't be resolved by making it consume self. Just makes commands entangled with the closure, making it even less workable in a larger code block.

@eidloi I'm not sure what you mean, the diff does resolve my example. Can you elaborate on why you think it doesn't?

An interesting thing about the count of changes in that particular diff is that most of the reassignments were only necessary because the context was if let Some(...) blocks that don't work with insert_if cleanly. I'm not sure how frequently that occurs in practice? It would be really interesting just on a personal level to see what @eidloi sickle_ui diff looked like and what sorts of changes the broader community sees from an RC. As an API ergonomics change, I definitely would expect changes to be needed but the context of those changes seems important. Like, if the changes are a tightening of code, I would call that a success, but if they end up being changes for the sake of changes then that would be a failure.

It's definitely also possible that the lack of these ergonomics originally have forced workarounds that are now tightly ingrained and hard to refactor away in larger codebases. The cost of that would be unfortunate for folks to have to deal with for sure.

Also, I somehow feel that his example won't be resolved by making it consume self. Just makes commands entangled with the closure, making it even less workable in a larger code block.

@eidloi I'm not sure what you mean, the diff does resolve my example. Can you elaborate on why you think it doesn't?

'twas just a feeling I got looking at the code in the issue. Not mentioning the soundness of the examples in there, I wasn't looking at it too long 'cus it irks me to attempt to use commands in a callback of commands. Just anti-rusty if you ask me.

But if you are keen on having this line of methods, maybe an optional extension could be created to provide self-consuming variants and then developers can switch to that if they feel the need. Maybe a ConsumingEntityCommands could be returned form commands.consuming_entity()?

An interesting thing about the count of changes in that particular diff is that most of the reassignments were only necessary because the context was if let Some(...) blocks that don't work with insert_if cleanly. I'm not sure how frequently that occurs in practice? It would be really interesting just on a personal level to see what @eidloi sickle_ui diff looked like and what sorts of changes the broader community sees from an RC. As an API ergonomics change, I definitely would expect changes to be needed but the context of those changes seems important. Like, if the changes are a tightening of code, I would call that a success, but if they end up being changes for the sake of changes then that would be a failure.

It's definitely also possible that the lack of these ergonomics originally have forced workarounds that are now tightly ingrained and hard to refactor away in larger codebases. The cost of that would be unfortunate for folks to have to deal with for sure.

Here you go: https://github.com/UmbraLuminosa/sickle_ui/compare/migration_attempt_to_0.15?expand=1

'twas just a feeling I got looking at the code in the issue. Not mentioning the soundness of the examples in there, I wasn't looking at it too long 'cus it irks me to attempt to use commands in a callback of commands. Just anti-rusty if you ask me.

But if you are keen on having this line of methods, maybe an optional extension could be created to provide self-consuming variants and then developers can switch to that if they feel the need. Maybe a ConsumingEntityCommands could be returned form commands.consuming_entity()?

It's certainly sound, but I get the personal preference on usage there. Trying to ease the pains of heavily nested UI is what got me here, I think trying to push that up out of the callbacks likely makes things a lot harder to manage and maintain.

Whether a separate consuming variant is the way, I'll leave that to the folks in charge. 😄

Here you go: https://github.com/UmbraLuminosa/sickle_ui/compare/migration_attempt_to_0.15?expand=1

Yeah, so aside from the changes that look like they're necessary from other unrelated updates, this looks generally like the tightening I would expect from the ergonomics change. I think the one case of having to re-assign I see in there could be resolved with add_pseudo_state_if? If it did, that entire block of changes becomes a single chained set.

I am wondering tho if this diff is the entire thing? I thought you'd mentioned something about adding reborrow calls, but I'm not seeing any in there (or is that hidden in the call to entity_commands() ?)

EDIT : Yeah, ok, I dug a bit deeper into the underlying code for those changes and I see kind of what's happening there. It's funny that we're both ending up here trying to make the UI experience easier.

EDIT : Yeah, ok, I dug a bit deeper into the underlying code for those changes and I see kind of what's happening there. It's funny that we're both ending up here trying to make the UI experience easier.

Yepp :D