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

Mode state management issue in the main screen #83

Closed
rj-since-2000 opened this issue Mar 12, 2021 · 2 comments
Closed

Mode state management issue in the main screen #83

rj-since-2000 opened this issue Mar 12, 2021 · 2 comments
Assignees

Comments

@rj-since-2000
Copy link
Collaborator

The consumer of Mode is at the root of the Scaffold tree in the main_screen.dart which causes the entire tree to rebuild on switching from dark to light mode and vice versa, which is unnecessary and causes a bad user experience. Mode change should only affect the UI and not the business logic!

@harchani-ritik I propose to resolve this issue with provider<Mode> instead of consumer<Mode> so that only the UI is effected on switching the current mode and fix some other small state management issues too in the same screen.

@rj-since-2000
Copy link
Collaborator Author

rj-since-2000 commented Mar 29, 2021

@harchani-ritik Probably yes because the switch is actually present in the main screen, so we only need to refresh the main screen and the torrents list on toggle switch. Rest of the widgets can use Provider when they come in focus.

Actually after more research, I realised why do we even need a Consumer in the main screen. Let's try to analyse -

There are three providers - Api, GeneralFeatures and Mode

*(Api does not even extend ChangeNotifierProvider)
Api is used in -

'api.username'
Navigator.pushReplacement(context, MaterialPageRoute(builder: (context) => MainScreen())); is already calling the MainScreen to refresh on switching between accounts in the drawer. Thus it doesn't need a consumer to update in the UI.

'_getAccountsList(api, modeTheme, general)' in Drawer
Same as above

if (_currentIndex == 0) { showModalBottomSheet( isScrollControlled: true, context: context, builder: (BuildContext bc) { return AddBottomSheet( api: api, apiRequest: (url) { ApiRequests.addTorrent(api, url); }, dialogHint: 'Enter Torrent Url', ); }, ); } else { showModalBottomSheet( isScrollControlled: true, context: context, builder: (BuildContext bc) { return AddBottomSheet( apiRequest: (url) async { await ApiRequests.addRSS(api, url); setState(() {}); }, dialogHint: 'Enter Rss Url', ); }, ); }
Same as above

Now the GeneralFeatures -

It is used inside Drawer

'_getAccountsList(api, modeTheme, general)'
This just needs to pass provider of GeneralFeatures, so instead we can just pass provider. No need of Consumer.

'general.filterTileList' and 'general.listOfLabels'
These functions return data which isn't part of a Stream or any automatic change. Provider will work here.

'general.pageController'
This returns a controller defined in GeneralFeatures so it does not need a Consumer.

'general.pageController.jumpToPage(index)'
Same as above

Only the HomePage needs to be inside Consumer of GeneralFeatures because it has TorrentLists which is already inside a Consumer.
The DownloadsPage only needs homeDirectory to get the list of files. No need of Consumer.
RSSFeeds is already inside a Consumer it needs.
RSSFilterDetails just uses - rssFilters = await ApiRequests.getRSSFilters(Provider.of<Api>(context, listen: false)); so it also doesn't needs a Consumer in the tree root.

I tried debugging using a simple print('widget tree updated') inside the root widget of the main screen and with two providers(Mode and GeneralFeatures) and one Api class inside as it is now in the codebase, the print was being executed multiple times per second which is highly inefficient.

FUN FACT - I tried removing all the three Consumer in the root of the MainScreen and the app was working all fine with all updates in the torrent list, account change, account deletion, list sort, mode change, etc.

@harchani-ritik
Copy link
Collaborator

harchani-ritik commented Mar 29, 2021

Nice @rj-since-2000👏🏻. So you can replace all "consumer" usages with "Provider" wherever necessary and properly test all the functionalities once from your side too before pushing the changes.

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 a pull request may close this issue.

3 participants