-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Refactor: Replace manual reloadRequired flag with OpChanges in CardBrowser #20222
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
Refactor: Replace manual reloadRequired flag with OpChanges in CardBrowser #20222
Conversation
…loadRequired intent signaling
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!! I have a few questions
A unit test here would be extremely useful
| // Set a simple RESULT_OK. The Reviewer or calling activity will now | ||
| // rely on ChangeManager/OpChanges to detect if a refresh is needed. | ||
| setResult(RESULT_OK) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem relevant, should it be in another commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here this change is actually the core part of the refactor. the previous code was explicitly putting the reloadRequired boolean into the result Intent. Since,, that variable has been removed in favor of OpChanges, keeping the Intent().apply { ... } block would result in a compiler error or a redundant empty Intent.for that i simplified it to setResult(RESULT_OK) to complete the removal of manual signaling for this activity's result.
| private var onEditCardActivityResult = | ||
| registerForActivityResult(StartActivityForResult()) { result: ActivityResult -> | ||
| Timber.i("onEditCardActivityResult: resultCode=%d", result.resultCode) | ||
| if (result.resultCode == DeckPicker.RESULT_DB_ERROR) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
I agree that a unit test would be valuable here. here i am looking at how to mock OpChanges to verify that opExecuted triggers the UI refresh. if you have a specific existing test in the codebase you'd recommend as a reference for ChangeManager subscribers, i'd be happy to implement a similar test for this change. thank you :) |
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't worry for now. First contribution, let's get it in.
Thanks so much!!
|
@david-allison updated the code to rename the refresh method and restored the error handling checks. |
lukstbit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks and welcome!
Purpose / Description
This PR refactors
CardBrowser.ktto remove the redundantreloadRequiredboolean flag. The goal is to move away from manual intent-based signaling between activities and instead utilize the centralizedOpChangessystem viaChangeManager. This modernizes the codebase and ensures that UI refreshes are driven by actual database events.Fixes
CardBrowser.ktregardingOpChangesmigration.Approach
private var reloadRequiredlocal variable.setResultcalls inonCreate,onCardsUpdated, and Activity Result listeners that were manually passingRELOAD_REQUIRED_EXTRA_KEY.opExecutedto listen forOpChanges(browserSidebar, browserTable, noteText, card, and collection).ChangeManagersubscription rather than an explicit signal from the Browser.How Has This Been Tested?
Manual Testing on Physical Device:
Configuration:
Learning
Researched the AnkiDroid
ChangeManagerarchitecture to understand howOpChangesbroadcasts database modifications globally. This pattern reduces coupling between activities by using the observer pattern.Checklist