dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.1k stars 1.17k forks source link

[API Proposal]: Make ItemsControl::OnBringItemIntoView protected #10006

Open Laniusexcubitor opened 4 weeks ago

Laniusexcubitor commented 4 weeks ago

Background and motivation

When inheriting from ItemsControl, there is no direct way to use the existing API, but it can be useful to have the possibility to use it.

API Proposal

protected internal object OnBringItemIntoView(object arg)
{
    //
}

API Usage

I just want to be able use the same code as in ListBox

public void ScrollIntoView(object item)
{
    if (ItemContainerGenerator.Status == GeneratorStatus.ContainersGenerated)
    {
        OnBringItemIntoView(item);
    }
    else
    {
        // The items aren't generated, try at a later time
        Dispatcher.BeginInvoke(DispatcherPriority.Loaded, new DispatcherOperationCallback(OnBringItemIntoView), item);
    }
}

Alternative Designs

Perhaps a better alternative would be to bring the public method ListBox::ScrollIntoView up to ItemsControl. But make it virtual, as e.g. ComboBox needs some special love (see ComboBoxAutomationProvider::ScrollItemIntoView).

Risks

Could break Applications when developers have a derived ItemsControl where they already have a OnBringItemIntoView method with the same signature. Or in case of the alternative a ScrollIntoView method.

miloush commented 4 weeks ago

TreeView is a good special case to keep in mind. The selected item is read-only (which is also an issue for #8861), because you would have to do a recursive search to find the container. It is also not enough to just scroll to it, you have to expand all the parents to bring it into view, so the way this can currently be done for TreeView is you get ahold of the container first by any means necessary and then you select it/bring it into view.

In other words, the existing API you want to use would not work for all the controls.

The OnBringItemIntoView either calls BringIntoView on the container, which you can do yourself easily if you have it, or VirtualizingPanel.BringIndexIntoView. For the latter, you need access to the panel resp. ItemsHost and a way to figure out the index and this might be not so trivial. (I would support a public way to access ItemsHost.)

There is nothing wrong with offering a functionality that the derived controls do not have to use as long as they know they can't use it. You cannot bump ListBox.ScrollIntoView up the hierarchy because then you would have to implement the functionality for all the derived controls. It might not be breaking in terms of API surface, but suddenly TreeView and other 3rd party controls would have this method that possibly does not work for them.

So the option is to have this available as protected, but you run into the danger that implementations wouldn't know the limitations and just introduce a public wrapper to it, or call it when it doesn't work like above. That might be an acceptable risk which could be further mitigated with documentation, and possibly a method name. It is also not great that the argument is called arg rather than item when publicly you can only pass item to it, also fixable by different method name (different name would even lower the chance of a conflict).

I am not terribly opposed to the idea, but it's not a great situation either. If making ItemsHost accessible would at least enable people to overcome the gap, that would be my preferred choice, as it has other benefits too.

Laniusexcubitor commented 4 weeks ago

Yes I agree, pulling the public API up, was not a clever idea. Making the ItemsHostaccessible is a good idea, currently I inherit VirtualizingStackPanel just to allow to set an own ItemsHost property in my ItemsControls.