Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #24 #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixes #24 #25

wants to merge 1 commit into from

Conversation

JChrist
Copy link

@JChrist JChrist commented Mar 13, 2020

When widget is created or updated, there should be a check whether selectedItems now points to an invalid item (e.g. removed)

When widget is created or updated, there should be a check whether `selectedItems` now points to an invalid item (e.g. removed)
@lcuis
Copy link
Collaborator

lcuis commented Mar 13, 2020

Hello @JChrist ,

Thank you very much for reporting the defect, providing the example and proposing the fix.
I was able to test your example and fix and it works. However, this only works when the removed item is the last in the list. I tried the following:

  1. Add a "Remove first Item" button:
          RaisedButton(child: Text('Remove first Item'),onPressed: () => setState(() => items.removeAt(0))),
  1. Select the first item of the list.
  2. Click the "Remove first Item" button.

Expected result: First item is deselected and nothing is selected.
Effective result: Second item is selected.

Here is the code I added to the example main for this test based on your example:

      "Remove item":Column(
        children: <Widget>[
          RaisedButton(child: Text('Remove first Item'),onPressed: () => setState(() => items.removeAt(0))),
          RaisedButton(child: Text('Remove last Item'),onPressed: () => setState(() => items.removeLast())),
          SearchableDropdown(items: items,
              value: selectedValue, onChanged: (e) => setState(() => selectedValue = e))
        ],
      ),

Here is the outcome:
issue24

Also, I would be interested in seeing what would happen in the multiple selection case. I believe this will be difficult to find a solution to be applied from the plugin code in this case because the selected list is made of indexes, not values. This is because it is impossible to know which values have been removed unless a previous version of the list is kept.

However, there are examples of editable lists for single and multiple selection cases with search_choices, maybe these can be adapted to your needs for searchable_dropdown:
single editable
multi editable

@JChrist
Copy link
Author

JChrist commented Mar 13, 2020

Hi @lcuis ,
thanks for your input.
I thought about this issue, but as you already correctly mentioned it stems from the fact that:

because the selected list is made of indexes, not values

The only way I can think of correctly addressing it, is heavily refactoring it to make selectedItems a List instead of List (i.e. list of values rather than list of indexes).
This way, we can change my current "fix" of removing selected items if their index is greater than the maximum number of items to removing selected items if their value is no more contained in one of the items.

However, that will most certainly contain breaking changes. Your thoughts?

@lcuis
Copy link
Collaborator

lcuis commented Mar 13, 2020

Hello @JChrist ,
As you mentioned, I believe that this is a good solution except that it will cost disruptions to current users of this index based implementation because of the refactoring. Adding a boolean parameter to be index or value based for selection lists would make the code a lot heavier IMHO. One way to address this could be a new forked version for value based selection lists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants