rrousselGit / state_notifier

ValueNotifier, but outside Flutter and with some extra perks
MIT License
311 stars 28 forks source link

StateNotifierProvider.value(... does not trigger state changes #9

Closed jamiewest closed 4 years ago

jamiewest commented 4 years ago

I can't seem to get the StateNotifierProvider to work when supplying an existing StateNotifier. The following modified example will not trigger any changes to the app.

void main() {
  var notifier = MyStateNotifier();

  runApp(
    MultiProvider(
      providers: [
        StateNotifierProvider<MyStateNotifier, MyState>.value(
          value: notifier
        )
      ],
      child: MyApp(),
    ),
  );
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      title: 'Flutter Demo',
      home: MyHomePage(),
    );
  }
}

class MyHomePage extends StatelessWidget {
  const MyHomePage({Key key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Counter example'),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text(
              'You have pushed the button this many times:',
            ),
            Text(
              context.select((MyState value) => value.count).toString(),
              style: Theme.of(context).textTheme.title,
            ),
          ],
        ),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: context.watch<MyStateNotifier>().increment,
        tooltip: 'Increment',
        child: Icon(Icons.add),
      ),
    );
  }
}
rrousselGit commented 4 years ago

I suspect that you broke the contact of StateNotifier by making state mutable.

Do you mind sharing your MyStateNotifier?

jamiewest commented 4 years ago

Same as your example from the repo.

class MyState {
  MyState(this.count);
  final int count;
}

class MyStateNotifier extends StateNotifier<MyState> with LocatorMixin {
  MyStateNotifier() : super(MyState(0));

  void increment() {
    state = MyState(state.count + 1);
  }

  @override
  @protected
  set state(MyState value) {
    if (state.count != value.count) {
      read<Logger>().countChanged(value.count);
    }
    super.state = value;
  }
}

The only thing I've really modified was creating an instance of MyNotifierState and passing it to StateNotifierProvider.value(...

rrousselGit commented 4 years ago

I've jut tested.

This is logical: when using StateNotifierProvider.value, the read/update/... functions are not connected to Flutter. As such trying to call read<Logger> does not work.

Objects using LocatorMixin can only be used with the default constructor

mono0926 commented 4 years ago

I understand that the exception occurs at here:

    if (state.count != value.count) {
      read<Logger>().countChanged(value.count);
    }

The following modified example will not trigger any changes to the app.

If commented out above lines, the app works fine.

Objects using LocatorMixin can only be used with the default constructor

But I feed this restriction is huge 🤔


I tried to make .value() constructor compatible with LocatorMixin.

  Widget buildWithChild(BuildContext context, Widget child) {
    return InheritedProvider.value(
      value: value,
      child: StateNotifierBuilder<Value>(
        stateNotifier: value,
        builder: (c, state, _) {
          // ↓Added
          if (value is LocatorMixin) {
            (value as LocatorMixin).read = _contextToLocator(context);
          }
          // ↑Added
          return Provider.value(
            value: state,
            builder: builder,
            child: child,
          );
        },
      ),
    );
  }

https://github.com/rrousselGit/state_notifier/blob/047899b247efe47f7e50a00c34b9fa646f47c8a8/packages/flutter_state_notifier/lib/flutter_state_notifier.dart#L197-L211

I know it isn't easy actually, but I'd like you to make .value() constructor compatible with LocatorMixin 🙏

rrousselGit commented 4 years ago

Actually, .value is voluntarily not compatible with LocatorMixin

Plugging LocatorMixin is very easy to do. The issue is elsewhere: We can have multiple .value() using the same StateNotifier

Consider:

StateNotifierWithLocator notifier;

return MaterialApp(
  routes: {
    '/': StateNotifierProvider.value(value: notifier, child: Home()),
    '/details':  MultiProvider(
      providers: [
        Provider.value(value: 42),
        StateNotifierProvider.value(value: notifier),
      ],
      child: Details(),
    ),
  }
)

Then in this example, the StateNotifierProvider.value inside /details has access to Provider.of<int> but the one inside / doesn't.\ That could lead to very awkward situations.

mono0926 commented 4 years ago

Thanks for the explanation, I understood 👍

rrousselGit commented 4 years ago

I agree that it is confusing though, and we can't even add an error message for it.

Hopefully, I would expect that people won't need to use .value() anymore once we have Navigator 2.0