AhmedLSayed9 / dropdown_button2

Flutter's core Dropdown Button widget with steady dropdown menu and many other features.
https://pub.dev/packages/dropdown_button2
MIT License
264 stars 122 forks source link

Add an ability to override comparator function #296

Open vasilich6107 opened 2 months ago

vasilich6107 commented 2 months ago

There are some cases when we have no ability to override the == operator for example in case when we get a list of objects from external source. It would be nice to have an ability to override object comparator function which is used to compare objects

image
AhmedLSayed9 commented 2 months ago

You can override the == operator for the object itself. or, use some unique value at the object like an id to be used as value.

AhmedLSayed9 commented 2 months ago

@vasilich6107 Can you check https://github.com/AhmedLSayed9/dropdown_button2/issues/127 and see if it's what you're looking for?

vasilich6107 commented 2 months ago

What if I have no access to object class?

Imagine that I'm getting user info with role object UserRole another api returns a list of available roles List<UserRole>

I have to map this classes into custom objects to have an ability to override == operator Otherwise I will have to use id as value and list of id's as items and build selected item with searching through the list of items. Ability to override the comparator will make it easy in case of refactoring if you have something already created but you need to change only comparison method

vasilich6107 commented 2 months ago

even if I have an ability to override == there are cases when you have a need to change the comparator method on the fly.

You have a list of items, that could be disabled on BE. One entity assigned to disable person. So be will return a list of only enabled persons.

at this point you have a need to inject this preselected item into the list of items with updated name 'deactivated` this is the case when it is nice to have comparator update.

Do you see any disadvantages on adding an ability to set custom comparator?

vasilich6107 commented 2 months ago

If it works for you I can create a PR

AhmedLSayed9 commented 2 months ago

Do you see any disadvantages on adding an ability to set custom comparator?

You can set custom comparator by using value.

I'm not sure what value will this add over doing:

class _MyHomePageState extends State<MyHomePage> {
  final List<Item> items = const [
    Item('1', 'Item1'),
    Item('2', 'Item2'),
    Item('3', 'Item3'),
    Item('4', 'Item4'),
  ];
  final valueListenable = ValueNotifier<String?>(null);

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: DropdownButtonHideUnderline(
          child: DropdownButton2<String>(
            isExpanded: true,
            hint: Text(
              'Select Item',
              style: TextStyle(
                fontSize: 14,
                color: Theme.of(context).hintColor,
              ),
            ),
            items: items
                .map((Item item) => DropdownItem<String>(
                      value: item.id,
                      height: 40,
                      child: Text(
                        item.title,
                        style: const TextStyle(
                          fontSize: 14,
                        ),
                      ),
                    ))
                .toList(),
            valueListenable: valueListenable,
            onChanged: (String? value) {
              valueListenable.value = value;
            },
            buttonStyleData: const ButtonStyleData(
              padding: EdgeInsets.symmetric(horizontal: 16),
              height: 40,
              width: 140,
            ),
          ),
        ),
      ),
    );
  }
}

class Item {
  const Item(this.id, this.title);

  final String id;
  final String title;
}
Mir1001 commented 1 month ago

I am experiencing a similar issue. Specifically, I am retrieving a list of users from an API, which includes the currently logged-in user but with less detailed information (same ID, public data, etc.). When I try to set the selected item/object to the logged-in user, the object isn't found because the equality operator (==) is auto-generated by Freezed and I prefer not to modify it.

As a workaround, I could replace the logged-in user in the list if it exists or use just IDs as values, though this approach is less intuitive. What is best approach?

AhmedLSayed9 commented 1 month ago

I am experiencing a similar issue. Specifically, I am retrieving a list of users from an API, which includes the currently logged-in user but with less detailed information (same ID, public data, etc.). When I try to set the selected item/object to the logged-in user, the object isn't found because the equality operator (==) is auto-generated by Freezed and I prefer not to modify it.

As a workaround, I could replace the logged-in user in the list if it exists or use just IDs as values, though this approach is less intuitive. What is best approach?

What's wrong with using id as value here?

Mir1001 commented 1 month ago

AhmedLSayed9 Thanks. It is true.

I was under the impression that the types for items and DropdownButton2 needed to be the same type.

...
        items: items //can be type of Item
                .map((Item item) => DropdownItem<String>( //type of key
                      value: item.id,
                      height: 40,
vasilich6107 commented 1 month ago

The comparison fn compares value and item.value It would be preferable to have unified list of entities both cases and override comparison fn.

In your example https://github.com/AhmedLSayed9/dropdown_button2/issues/296#issuecomment-2178993745 value will be id so when I need to do something with the output from dropdown I need to search again through

final List<Item> items = const [
    Item('1', 'Item1'),
    Item('2', 'Item2'),
    Item('3', 'Item3'),
    Item('4', 'Item4'),
  ];

to get the actual item being selected.

It would be easier to have comparison function.

Could you clarify how suggested approach works with sealed classes which should be compared by type.

What should I do if I have

sealed calss A {}

class B extends A {

}

class C extends A {

}

so I want my items to be

items: [C(), B(),]

The A and B classes does not have similar fields, they could be compared only by type.

AhmedLSayed9 commented 1 month ago

In your example #296 (comment) value will be id so when I need to do something with the output from dropdown I need to search again through

final List<Item> items = const [
    Item('1', 'Item1'),
    Item('2', 'Item2'),
    Item('3', 'Item3'),
    Item('4', 'Item4'),
  ];

to get the actual item being selected.

It would be easier to have comparison function.

Yes, you'll have to search by id to get the Item back but that's not a big deal even with huge lists.

Also, you can always override the == operator for the object so this is only needed when you're using an object from another api and you don't want to extend it.

Could you clarify how suggested approach works with sealed classes which should be compared by type.

What should I do if I have

sealed calss A {}

class B extends A {

}

class C extends A {

}

so I want my items to be

items: [C(), B(),]

The A and B classes does not have similar fields, they could be compared only by type.

I'm not sure if there's a real use-case like this but anyway, you can do:

sealed class A {
  const A();
}

class B extends A {
  const B();
}

class C extends A {
  const C();
}
items: const [C(), B(),]
vasilich6107 commented 1 month ago

Yep This is a real case.

I have form with the data field. Data field represented by a bunch of classes extended from sealed one.

I have a dropdown where I want to put an array of classes as items.

Then use dropdown to swap the data class for a part of the form.

The dropdown throws cause it is unable to find the value among items...

Could you be so kind and allow users to override comparison method?

vasilich6107 commented 1 month ago

Yes, you'll have to search by id to get the Item back but that's not a big deal even with huge lists.

Yes, this is not a performance penalty but instead of having a comparison method users have to do the workarounds with id to class mapping.

AhmedLSayed9 commented 1 month ago

Could you be so kind and allow users to override comparison method?

The thing is this how the core DropdownButton works and it's more familiar to users.

Also, we don't want to add more apis and complicate things unless it's really needed.

I still can't find a strong reason for doing:

valueComparator: (value) => value.id,

rather than:

value: value.id,

The only benefit here is the user won't need to search back to get the actual item.

I'm thinking we might make it optional and keep value then user can only use one of value and valueComperator but this is repetitive and I don't like it as one property can do both, or, just update value to simplify things then user will have to do:

value: (value) => value,

which seems a bit weird.

That's why I'm a bit skeptical about this change.

AhmedLSayed9 commented 1 month ago

Also, this will break the dropdown menu items consistency, as they use the same type specified for the DropdownButton.

/// The type `T` is the type of the value the entry represents. All the entries
/// in a given menu must represent values with consistent types.

This change will allow items to have different value types which is not allowed.

AhmedLSayed9 commented 1 month ago

Furthermore, You'll have to set valueListenable that represents the value of the currently selected [DropdownItem] with the same value you set to the item. This means you'll still need to search back to get the actual item :)

vasilich6107 commented 1 month ago

I still can't find a strong reason for doin

What about the example with sealed classes?

AhmedLSayed9 commented 1 month ago

What about the example with sealed classes?

I've answered above.

vasilich6107 commented 1 month ago

But if I set the value to dropdown it will produce an assertion error in console that it is not able to find a corresponding item among items

AhmedLSayed9 commented 1 month ago

@vasilich6107 Can you show an example of what you're trying to do? Maybe we're looking for a wrong solution after all (see https://xyproblem.info)

vasilich6107 commented 1 month ago

Sure will prepare the example this week