Updated Scroll_to_View

[xarvic]

This PR adds that ClipBox and Tabs can handle scroll_to_view.

• ClipBox discards scroll_to_view_requests for hidden areas unless managed() was called.
• Tabs discards all scroll_to_view requests which originate from hidden widgets.
• Notification now has a known_target flag. When set to false there will be no warning even if the notification reaches the root widget.

[jneem]

Could you explain the motivation for having a ClipBox that isn't managed? My initial reaction is that if you're clipping your children and ignoring SCROLL_TO_VIEW then that's a bug, and we don't need to be working around it in ClipBox.

[xarvic]

Could you explain the motivation for having a ClipBox that isn't managed? My initial reaction is that if you're clipping your children and ignoring SCROLL_TO_VIEW then that's a bug, and we don't need to be working around it in ClipBox.

I can't think of a specific usecase. But you can use ClipBox without creating a custom widget and in this case it could result in pretty strange behavior.
Maybe we should print a warning when a SCROLL_TO_VIEW notification was stopped?

[jneem]

Sorry for the slow reply. I've been thinking a bit more about this, and I think what worries me is that both the mananged and unmanaged scenarios have the potential for silently causing unexpected behavior. I agree that if you use ClipBox without handling SCROLL_TO_VIEW (for example, because you just use it directly in your widget tree without a custom widget or a controller) then you might see surprising behavior without warning. But on the other hand if you are handling SCROLL_TO_VIEW but you forget to make the ClipBox managed then you'll also see surprising behavior without warning.

I don't see a great way out of this situation, but I'm still of the opinion that if you have a ClipBox you should handle SCROLL_TO_VIEW. So maybe it makes sense to have managed be the default, and the alternative could be called by a very explicit name like clip_scroll_notifications?

[xarvic]

I don't see a great way out of this situation, but I'm still of the opinion that if you have a ClipBox you should handle SCROLL_TO_VIEW. So maybe it makes sense to have managed be the default, and the alternative could be called by a very explicit name like clip_scroll_notifications?

Sounds good.

I think what worries me is that both the mananged and unmanaged scenarios have the potential for silently causing unexpected behavior.

In this case maybe we should not have a default value, but force the user to make an explicit decision which option would fit? Either by introducing ClipBox::managed and ClipBox::unmanaged instead of ClipBox::new or adding a flag to the current constructor?
Although this would be a breaking change.

[xarvic]

@jneem what do you think about this. I am in favor of the breaking change. But your suggestion would also be fine for me.

[jneem]

Oh, sorry, for some reason in my mind I had already replied. I don't have a strong preference between having a default or not having a default. I agree that defaulting to managed has the advantage of not being breaking, but I don't think that's a dealbreaker. I do prefer having named methods over having flags (because the resulting code is easier to read).

[xarvic]

Have I understood this correctly, you are in favor of the ClipBox::managed and ClipBox::unmanaged approach?

[jneem]

I'm ok with two approaches

• ClipBox::new defaulting to managed, together with a ClipBox::unmanaged(self) -> Self builder method. (Or maybe a more explicit name like ClipBox::clip_notifications?); or
• ClipBox::managed and ClipBox::unmanaged.

I don't have a strong preference between these two.

[jneem]

Thanks for the changes! Just a couple documentation clarifications, and I think you also need to fix the doc link for CI to be satisfied.

[jneem]

There's no more "by default", right? The choice is now explicit.

[xarvic]

Yes

[xarvic]

Thanks for your time :)

[Fenex]

incorrect function name in note, fix #2210