icemanbsi / searchable_dropdown

MIT License
107 stars 166 forks source link

Fixes #24 #25

Open JChrist opened 4 years ago

JChrist commented 4 years ago

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

lcuis commented 4 years ago

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))),
  2. Select the first item of the list.
  3. 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 commented 4 years ago

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 commented 4 years ago

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.