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

Load User Defaults list asynchronously to improve performance #22535

Merged

Conversation

salimbraksa
Copy link
Contributor

@salimbraksa salimbraksa commented Feb 2, 2024

Related PR

This PR depends on:

Description

There was a short ( but noticeable ) delay when navigating to User Default Debug screen, this was due to running the load method on the main thread. This PR fixes the issue by running it asynchronously.

Before After
CleanShot.2024-02-02.at.19.43.29.mp4
CleanShot.2024-02-02.at.19.44.52.mp4

Test Instructions

  1. Navigate to User Defaults Debug screen
  2. Expect User Defaults loading doesn't freeze the UI
  3. Expect the User Defaults list to load

Regression Notes

  1. Potential unintended areas of impact
    Double check the User Defaults can be loaded.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N/A

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@dangermattic
Copy link
Collaborator

dangermattic commented Feb 2, 2024

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@salimbraksa salimbraksa self-assigned this Feb 2, 2024
@salimbraksa salimbraksa requested a review from justtwago February 2, 2024 18:52
@salimbraksa salimbraksa marked this pull request as ready for review February 2, 2024 18:52
@salimbraksa salimbraksa changed the base branch from trunk to salim/issue/22479-debug-screen-local-flags February 2, 2024 18:53
@@ -16,40 +16,51 @@ final class BooleanUserDefaultsDebugViewModel: ObservableObject {
}
}

@Published var userDefaultsSections: Sections = []
@MainActor @Published private(set) var userDefaultsSections: Sections = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decorating this property with @MainActor means that it must be mutated on the main thread.


init(coreDataStack: CoreDataStack = ContextManager.shared,
init(coreDataStack: CoreDataStackSwift = ContextManager.shared,
Copy link
Contributor Author

@salimbraksa salimbraksa Feb 2, 2024

Choose a reason for hiding this comment

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

The CoreDataStackSwift is the same as CoreDataStack protocol, but with additional async APIs.

Task {
await self.load()
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This load method is for convenience only so that callers don't have to do the Task { } wrapping.

let (key, value) = keyValue
result.append(processSingleUserDefault(key: key, value: value))
private func processGroupedUserDefaults(_ userDefaults: [String: Bool]) async -> [Row] {
let rows = try? await self.coreDataStack.performAndSave { [weak self] context -> [Row] in
Copy link
Contributor Author

@salimbraksa salimbraksa Feb 2, 2024

Choose a reason for hiding this comment

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

When retrieving Core Data objects from a background thread, it's important not to use the mainContext since it's meant for the main thread only. Calling the main context from a background thread could cause the app to crash.

Instead, the method coreDataStack.performAndSave generates a new context and executes out the block on a background thread. This context is then used to fetch Core Data objects.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 2, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr22535-a4e6276
Version24.1
Bundle IDorg.wordpress.alpha
Commita4e6276
App Center BuildWPiOS - One-Offs #8667
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 2, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr22535-a4e6276
Version24.1
Bundle IDcom.jetpack.alpha
Commita4e6276
App Center Buildjetpack-installable-builds #7695
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

…issue/22479-debug-screen-local-flags-async-load
@salimbraksa salimbraksa force-pushed the salim/issue/22479-debug-screen-local-flags-async-load branch from e0523c2 to a4e6276 Compare February 4, 2024 02:18
Base automatically changed from salim/issue/22479-debug-screen-local-flags to trunk February 5, 2024 16:34
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

Copy link
Contributor

@justtwago justtwago 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 for the enhancement, Salim! Tested it and it works as expected - there are no freezes when entering the debug screen.

@justtwago justtwago merged commit cee365c into trunk Feb 5, 2024
23 checks passed
@justtwago justtwago deleted the salim/issue/22479-debug-screen-local-flags-async-load branch February 5, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants