Look at slow duduplication in CheckGroup

[Jacalz]

Checklist

• [X] I have searched the issue tracker for open issues that relate to the same problem, before opening a new one.
• [X] This issue only relates to a single bug. I will open new issues for any other problems.

Describe the bug

Both RadioGroup and CheckGroup remove duplicated items each time they are refreshed. This is slow for large number of items and unnecessary to do multiple times.

The duplication was added because multiple items were showing as selected if the had the same string. We should try to remove duplication and just make sure that it is not showing multiple items as selected.

How to reproduce

Radio: https://github.com/fyne-io/fyne/blob/8c2509518b2df442a6b748d9b07754739592e6d7/widget/radio_group.go#L116 Check: https://github.com/fyne-io/fyne/blob/8c2509518b2df442a6b748d9b07754739592e6d7/widget/check_group.go#L149

Screenshots

No response

Example code

See links above ^

Fyne version

v2.3.0

Go compiler version

1.19.3

Operating system

Linux

Operating system version

Fedora 36

Additional Information

No response

[Jacalz]

The other option is of course to do the de-duplication less often but it would be good avoid if it all together if we can.

[andydotxyz]

I liked the idea of removing the (slow) code and ensuring that only an item tapped is selected to better fix the original bug.

[alx3dev]

I am not sure about original bug (is problem in append?) if yes, what about adding code to check if item exist before append?

function to be added:

func NotIncluded(options []string, value string) bool {
    result := true
    for x := 0; x < len(options); x++ {
        if options[x] == value {
            result = false
        }
    }
    return result
}

then in append:

// Append adds a new option to the end of a RadioGroup widget.
func (r *RadioGroup) Append(option string) {
    if NotIncluded(r.Options, option) {
        r.Options = append(r.Options, option)
        r.Refresh()
    }
}

[Bluebugs]

Options is public, so it would bypass the Append check :-( A solution is to cache the last value of Options when it was checked and only do the costly operation when there is a change.

[andydotxyz]

I think on the call we found a way to avoid needing the de-duplication code at all, by fixing the initial issue that multiple items were selected if they were duplicates.

[Jacalz]

Indeed. I will try to remove the function entirely and just make sure to not display multiple items, with the same name, as selected when one is clicked.

[Jacalz]

I have a looked a bit at this now and there is not a good way to remove the deduplication without having every item with the same name be selected as well (the change in https://github.com/fyne-io/fyne/pull/3782 kind of solves this but only until the whole widget is refreshed the next time).

Can we say that it is up to the develop to not include duplicates? I am kind of thinking that this is the best path forward as most usages of the RadioGroup and CheckGroup widgets are using hard-coded object slices from what I can tell. 95+% of developers do not need it to be deduplicated and those that do can do it themselves.

[andydotxyz]

I see the logic in your suggestion. Howe ever this code was added to resolve an issue where duplicates created unusual results. If we're removing it I guess it unresolves the original issue?

[Jacalz]

Original bug for reference: https://github.com/fyne-io/fyne/issues/213

I see the logic in your suggestion. Howe ever this code was added to resolve an issue where duplicates created unusual results. If we're removing it I guess it unresolves the original issue?

Yes, unfortunately. We can't continue having it do what it currently is doing and allocating lots each refresh but I don't see much of a way around it. If we are to remove de-duplication, I do see two options:

1. Deduplicate once in NewRadioGroup and avoid doing it for refresh. Probably not that nice of an option.
2. Remove it entirely and say that it is up to the developer to do the de-duplication.

[dweymouth]

At the very least we could avoid allocating memory each refresh by using a sync.Pool to re-use previously allocated slices/maps. Might this be worth looking into?

[dweymouth]

The other option, which maybe could be better, is to internally store the selected item as an index and string. When the user clicks a duplicated radio button, the index will be stored. On refresh, if the public Selected property (string) and selectedIndex (int) don't match, ie Options[selectedIndex] != Selected, then it will set selectedIndex to the first matching option

[Jacalz]

I like the second option. The first one seems like an improvement but more like a hack than actually fixing the problem at hand.