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

Remove globalAppStore and add provider pattern for it #733

Merged
merged 20 commits into from
Sep 2, 2022

Conversation

Eldar2021
Copy link
Member

@Eldar2021 Eldar2021 commented Aug 23, 2022

Deleted global app store. Created Provider pattern for global store. Subtask of #132.

@Eldar2021 Eldar2021 requested a review from clangenb August 23, 2022 05:57
@clangenb clangenb changed the title E deleted global app store Remove globalAppStore and add provider pattern for it. Aug 27, 2022
@clangenb clangenb changed the title Remove globalAppStore and add provider pattern for it. Remove globalAppStore and add provider pattern for it Aug 27, 2022
@clangenb clangenb mentioned this pull request Aug 27, 2022
2 tasks
# Conflicts:
#	lib/app.dart
#	lib/main.dart
#	lib/page-encointer/bazaar/0_main/bazaar_main.dart
#	lib/page-encointer/home_page.dart
#	lib/page/account/create/add_account_page.dart
#	lib/page/account/create/create_account_page.dart
#	lib/page/account/create/create_pin_page.dart
#	lib/page/account/create_account_entry_page.dart
#	lib/page/account/import/import_account_page.dart
#	lib/page/assets/receive/receive_page.dart
#	lib/page/assets/transfer/detail_page.dart
#	lib/page/assets/transfer/payment_confirmation_page/index.dart
#	lib/page/assets/transfer/transfer_page.dart
#	lib/page/network_select_page.dart
#	lib/page/profile/account/account_manage_page.dart
#	lib/page/profile/account/change_password_page.dart
#	lib/page/profile/account/export_account_page.dart
#	lib/page/profile/contacts/account_share_page.dart
#	lib/page/profile/contacts/contact_detail_page.dart
#	lib/page/profile/contacts/contact_list_page.dart
#	lib/page/profile/contacts/contact_page.dart
#	lib/page/profile/contacts/contacts_page.dart
#	lib/page/profile/settings/remote_node_list_page.dart
#	lib/page/profile/settings/settings_page.dart
#	lib/page/profile/settings/ss58_prefix_list_page.dart
#	lib/page/qr_scan/qr_scan_page.dart
#	lib/page/reap_voucher/reap_voucher_page.dart
#	test_driver/app.dart
Copy link
Member

@clangenb clangenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have to request some systematic change for this PR. It seems in most of the places, we should use context.watch (sometimes context.select), instead of context.read.

With context.watch, we can still assign a variable inside the widget:

final store = context.watch<AppStore> and we will still be notified about state changes from the appStore, which will trigger a rebuild. Please come forward if I have misunderstood something.

@Eldar2021
Copy link
Member Author

It was not good to use context.read() in build method, I corrected it to context.watch() and context.select() in needed places.

@clangenb clangenb self-requested a review August 30, 2022 08:16
Copy link
Member

@clangenb clangenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I just noticed that I accidentally approved instead of requesting changes. I will review later again!

Copy link
Member

@clangenb clangenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I tested a basic encointer meetup with the test network, and it was successful, so I think the new provider pattern works as intended, good job! :)

I only have some minor comments, and questions.

One general point: I would like you to clean up the code a bit by assigning a variable final store = context.read<AppStore>, where we have many context.reads now. I think it makes the code more readable at many places. Do you agree?

lib/page/assets/receive/receive_page.dart Outdated Show resolved Hide resolved
lib/page-encointer/home_page.dart Show resolved Hide resolved
lib/page/assets/transfer/transfer_page.dart Outdated Show resolved Hide resolved
lib/page/profile/account/account_manage_page.dart Outdated Show resolved Hide resolved
test_driver/app.dart Outdated Show resolved Hide resolved
test/store/encointer/encointer_test.dart Outdated Show resolved Hide resolved
lib/app.dart Outdated Show resolved Hide resolved
lib/main.dart Outdated Show resolved Hide resolved
lib/page/assets/receive/receive_page.dart Outdated Show resolved Hide resolved
Copy link
Member

@clangenb clangenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your patience and fixes. Only one question remains. :)

test/store/encointer/encointer_test.dart Show resolved Hide resolved
test_driver/app.dart Show resolved Hide resolved
lib/app.dart Outdated Show resolved Hide resolved
lib/main.dart Outdated Show resolved Hide resolved
Copy link
Member

@clangenb clangenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks for the hard work on for your patience!

Comment on lines +54 to 58
// On test mode instead of LocalStorage() must be use MockLocalStorage()
create: (context) => AppStore(util.LocalStorage()),
child: const WalletApp(Config()),
),
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, I see I did not get that before, this is one of the beauties of using the provider; we don't even need to check the config. :D

Thanks for the comment. It is helpful!

@clangenb clangenb merged commit 81cc728 into master Sep 2, 2022
@Eldar2021 Eldar2021 deleted the e-deleted-global-app-store branch September 5, 2022 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants