add OnScroll event to list

[csturiale]

Description:

This add the possibility to add a function to the OnScroll event to a widget.List

Checklist:

• [X ] Tests included.
• [ X] Lint and formatter run with no errors.
• [ X] Tests all pass.

Where applicable:

• [ X] Public APIs match existing style and have Since: line.
• [ N/A] Any breaking changes have a deprecation path or have been discussed.
• [ N/A] Check for binary size increases when importing new modules.

[dweymouth]

What is the use case for this? I see you are only invoking the callback in the scrollTo function, which AFAIK is only invoked when the public ScrollTo API is invoked, so you could just hook into whenever you invoke ScrollTo to achieve your use case instead?

[csturiale]

The use case is :
be able to Select the item when currentFocus changes ( ie. When I'm moving between the items with Arrows Keys )

There is no need to have a direct call to ScrollTo func
ie list.OnScroll = func(id widget.ListItemID) { list.Select(id) }

[dweymouth]

Would it not make more sense for the API to be OnFocused(id) then? OnScroll would imply that it would get invoked on any scrolling (like using the mouse wheel)

[csturiale]

PR updated

[dweymouth]

The naming here should be "OnFocused" to match the convention. Also we will need a comment with a "Since: 2.6" line (see the HideSeparators property for example)

[dweymouth]

You will want to call this callback whenever l.currentFocus or l.focused changes, not here in scrollTo, since scrollTo is invoked sometimes for things that don't change the focus (like list.ScrollToItem). You will probably also want to add an OnFocusLost API, or else call OnFocused with -1 as the argument whenever the list loses focus.

[dweymouth]

Also, please add some unit tests to verify the behavior, see the existing examples in list_test.go

[csturiale]

Done!
Thanks a lot for your hints, let me know if it can be enough

[Jacalz]

Isn't OnFocused just exactly the same as the OnSelected callback that we already have? Why do we need this as well?

[dweymouth]

Isn't OnFocused just exactly the same as the OnSelected callback that we already have? Why do we need this as well?

It's not the same, because items can be focused without being selected, as happens in the case of keyboard scrolling the list. @csturiale's use case AFAIK is to bind OnFocused to list.Select so that items will be focused and selected as the list is keyboard-scrolled, though I can see other uses for it as well. But I am still a bit on the fence, as personally, I've found it's better to handle focus on your row widgets themselves if you need to do something custom with focus, and use a list extended to be Disabled, so the list itself doesn't take focus. Of course this is more code though, especially if you're doing something simple.

[andydotxyz]

It's important to realise the difference between selection and focus. The list itself is a single Focusable item so that keys like up/down function correctly.

I'm not really understanding what this addition is for, but shifting focus should only happen when the user moves focus with tab/shift-tab or by tapping.

[dweymouth]

It's important to realise the difference between selection and focus. The list itself is a single Focusable item so that keys like up/down function correctly.

I'm not really understanding what this addition is for, but shifting focus should only happen when the user moves focus with tab/shift-tab or by tapping.

@csturiale wanted to know when an item is focused so that it can be automatically selected. Though I can see other uses, like perhaps updating a status bar below the list with details about the currently focused row, or something like that. Though to make the list Focus API "complete" I think an API for the developer to assign custom keybindings to the focused row would also be needed. (That is what I would need at minimum to be able to switch to using the builtin focus instead of making my rows individually focusable. Though there are also other niceties like if list row N is focused, and you press tab, the focus moving to the focusable widgets within the row itself if any which is what would be expected by the user)

[andydotxyz]

@csturiale wanted to know when an item is focused so that it can be automatically selected

But this is confusing to me - because the /List/ is focused but a sub-item is selected...
When moving between items in a list the focus won't change - unless the developer has put items with focus inside each item of the list.

[dweymouth]

When you keyboard-scroll the list, the rows update with a focus indicator rectangle, simulating that the rows themselves have focus. And to the user it appears this way (that the row, and not the list, has focus). Developers may want to react to the "focus" moving within the list. Or in my case have custom OnTypedKey handling which I solved by actually making my rows focusable and handling the up-down focus moving myself.

[andydotxyz]

So this could be completed with a FocusFollowsSelection bool instead of a new (confusingly named) focus API, where it is not actually changing focus in the same sense that other focus named APIs.

When you keyboard-scroll the list, the rows update with a focus indicator rectangle

Just for clarities sake what updates on up/down is the hover rectangle (dark colour) not the focus rectangle (faded primary colour). So maybe the suggestion above would actually be FocusFollowsHighlightedItem.

[dweymouth]

I think @csturiale 's specific use case could be solved that way, with a SelectionFollowsFocus / SelectionFollowsHighlightedItem bool property, to automatically select rows that become "focused"/highlighted with keyboard scrolling. Other focus use cases may still have to resort to making custom row widgets that are themselves focusable. (Which I think in many ways is better anyway, though more complex)

[csturiale]

Can OnListItemHover be a better name?

[andydotxyz]

I don't think we should expose it at that level as the semantics of hover are not consistent across different platforms.

As with our API design across widgets we should really expose something that describes the outcome or behaviour.

[csturiale]

Maybe the currentFocus field is also a bit misleading following your reasoning.
@andydotxyz is it ok to update the PR by adding the field SelectionFollowsFocus bool ?

[andydotxyz]

Maybe the currentFocus field is also a bit misleading following your reasoning.

That is indeed correct, private variables don't have the same naming rigour as the public API.

@andydotxyz is it ok to update the PR by adding the field SelectionFollowsFocus bool ?

If that is the behaviour you are looking for then I can't think of a better way to do it.

Bear in mind that we try to keep the API for all collection widgets in sync (List, Table, Tree, GridWrap) so you should consider adding it for all.