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

WIP: Implement Streams for Forms #1162

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

erayerdin
Copy link
Contributor

@erayerdin erayerdin commented Dec 5, 2022

Connection with issue(s)

Close #1155

Solution description

FormBuilderState now has a StreamSubscription named onChanged so that people can subscribe to the stream of changes and react to the changes.

This PR is a WIP (work-in-progress) and should not be merged yet.

  • Devs can use _formKey.currentState!.onChanged.listen((FormBuilderFields fields) {}) to listen to the form changes.
  • Devs can use _formKey.currentState!.onChanged.cancel() to dispose of the stream.
  • A very primitive example has been implemented, but is subject to change. See the video below.
  • I have no idea as to how to test the streams. I have been trying to test it for a week now and cannot do so. That is the main reason why I don't want this feature to be merged. Let me first figure out a way to test the streams. The example works like a charm, though.

Screenshots or Videos

Peek.2022-12-05.15-29.mp4

This is what I added in example project, which is not very elegant to look at. I will refactor this later on. This is only for demo purposes.

As you can see, StreamBuilder reacts to the changes live and re-renders each time the form is updated.

To Do

  • Read contributing guide
  • Check the original issue to confirm it is fully satisfied
  • Add solution description to help guide reviewers
  • Add unit test to verify new or fixed behaviour
  • If apply, add documentation to code properties and package readme

@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Merging #1162 (4ab0322) into main (76be651) will increase coverage by 0.02%.
The diff coverage is 87.50%.

❗ Current head 4ab0322 differs from pull request most recent head 9e8dd52. Consider uploading reports for the commit 9e8dd52 to get more accurate results

@@            Coverage Diff             @@
##             main    #1162      +/-   ##
==========================================
+ Coverage   84.93%   84.96%   +0.02%     
==========================================
  Files          19       19              
  Lines         697      705       +8     
==========================================
+ Hits          592      599       +7     
- Misses        105      106       +1     
Impacted Files Coverage Δ
lib/src/form_builder.dart 71.27% <87.50%> (+1.50%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@erayerdin
Copy link
Contributor Author

I stylized the example app. You can see the changes in realtime in a table.

Peek.2022-12-06.01-10.mp4

@deandreamatias
Copy link
Collaborator

@erayerdin take a look in this article. Help me a lot when I've implemented this tests

@erayerdin
Copy link
Contributor Author

erayerdin commented Dec 6, 2022

Tests are done. Added a section to readme. There are still things to do, though. Currently, StreamBuilder strangely needs setState to refresh its content in the example app. I'll try to write a StatelessWidget and investigate it.

I will also try how Submit and Reset buttons behave in the example app.

@erayerdin
Copy link
Contributor Author

So, I've changed example/main.dart on my local machine. This is what I've done, and it's the ideal usecase in my mind.

The Minimum Ideal Usecase

class CompleteForm extends StatelessWidget {
  final _formKey = GlobalKey<FormBuilderState>();

  CompleteForm({super.key});

  @override
  Widget build(BuildContext context) {
    return FormBuilder(
      key: _formKey,
      child: Scaffold(
        body: Padding(
          padding: const EdgeInsets.all(8),
          child: Center(
            child: Column(
              mainAxisSize: MainAxisSize.min,
              children: [
                FormBuilderTextField(name: 'text1'),
                StreamBuilder(
                  stream: _formKey.currentState!.onChanged,
                  builder:
                      (context, AsyncSnapshot<FormBuilderFields> snapshot) {
                    return Text(
                        snapshot.data?.values.first.value.toString() ?? 'null');
                  },
                ),
              ],
            ),
          ),
        ),
      ),
    );
  }
}

Why do I consider this as ideal?

  • It's a stateless widget. Flutter won't unnecessarily hold states for rerendering like stateful widget.
  • Stateless widgets are much more readable and comprehensible than stateless ones.

This doesn't work though. The reason is currentState in _formKey.currentState!.onChanged is null when the app is first loaded. Thus the whole app crashes saying null-check operator used in a null value.

This can be avoided by hot reloading or hot refreshing the app, but a real world user won't be able to do that.

What Works

class CompleteForm extends StatefulWidget {
  const CompleteForm({super.key});

  @override
  State<CompleteForm> createState() => _CompleteFormState();
}

class _CompleteFormState extends State<CompleteForm> {
  final _formKey = GlobalKey<FormBuilderState>();

  @override
  Widget build(BuildContext context) {
    return FormBuilder(
      key: _formKey,
      onChanged: () {
        setState(() {});
      },
      child: Scaffold(
        body: Padding(
          padding: const EdgeInsets.all(8),
          child: Center(
            child: Column(
              mainAxisSize: MainAxisSize.min,
              children: [
                FormBuilderTextField(name: 'text1'),
                StreamBuilder(
                  stream: _formKey.currentState?.onChanged,
                  builder:
                      (context, AsyncSnapshot<FormBuilderFields> snapshot) {
                    return Text(
                        snapshot.data?.values.first.value.toString() ?? 'null');
                  },
                ),
              ],
            ),
          ),
        ),
      ),
    );
  }
}

This one works because we do setState on form's each onChanged.

Why do I not find this ideal usecase?

  • Again, readability.
  • onChanged on form constantly does setState on each keypress, which is frequent enough that it is not desirable. It might cause some performance and consistency issues in some edge cases (like performance overhead or keyboard buttons not responding in some cases as the user is fast-typing).

So, from this point on, I need to figure out a way to make sure that currentState is not null and keep the devs on StatelessWidget. I'm not quite sure as to how, but I will figure it out somehow I guess.

@deandreamatias deandreamatias added the help wanted Extra attention is needed label Dec 19, 2022
@deandreamatias deandreamatias self-requested a review December 19, 2022 07:00
@erayerdin
Copy link
Contributor Author

These days, I'm busy with work, so I need to have a hold on this. Will come back to it tho.

@erayerdin
Copy link
Contributor Author

It's been almost a month. I've asked this question on SO. Let's see if someone can figure it out.

This comment from 1157 does not do anything good in our case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide Streams for FormBuilderState
2 participants