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.

Sign up for GitHub

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

Jump to bottom

Don't use AccountSettings on main thread #1049

Merged
merged 5 commits into from
Oct 3, 2024
Merged

Don't use AccountSettings on main thread #1049

merged 5 commits into from
Oct 3, 2024

Conversation

sunkup
Copy link
Member

@sunkup sunkup commented Sep 30, 2024 •
edited by rfc2822
Loading

Purpose

AccountSettings uses blocking I/O and may run migrations. We don't want to block the main thread like that, which is why this PR will stop creating/using AccountSettings on the main thread.

Short description

  • Document that AccountSettings shouldn't be used in the main thread
  • Don't access AccountSettings on the main thread in
    • AccountSettingsModel: Switch to state flow, since snapshot was complicated and didn't work well for me on the background thread ...
    • AccountScreenModel
    • DebugInfoModel

Resources

https://developer.android.com/topic/architecture/ui-layer/state-production#mutating_the_ui_state_from_background_threads

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@sunkup sunkup linked an issue Sep 30, 2024 that may be closed by this pull request
AccountsSettings is used from main thread #1047
Closed
2 tasks
@sunkup sunkup added the bug Something isn't working label Sep 30, 2024
@sunkup sunkup self-assigned this Sep 30, 2024
@sunkup sunkup force-pushed the 1047-accountssettings-is-used-from-main-thread branch 2 times, most recently from ed2a6d5 to 711c603 Compare October 1, 2024 10:17
@sunkup sunkup requested a review from ArnyminerZ October 1, 2024 10:47
@sunkup sunkup marked this pull request as ready for review October 1, 2024 10:47
@sunkup sunkup requested a review from rfc2822 October 1, 2024 13:18
@sunkup sunkup force-pushed the 1047-accountssettings-is-used-from-main-thread branch 2 times, most recently from 1384232 to e6233a7 Compare October 2, 2024 09:24
@sunkup sunkup requested a review from rfc2822 October 2, 2024 09:41
rfc2822
rfc2822 reviewed Oct 2, 2024
View reviewed changes
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Nice, only #1049 (comment) please as you had it before. Then I'll merge.

@sunkup sunkup force-pushed the 1047-accountssettings-is-used-from-main-thread branch 2 times, most recently from 824ee57 to 24c9609 Compare October 3, 2024 09:33
@sunkup sunkup force-pushed the 1047-accountssettings-is-used-from-main-thread branch from 24c9609 to a3a856e Compare October 3, 2024 09:34
@sunkup sunkup requested review from rfc2822 and removed request for ArnyminerZ October 3, 2024 09:36
@rfc2822 rfc2822 merged commit 196bfb3 into main-ose Oct 3, 2024
7 of 8 checks passed
@rfc2822 rfc2822 deleted the 1047-accountssettings-is-used-from-main-thread branch October 3, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfc2822 rfc2822 rfc2822 approved these changes

Assignees

@sunkup sunkup

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AccountsSettings is used from main thread
2 participants
@sunkup @rfc2822