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

[help] Implementing multiple selection with custom object #97

Closed
Macacoazul01 opened this issue Dec 13, 2022 · 19 comments
Closed

[help] Implementing multiple selection with custom object #97

Macacoazul01 opened this issue Dec 13, 2022 · 19 comments
Labels
bug Something isn't working question Further information is requested

Comments

@Macacoazul01
Copy link
Collaborator

I'm having some problems on trying to implement multiple selection with a custom class.

Everything is rendered ok but on the console this log is printed:

Warning: didChange call threw an error: Expected a value of type 'Fornecedor?', but got one of type 'List<int>' C:/b/s/w/ir/cache/builder/src/out/host_debug/dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 266:49      throw_
C:/b/s/w/ir/cache/builder/src/out/host_debug/dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 99:3        castError
C:/b/s/w/ir/cache/builder/src/out/host_debug/dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 452:10  cast
C:/b/s/w/ir/cache/builder/src/out/host_debug/dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/types.dart 367:9        as
packages/search_choices/search_choices.dart 1304:7                                                                             sendSelection
packages/search_choices/search_choices.dart 1454:11                                                                            showDialogOrMenu

Maybe i forgot to implement some function...

Here's the sample code, @lcuis:
multi select.zip

Can you help me?

@Macacoazul01
Copy link
Collaborator Author

Also, another question: why the selection returns the indexes of selected values on the list instead of the values?

Is there any way to have the list of Fornecedor on the sample callback function?

@lcuis
Copy link
Owner

lcuis commented Dec 13, 2022

Hello @Macacoazul01 ,

Thanks for raising this!

Indeed, this is not ideal. I think this all comes down to the fact that SearchChoices is typed with T where T is the custom object type. This is valid for the single case but this creates issues for the multiple case where T should probably refer to the List of custom objects type. This seems like a good but quite transforming change.

Here is my attempt at solving the didChange warning:

  sendSelection(dynamic selection, [BuildContext? onChangeContext]) {
    try {
      if(selection is List<int> && ! (widget.items?.first.value is List<int>)) {//trying to address the multiple case warning
        didChange(selection.map((i) => widget.items?[i].value,).toList());
      }
      else {
        didChange(selection);
      }
    } catch (e, st) {
      debugPrint(
          "Warning: didChange call threw an error: ${e.toString()} ${st.toString()} You may want to reconsider the declared types otherwise the form validation may not consider this field properly.");
    }
...

But this generates this compilation error:

error: The argument type 'List<T?>' can't be assigned to the parameter type 'T?'. (argument_type_not_assignable at [search_choices] lib/search_choices.dart:1305)

Solving this by changing the multiple case to take the list of objects as T seems like a long battle to me and I am not sure there are better alternatives.

Now, still in multiple case, it should be possible to have a new onChanged callback named something like onChangedMultiple that would receive the list of objects instead of the list of indexes. Is this something that would desirable to you?

@lcuis
Copy link
Owner

lcuis commented Dec 13, 2022

In fact, in the multiple case, this line:

class SearchChoices<T> extends FormField<T> {

should probably be like this:

class SearchChoices<T> extends FormField<List<T>> {

but this would break the single case.
This means there should be two SearchChoices classes and this would probably increase a lot the quantity of code. I might be wrong and this may be worth it anyway.
What do you think about this @Macacoazul01 ?

@Macacoazul01
Copy link
Collaborator Author

I'm gonna try exploring this part of the package

@lcuis
Copy link
Owner

lcuis commented Dec 13, 2022

Thanks a lot @Macacoazul01 !

@Macacoazul01
Copy link
Collaborator Author

@lcuis on the latest commit of #99 this seems fixed. It's still missing form validation but i don't think it's necessary on this case as a custom object is predefined

@lcuis
Copy link
Owner

lcuis commented Dec 14, 2022

Thanks a lot @Macacoazul01 !
I will merge, review, run the testing and probably do some minor adaptations and then publish to pub.dev .

@lcuis
Copy link
Owner

lcuis commented Dec 14, 2022

I had to interrupt while trying to integrate your custom objects examples in the list of examples. I will resume as soon as possible.

@Macacoazul01
Copy link
Collaborator Author

I've found some problems trying to implement this sample on a different way because you wrap only in the middle of the code the materialapp.

Gonna create another issue to attack this point.

@lcuis
Copy link
Owner

lcuis commented Dec 14, 2022

I've found some problems trying to implement this sample on a different way because you wrap only in the middle of the code the materialapp.

Gonna create another issue to attack this point.

Oh, I see. I will leave it as it is then for now.

@lcuis
Copy link
Owner

lcuis commented Dec 14, 2022

In fact, I was able to integrate your example after some simplification. I hope you won't mind.
For easy access, you can type "42" in the examples search text field at the top of the examples screen.

lcuis added a commit that referenced this issue Dec 14, 2022
@lcuis
Copy link
Owner

lcuis commented Dec 14, 2022

Automated testing shows that example 41 (Validator in form) fails with the latest version. I am trying to understand why.

@lcuis
Copy link
Owner

lcuis commented Dec 14, 2022

I had to restore the call to the Form didChange in case of multiple selection but I reduced the warning message.
bf698da
I think it is better from a developer perspective to still have the call and the warning in case of exception even if it is minimal.

@lcuis
Copy link
Owner

lcuis commented Dec 14, 2022

Release 2.2.0 has just been published to pub.dev with the mentioned changes. Though, this doesn't fully solve yet the multiple cases form didChange calls.

@Macacoazul01
Copy link
Collaborator Author

The way it is will still lead to the form warning, maybe you should maintain the old implementation with the warning being set on the else of if(!multiple selection)

@Macacoazul01
Copy link
Collaborator Author

As the didchange will always lead to the error

@lcuis
Copy link
Owner

lcuis commented Dec 14, 2022

I changed the warning in the multiple selection case to display just this:

Warning: SearchChoices multipleSelection doesn't fully support Form didChange call.

The didChange call leads to the error in some multiple selection cases but not all of them. For instance, example 41 "Validator in form" is a multiple selection case but does not display any warning with version 2.2.0 . I wonder what makes your example (42) triggering it while it is not triggered by example 41. Is it the fact that the list of items is made of custom objects?

@Macacoazul01
Copy link
Collaborator Author

@lcuis maybe this can improve the solution:
#102
As form validator only makes sense if the developer defined it, the code will only call the validator if defined for the list or item

@Macacoazul01 Macacoazul01 added bug Something isn't working question Further information is requested labels Dec 14, 2022
@lcuis
Copy link
Owner

lcuis commented Dec 15, 2022

Yes, I think this is a good idea that would solve your issue. Though, this is not solving all the cases and I would like to keep this issue open until this is solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants