Consistent Selection API

[stuartmscott]

Is your feature request related to a problem? Please describe:

Selection APIs in widgets and collections are inconsistent.

Is it possible to construct a solution with the existing API?

n/a

Describe the solution you'd like to see:

Radio.ClearSelection()
Radio.SetSelectedOption(option string)
Radio.OnSelectionChanged(option string)

Select.ClearSelection()
Select.SelectedIndex() int
Select.SetSelectedOption(text string)
Select.SetSelectedIndex(index int)
Select.OnSelectionChanged(option string)

TabContainer.SelectedTab() *TabItem
TabContainer.SelectedIndex() int
TabContainer.SetSelectedTab(item *TabItem)
TabContainer.SetSelectedIndex(index int)
TabContainer.OnSelectionChanged(item *TabItem)

List.ClearSelection()
List.SelectedItem() int
List.SetSelectedItem(index int)
List.OnSelectionChanged(index int) // -1 == no selection

Table.ClearSelection()
Table.SelectedCell() (int, int)
Table.SetSelectedCell(row, col int)
Table.OnSelectionChanged(row, col int) // -1, -1 == no selection

Tree.ClearSelection()
Tree.SelectedNode() string
Tree.SetSelectedNode(uid string)
Tree.OnSelectionChanged(uid string) // tree.noSelection = "nothing" == no selection

Deprecation warning will be added in 1.4 and the APIs will be renamed in 2.0

[andydotxyz]

Radio and Select export the selection as Selected hence the setter is currently SetSelected. If we Change the setters we might need to change Field/Getter as well.

I think that we might find that using the same API for collection/container and simple input widgets may not be the simplest solution.

[stuartmscott]

As agreed in Slack, collections will use;

List.ClearSelection()
List.Selection() int
List.SetSelection(index int)
List.OnSelectionChanged(index int) // -1 == no selection

Table.ClearSelection()
Table.Selection() (int, int)
Table.SetSelection(row, col int)
Table.OnSelectionChanged(row, col int) // -1, -1 == no selection

Tree.ClearSelection()
Tree.Selection() string
Tree.SetSelection(uid string)
Tree.OnSelectionChanged(uid string) // tree.noSelection = "nothing" == no selection

[stuartmscott]

I think there is value in extending this convention to other widgets;

Radio.ClearSelection()
Radio.Selection() string
Radio.SelectionIndex() int
Radio.SetSelection(option string)
Radio.OnSelectionChanged(option string)

Select.ClearSelection()
Select.Selection() string
Select.SelectionIndex() int
Select.SetSelection(text string)
Select.SetSelectionByIndex(index int)
Select.OnSelectionChanged(option string)

TabContainer.Selection() *TabItem
TabContainer.SelectionIndex() int
TabContainer.SetSelection(item *TabItem)
TabContainer.SetSelectionByIndex(index int)
TabContainer.OnSelectionChanged(item *TabItem)

[andydotxyz]

Happy with the collection/container stuff. Still a little unsure about Radio/Select as the exposed fields and nature of being input elements makes it a little less obvious that the changes fit. (They have exported Selected fields so Selection() getter would be weird).

[andydotxyz]

What other actions should we take on this ticket, should anything be moved out to another ticket?

[stuartmscott]

Let's move this over to 2.0 because I want to apply this consistency to AppTab and DocTab as well

[stuartmscott]

Including support for Multiple Selections has complicated the issue, however a Slack discussion has yielded a potential solution:

Collections by default will use single selection, and will have a field MultiSelect bool for the developer to enable multiple selection.

The selection API will use slices so they work in both single selection and multiple selection use cases;

list.OnSelectionChanged = func(indicies []int) {}
table.OnSelectionChanged = func(cells []Cell) {} // where type Cell struct {Row, Col int}
tree.OnSelectionChanged = func(uids []string) {}

In the above callbacks, developers can use if indicies|cells|uids == nil || len(indicies|cells|uids) == 0 to check whether the selection is empty.

The rest of the API will be;

// Get all current selections
func (l *List) Selection() []int
func (t *Table) Selection() []Cell
func (t *Tree) Selection() []string

// Clear all selections
func (l *List) ClearSelection()
func (t *Table) ClearSelection()
func (t *Tree) ClearSelection()

// Add selection to slice if MultiSelect, else replace slice with slice of single element (first parameter)
func (l *List) Select(int...)
func (t *Table) Select(Cell...)
func (t *Tree) Select(string...)

// Remove from selection slice
func (l *List) Unselect(int...)
func (t *Table) Unselect(Cell...)
func (t *Tree) Unselect(string...)

[stuartmscott]

Moving back to the 1.4 milestone so we can finalize the API before its first public release

[okratitan]

I really like this proposal and think it is the first one that has a clear way of consistently, and concisely I might add (Not 3-5 additional methods or 3 constants, or additional api), representing selection that works for every case we will have. Every other proposal so far has ended with debates with seemingly no solution or agreement for handling empty selection. This does it ideally with nil. Further any other proposal I've seen is going to carry technical debt by releasing apis that are not designed for multiple selection cases and A. subject to change in the near term OR B. Subject to be duplicated/made redundant.

[andydotxyz]

I worry that this seemingly overcomes the complexity of potential row/column selection in table by ignoring it. If the multiple selection is of Cell (which I think is missing from the illustration) then it seems like we’d need more APIs added to handle row/col which is where we started to see complexity using the previous model too

[okratitan]

func (t Table) Selection() []Cell <--- multiple selection of cell. No APIs needed to handle row/col. Row and column selections shouldn't be passed or equated to being a selection value of table. They are convenience function for selecting all cells in a row or column. They themselves are not part of the value of what is being selected in a table. Clicking on a row or column and thus "selecting" it is selecting all of the cells in that row or column and that is what should be passed/managed in consistency. So func (t Table) Selection() []Cell handles that case fine.

[andydotxyz]

I understand your position @okratitan that a row selection could be identified by just the cells that it contains, but implementing this way is limiiting us and many mainstream applications do not seem to handle selection like this, for example common spreadsheet apps refer to "E:E" for "all cells in column E" instead of "E1:E10000".

Additionally I would think carefully about the implications of using only cells to identify large selections, for example on a table that is 20'000x20'000 selecting a single row using row notation is 1 item of information to share, but using cells it is 20'000 items in the selection slice. If the user then selects 5 columns the col/row approach would have 5 data points to compare against whereas the cell based approach would be iterating through 100'000 items in the selection slice. This seems likely to become very slow without need.

[andydotxyz]

Thanks for the illustration @stuartmscott but I have concerns about designing and implementing an API for multi-selection in widgets that are not currently capable of this:

1) This could be rather confusing to developers - it is a singular data item using a slice notation 2) It will lead to many bug reports about multi selection not workiing 3) It is based on assumptions about how future multiple selection will work that we currently cannot test.

[okratitan]

Including support for Multiple Selections has complicated the issue, however a Slack discussion has yielded a potential solution:

Collections by default will use single selection, and will have a field MultiSelect bool for the developer to enable multiple selection.

The selection API will use slices so they work in both single selection and multiple selection use cases;

list.OnSelectionChanged = func(indicies []int) {}
table.OnSelectionChanged = func(cells []Cell) {} // where type Cell struct {Row, Col int}
tree.OnSelectionChanged = func(uids []string) {}

In the above callbacks, developers can use if indicies|cells|uids == nil || len(indicies|cells|uids) == 0 to check whether the selection is empty.

The rest of the API will be;

// Get all current selections
func (l *List) Selection() []int
func (t *Table) Selection() []Cell
func (t *Tree) Selection() []string

// Clear all selections
func (l *List) ClearSelection()
func (t *Table) ClearSelection()
func (t *Tree) ClearSelection()

// Add selection to slice if MultiSelect, else replace slice with slice of single element
func (l *List) Select(int)
func (t *Table) Select(Cell)
func (t *Tree) Select(string)

// Remove from selection slice
func (l *List) Unselect(int)
func (t *Table) Unselect(Cell)
func (t *Tree) Unselect(string)

I'm going to take a stab to update this for range support which I think should resolve a lot of other concerns:

Including support for Multiple Selections has complicated the issue, however a Slack discussion has yielded a potential solution:

Collections by default will use single selection, and will have a field MultiSelect bool for the developer to enable multiple selection.

The selection API will use slices so they work in both single selection and multiple selection use cases;

type ListRange struct {
        FirstIndex int,
        LastIndex int
}
type TableCell struct {
        Column int,
        Row int
}
type TableRange struct {
        FirstCell TableCell,
        LastCell TableCell
}

list.OnSelectionChanged = func(ranges []ListRange) {}
table.OnSelectionChanged = func(ranges []TableRange) {}
tree.OnSelectionChanged = func(uids []string) {}

In the above callbacks, developers can use ranges|uids == nil || len(ranges|uids) == 0 to check whether the selection is empty.

The rest of the API will be;

// Get all current selections
func (l *List) Selection() []ListRange
func (t *Table) Selection() []TableRange
func (t *Tree) Selection() []string

// Clear all selections
func (l *List) ClearSelection()
func (t *Table) ClearSelection()
func (t *Tree) ClearSelection()

// Add selection to slice if MultiSelect, else replace slice with slice of single element
func (l *List) Select(int)
func (t *Table) Select(TableCell)
func (t *Tree) Select(string)

// Remove from selection slice
func (l *List) Unselect(int)
func (t *Table) Unselect(TableCell)
func (t *Tree) Unselect(string)

[andydotxyz]

I don't want to copy for all of the widgets as above, I think we have established that they should be consistent. The crux of my confusion is abour how we don't have multiple selection but the recent proposals force this in the API somewhat prematurely. I want to ensure that the callback types in selection match the callback types in the data handling, as they are referring to the same things. I would also like to see that we can elegantly handle row and column selection in table. Given that table seems to be the problematic one, here is a proposal for just that (list and tree would probably not adjust accordingly with minor name changes.

// Clear all
func (t *Table) ClearSelection()

// Select a cell - without multiple support this clears others
func (t *Table) Select(row, col int)

// get the current selection -- in a spreadsheet this is the cell that would be editable, even if other selections exist
func (t *Table) Selection() (row, col int)

// Unselect a single cell
func (t *Table) Unselect(row, col int)

/* then to add multiple support */

// turn on multiple selection in a table supporting it - off by default
Table.MultiSelection bool // or similar, that style matches Entry.MultiLine

// Get all current selections
func (t *Table) AllSelections() []TableSelection //initially just (row, col int) but we probably want range too

/* which could be used as so */

t.OnSelectionChanged = func(row, col int) {
  // could use just that for single - if we want to add row or col selections would be:
  if row == NoSelection {
    if col == NoSelection {
      // nothing selectted
    } else {
      // col selected 
    }
  } else {
    // row selected
  }
  // fall through to normal selection

  // OR if we want to know about multi we:
  all := list.AllSelections()
  if len(all) == 0 {
    // All unselected
  } else {
    // Selection made
    for _, sel := range all
  }
}

This supports a simple API for now, a match of data index API and a way to extend to multiple selections when we understand that requirement better.

[andydotxyz]

Following the call about this we have a new proposal that should satisfy the various requirements and has a plan for the future.

We managed to propose that these remain "simple" widgets so complexities like row / col headers, sorting and selection would be as part of an intermediate "presenter" that would manage data and some advanced features (assuming non-homogenous cells in table). So we were able to dismiss this from the collections widget APIs.

Single selection (1.4)

We introduce an ID type for each. Table benefits as it documents the two ints, tree benefits because we can express uniqueness of ID in just one place. And it's consistent

[Table]CellID is a struct of {int, int}
[List]ItemID is a typedef for int
[Tree]NodeID is a typedef for string

Each widget should be updated throughout to use these types for consistency (i.e. UpdatedCell(TableCellId))

For singular selection we expose On... for the selection and the unselection avoiding the need for zero check. Internally we can store as a slice for our benefit

Table.OnSelected func(TableCellID)
Table.OnUnselected func(TableCellID)

func (t *Table) Select(TableCellID) // first unselects existing, then selects new
func (t *Table) Unselect(TableCellID)

// then list
List.OnSelected func(ListItemID)
List.OnUnselected func(ListItemID)

func (l *List) Select(ListItemID)
func (l *List) Unselect(ListItemID)

// and tree
Tree.OnSelected func(TreeNodeID)
Tree.OnUnselected func(TreeNodeID)

func (t *Tree) Select(TreeNodeID) <- first unselects existing, then selects new
func (t *Tree) Unselect(TreeNodeID)

Moving to multi (1.5/2.0)

This is a simple implementation with no ranges and no optimisations

Add boolean Table.MultiSelect bool, defaults to false. Then we just add func (t *Table) SetSelection(TableCellID...) that iterates through existing selection to cancel them then iterates through the new ones to select.

List would have List.MultiSelect bool and func (l *List) SetSelection(ListItemID...), that iterates cancelling excisting, then selecting those passed.

Range

We could choose between the following options (but no impact on 1.4 or the multiselection above). Probably no range support in Tree, until we can find a sensible way to express that it can only exist for notes with a common parent.

Range A, new exported types

A new range type, per collection, so we can have a Selection() []rangetype function.

TableRange type {start, end TableCellID}

Table.OnSelectedRange func(TableRange)
Table.OnUnselectedRange func(TableRange)

func (t *Table) SelectRange(TableRange) <- deselects any items/ranges inside this range, then adds to list or ranges
func (t *Table) UnselectRange(TableRange)

func (t *Table) Selection() []TableRange
func (t *Table) ClearSelection()
// and List
ListRange type {start, end ListItemD}

List.OnSelectedRange func(ListRange)
List.OnUnselectedRange func(ListRange)

func (l *List) SelectRange(ListRange) <- deselects any items/ranges inside this range, then adds to list or ranges
func (l *List) UnselectRange(ListRange)

func (l *List) Selection() []ListRange
func (l *List) ClearSelection()

Range B (no new exported type, no Selection())

Table.selection []tableRange // probably want an internal type for tracking

Table.OnRangeSelected func(start, end TableCellID)
Table.OnRangeUnselected func(start, end TableCellID)

func (t *Table) SelectRange(start, end TableCellID) <- deselects any single items inside this range, otherwise adds to list or ranges
func (t *Table) UnselectRange(start, end TableCellID)

// list
List.selection []listRange // probably want an internal type for tracking

List.OnRangeSelected func(start, end ListItemD)
List.OnRangeUnselected func(start, end ListItemD)

func (l *List) SelectRange(start, end ListItemD) <- deselects any single items inside this range, otherwise adds to list or ranges
func (l *List) UnselectRange(start, end ListItemD)

[stuartmscott]

Consider renaming ClearSelection to UnselectAll as this could be accompanied by SelectAll and has a nice symmetry with Select, Unselect, as well as SelectRange, UnselectRange.

[andydotxyz]

That seems like a good proposal. Keep the symettry whilst adding convenience once we support multi selection. I think we are still good with the currrent proposal for 1.4 right?

[stuartmscott]

Single selection API landed in 1.4, moving to 2.0 milestone for multi selection

[fpabl0]

Maybe I am out of context, but having exported Range types for multiple selection isn't limit the usage for only contiguous selection?

On the other hand, is it possible to have ClearSelection/UnselectAll on widget.List soon?

[andydotxyz]

Yes I think exporting the Range types may be the less clear of the options, I think that option B is more interesting as it's just additive. That said I suppose that ClearSelection could be added now and remain useful going into multiple select land. Internally it will perform the appropriate UnSelect callbacks.

Does that sound OK @stuartmscott @okratitan ?

[okratitan]

I think as @fpabl0 said, option B looks like it would require the selection to be contiguous where as option A doesn't so I think option A is probably the better option there with some tweaking to the type definition?

Also... for now Unselect will work as a ClearSelection as it doesn't actually care what ID is passed in except to call the OnUnselected callback. So for now until we have ClearSelection if you don't care about/are ignoring the OnUnselected events anyway you can just do list.Unselect(anyrandomnumber) for instance and it will unselect whatever was selected.

I think adding a ClearSelection is reasonable though.

Also now that I think about it we decided on UnselectAll over ClearSelection I think?

[andydotxyz]

I don't want to speak for @fpabl0 but his message seemed to be referring to the challenges of an exposed type, which was option A. Option B suppports non-contiguous ranges (as would A I think) by calling the range selection multiple times. Much like, with a multiple select enabled, it would be possible to call Select many times with different cells, neither APIs enforce a single range based selection.

if you don't care about/are ignoring the OnUnselected events anyway you can just do list.Unselect(anyrandomnumber) for instance and it will unselect whatever was selected.

I would not recommend this as it will break in the future.

[stuartmscott]

Range is contiguous, however when combined with multi-select, could multiple ranges be selected at the same time?

Range support should be implemented after multi-select, and maybe alongside row/column headers in table so a user could tap a column header to select all cells that column.

As mentioned earlier I would prefer UnselectAll() over ClearSelection(), as it would pair well with SelectAll().

[fpabl0]

That said I suppose that ClearSelection could be added now and remain useful going into multiple select land. Internally it will perform the appropriate UnSelect callbacks.

Great!

I think as @fpabl0 said, option B looks like it would require the selection to be contiguous where as option A doesn't so I think option A is probably the better option there with some tweaking to the type definition? I don't want to speak for @fpabl0 but his message seemed to be referring to the challenges of an exposed type, which was option A.

Well, I don't think either option A or B are viable, both does not have a way to append Range selections that will limit to contiguous selection. This API is really hard to define 🤔. Maybe there could be a subtype inside all selectable widgets called Selection:

ListRange type {start, end ListItemID}

list := widget.NewList(...)

list.Selection.Set(ListItemID) // useful for single-selection (basically RemoveAll then Add)
list.Selection.SetRange(ListRange) // useful for single-range-selection (basically RemoveAll then AddRange)
list.Selection.Add(ListItemID) // useful for multi-selection
list.Selection.AddRange(ListRange) // useful for multi-selection
list.Selection.Remove(ListItemID)
list.Selection.RemoveRange(ListRange)
list.Selection.RemoveAll()
list.Selection.Get() []ListItemID
list.Selection.GetInRanges() []ListRange

Internally, list or table will store the selection in Ranges. If the developer calls Get() this function will generate a flatten list of ids from the Range type. GetInRanges() will directly return the saved ListRanges.

However this will obviously break all the current selectable widgets 😞.

cc @andydotxyz @okratitan @stuartmscott

[andydotxyz]

Well, I don't think either option A or B are viable, both does not have a way to append Range selections that will limit to contiguous selection.

Calling SelectRange (in multi-select mode) will not erase previous range. This is how you make non-contiguous ranges. I think you are trying to solve a problem that does not exist.

[fpabl0]

Calling SelectRange (in multi-select mode) will not erase previous range. This is how you make non-contiguous ranges. I think you are trying to solve a problem that does not exist.

Oh you are right (I hadn't scrolled to see the whole comment in the code you illustrated, sorry), however it would be confusing having SelectRange() behaves differently depending on Multi-select mode flag, don't you think it?

Also there would be great to have a way to retrieve selection as individual elements as long as range elements:

Selection() []ListItemID
SelectionInRanges() []ListRange

Names may be not good, but basically that is the idea. As I said above, the widget will hold selection in ranges, Selection() would only convert the range into a flat slice (useful when you select a small number of elements)

[andydotxyz]

There is no avoiding the fact that multi-select and single-select modes are different.

The get-multiple API is indeed something that needs work, and this is why we have not added Selection calls currently because we figured it needed work.

[andydotxyz]

Moving out to future release where we will handle Range and any remaining issues

[Roemer]

As a start, may I suggest to implement the following (for example to the Tree):

// Select adds the specified node(s) to the selection.
// If the tree is not multiselect, only the first given node will be processed
// and if it needs to be selected, all other currently selected nodes will be unselected.
Select(uids ...TreeNodeID)

// SelectAll adds all node(s) to the selection.
// If the tree is not multiselect, only the very first node will be selected.
SelectAll()

// SetSelection overwrites the current selection to the given nodes.
// If the tree is not multiselect, only the first node given will be selected.
SetSelection(uids ...TreeNodeID)

// GetSelection returns the currently selected nodes.
GetSelection() []TreeNodeID

// Unselect unselects all given nodes.
Unselect(uids ...TreeNodeID)

// UnselectAll unselects all nodes.
UnselectAll()

The difference between Select and SetSelection is that SetSelection would overwrite the current selection with the one given to it and Select would just additionally add the given ones to the current selection (if MultiSelect is true`).

This could later then be extended by also adding Range methods if wanted.

[andydotxyz]

The idea of getting selection (GetSelection or Selection as is more in-keeping with our API) works /until/ ranges are introduced. We have not progressed this further because we felt that the big picture needed to be figured out first. Single selection was easy - multiple, either many single items or via ranges, is more complex as these different modes can conflict. I don't think we can go and implement this partially with the expectation that range will be purely additive later on.

Also from the proposal above I think that Select vs SetSelection could be confusing. If the intent of the latter is to behave differently it should be clear from the name. Note also that the two would behave the same if multiple were enabled, though it is not documented what happens if multiple IDs are passed to a single select call...

[andydotxyz]

Also you need to consider the migration path. None of this can break existing API, so changing the signature of Select may not be possible, but adding SelectMultiple would be OK - and clearly indicates that it would only work with multiple selection enabled.