Add create accessor factory constructor

[mr7ssam]

Use case in code:

ReactiveTextField<Duration>(
  suffix: const Icon(PIcons.outline_time_circle),
  // here
  valueAccessor:
      ControlValueAccessor<Duration, String>.create(
    modelToView: (modelValue) =>  prettyDuration(modelValue!),
  ),
  /////
  onTap: () async {
    var resultingDuration = await showDurationPicker(
      context: context,
      initialTime: const Duration(minutes: 30),
    );
    if (resultingDuration != null) {
      provider.controls.duration
          .updateValue(resultingDuration);
    }
  },
  readOnly: true,
  formControl: provider.controls.duration,
  labelText: 'Preparation time',
),

[vasilich6107]

@Husssam12 sounds reasonable. Maybe the doubling of accessor in ControlValueAccessor.stringAccessor is redundant and ControlValueAccessor.string is more elegant. Let's wait for @joanpablo response. Maybe it will make sense to add accessors for all dart basic types like int, double, and so on)

[mr7ssam]

@Husssam12 sounds reasonable. Maybe the doubling of accessor in ControlValueAccessor.stringAccessor is redundant and ControlValueAccessor.string is more elegant. Let's wait for @joanpablo response. Maybe it will make sense to add accessors for all dart basic types like int, double, and so on)

Thanks for replying, I made it more generic

[joanpablo]

Hi @Husssam12,

Thanks a lot for the issue and for supporting Reactive Forms.

This is quite interesting, and an attractive idea. But I have some questions.

First, why would you create an inline ControlValueAccesor instead of a separate class with single responsibility in another file (separated from the widget file)? In your example, you have the prettyDuration method that is supposed to be implemented in the same Widget class. Why not create a separated class with this single responsibility (i.e. the value accessor)?

I mean, don't make me wrong. I think it is an exemplary implementation of inline ControlValueAccessor, but I would like to understand an actual use case that a separated class doesn't solve.

As you may know, adding this feature implies more code to the package to maintain and more tests to add, so I want to make sure it is because of a good reason and that solves a real use case.

looking forward to hearing from you.

[mr7ssam]

Hi @Husssam12,

Thanks a lot for the issue and for supporting Reactive Forms.

This is quite interesting, and an attractive idea. But I have some questions.

First, why would you create an inline ControlValueAccesor instead of a separate class with single responsibility in another file (separated from the widget file)? In your example, you have the prettyDuration method that is supposed to be implemented in the same Widget class. Why not create a separated class with this single responsibility (i.e. the value accessor)?

I mean, don't make me wrong. I think it is an exemplary implementation of inline ControlValueAccessor, but I would like to understand an actual use case that a separated class doesn't solve.

As you may know, adding this feature implies more code to the package to maintain and more tests to add, so I want to make sure it is because of a good reason and that solves a real use case.

looking forward to hearing from you.

The first reason is that I am a bit lazy :crying_cat_face: , but actually I have two reasons:

1. the implemention of modelToViewValue is in another utils package so i have separate responsibility for that, in my example the package is duration,
2. In most cases (maybe in my project) I don't need to implement viewToModelValue.

So I think it's a slightly faster way to create ControlValueAccessor with less code.

I hope I have clarified my use case.

[joanpablo]

Hi @Husssam12

What about the "viewToModel" method? In your example you are defining a callback for the "modelToView", but you have not defined one for the "viewToModel":

ControlValueAccessor<Duration, String>.create(
    modelToView: (modelValue) =>  prettyDuration(modelValue!),
)

Don't you suppose to update the FormControl value from the Widget (the View) value? Shouldn't be the arguments modelToView and viewToModel mandatory instead of optional?

Thanks in advance.

[joanpablo]

Hi again @Husssam12,

In your example (use case) you are calling the method prettyDuration() from the duration package passing as an argument the modelValue, but you are not checking if the modelValue is null or not.

ControlValueAccessor<Duration, String>.create(
    modelToView: (modelValue) =>  prettyDuration(modelValue!),
)

Because the argument of the _ModelToViewValueCallback is a nullable type you should not trust you will receive a not null argument. A safe implementation would be:

ControlValueAccessor<Duration, String>.create(
    modelToView: (modelValue) => modelValue != null ? prettyDuration(modelValue) : '',
)

[mr7ssam]

Hi @Husssam12

What about the "viewToModel" method? In your example you are defining a callback for the "modelToView", but you have not defined one for the "viewToModel":

ControlValueAccessor<Duration, String>.create(
    modelToView: (modelValue) =>  prettyDuration(modelValue!),
)

Don't you suppose to update the FormControl value from the Widget (the View) value? Shouldn't be the arguments modelToView and viewToModel mandatory instead of optional?

Thanks in advance.

Hi again @Husssam12,

In your example (use case) you are calling the method prettyDuration() from the duration package passing as an argument the modelValue, but you are not checking if the modelValue is null or not.

ControlValueAccessor<Duration, String>.create(
    modelToView: (modelValue) =>  prettyDuration(modelValue!),
)

Because the argument of the _ModelToViewValueCallback is a nullable type you should not trust you will receive a not null argument. A safe implementation would be:

ControlValueAccessor<Duration, String>.create(
    modelToView: (modelValue) => modelValue != null ? prettyDuration(modelValue) : '',
)

Hi again @Husssam12,

In your example (use case) you are calling the method prettyDuration() from the duration package passing as an argument the modelValue, but you are not checking if the modelValue is null or not.

ControlValueAccessor<Duration, String>.create(
    modelToView: (modelValue) =>  prettyDuration(modelValue!),
)

Because the argument of the _ModelToViewValueCallback is a nullable type you should not trust you will receive a not null argument. A safe implementation would be:

ControlValueAccessor<Duration, String>.create(
    modelToView: (modelValue) => modelValue != null ? prettyDuration(modelValue) : '',
)

Sorry for the late reply That's what I missed, this didn't happen in my case because the field is read only, I will make changes for this and update the pull request.

[codecov[bot]]

Codecov Report

Patch coverage: 12.50% and project coverage change: -0.30 :warning:

Comparison is base (c04d108) 95.88% compared to head (ec3a283) 95.59%.

:exclamation: Current head ec3a283 differs from pull request most recent head 1891d38. Consider uploading reports for the commit 1891d38 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #283      +/-   ##
===========================================
- Coverage    95.88%   95.59%   -0.30%     
===========================================
  Files           69       63       -6     
  Lines         1338     1406      +68     
===========================================
+ Hits          1283     1344      +61     
- Misses          55       62       +7     

Impacted Files	Coverage Δ
...ib/src/value_accessors/control_value_accessor.dart	76.66% <12.50%> (-23.34%)	:arrow_down:

... and 32 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[joanpablo]

Hi @mr7ssam @vasilich6107,

Let me recap:

at this point we have something like this:

typedef ModelToViewCallback<ModelDataType, ViewDataType> = ViewDataType?
    Function(ModelDataType? modelValue);

typedef ViewToModelCallback<ModelDataType, ViewDataType> = ModelDataType?
    Function(ViewDataType? modelValue);

class InlineValueAccessor<ModelDataType, ViewDataType>
    extends ControlValueAccessor<ModelDataType, ViewDataType> {
  final ModelToViewCallback<ModelDataType, ViewDataType> _modelToView;
  final ViewToModelCallback<ModelDataType, ViewDataType> _viewToModel;

  InlineValueAccessor({
    required ModelToViewCallback<ModelDataType, ViewDataType> modelToView,
    required ViewToModelCallback<ModelDataType, ViewDataType> viewToModel,
  })  : _modelToView = modelToView,
        _viewToModel = viewToModel;

  @override
  ViewDataType? modelToViewValue(ModelDataType? modelValue) =>
      _modelToView(modelValue);

  @override
  ModelDataType? viewToModelValue(ViewDataType? viewValue) =>
      _viewToModel(viewValue);
}

And @mr7ssam based on your use case you will use it like this:

ReactiveTextField<Duration>(
  valueAccessor: InlineValueAccessor<Duration, String>(
    modelToView: (modelValue) =>  prettyDuration(modelValue!),
    viewToModel(viewValue) => null,
  ),
  readOnly: true,
  formControl: provider.controls.duration,
),

Do you still think it is better than the following solution?

ReactiveTextField<Duration>(
  readOnly: true,
  valueAccessor: DurationValueAccessor(),
  formControl: provider.controls.duration,
)

class DurationValueAccessor extends ControlValueAccessor<Duration, String> {
  @override
  String? modelToViewValue(Duration modelValue) => prettyDuration(modelValue!);

  @override
  Duration? viewToModelValue(String viewValue) => null;
}

looking forward to hearing from you.