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

Enable Dagger graph access for BindableAdapter #2658

Closed
BenHenning opened this issue Feb 8, 2021 · 5 comments · Fixed by #4412
Closed

Enable Dagger graph access for BindableAdapter #2658

BenHenning opened this issue Feb 8, 2021 · 5 comments · Fixed by #4412
Assignees
@KevinGitonga
Labels
Impact: Low Low perceived user impact (e.g. edge cases). Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Member

BenHenning commented Feb 8, 2021 •
edited
Loading

BindableAdapters are created using one of two builders (single type & multi type), each of which has its own newBuilder function:

However, there are two drawbacks to this approach:

  1. It doesn't allow us to easily gain access to Dagger-bound objects like OppiaLogger (which we probably will want to use in the future to add warnings logs when something breaks in the adapter
  2. It requires passing in objects that could be automatically retrieved for better safety, such as the Fragment for setting the LifecycleOwner of the builder

This issue is tracking:

  1. Introducing a Factory class for each of the types of builders & making this injectable (similar to StatePlayerRecyclerViewAssembler
  2. Having each factory inject an AndroidX fragment & automatically set that as the lifecycle owner (requires noting in the factories' documentation that they can only be used in fragment-bound classes)
  3. Updating all places in code that use the builders today to instead inject factories
  4. Removing the old newBuilder functions since they've been functionally replaced with a factory
  5. Removing setLifecycleOwner since it will become an automatic mechanism, instead (which should also subsequently simplify the tests slightly, and lets us remove BaseBuilder in favor of a helper or extension function for retrieving the lifecycle owner
@Sparsh1212
Copy link
Contributor

Sparsh1212 commented Feb 9, 2021 •
edited
Loading

@anandwana001 @BenHenning
I would love to work on this. Also, if possible then please guide me on how to get started with this issue.

@Akshat-gour
Copy link
Contributor

@BenHenning
@anandwana001
If this issue is available. please assign to me.

@anandwana001
Copy link
Contributor

Assigning to @Sparsh1212 as he has asked it earlier.
You can start by creating a dependency - dependent graph, look for what are the dependencies for BindableAdapter and dependent of BindableAdapter. The five points mentioned above are good to start with.

@Sparsh1212
Copy link
Contributor

Assigning to @Sparsh1212 as he has asked it earlier.
You can start by creating a dependency - dependent graph, look for what are the dependencies for BindableAdapter and dependent of BindableAdapter. The five points mentioned above are good to start with.

Okay thankyou. I will start working on this ASAP.

@Sparsh1212
Copy link
Contributor

Sparsh1212 commented Feb 23, 2021 •
edited
Loading

@anandwana001 I tried working hard on this but I am facing some trouble understanding this, so that might take some more time. So, if it is urgent, then you can assign this to any other contributor.

@Sparsh1212 Sparsh1212 removed their assignment Feb 23, 2021
@prayutsu prayutsu self-assigned this Feb 23, 2021
@prayutsu prayutsu mentioned this issue Mar 3, 2021
Closed
8 tasks
@prayutsu prayutsu removed their assignment Jul 22, 2021
@yash10019coder yash10019coder mentioned this issue Jan 27, 2022
Closed
6 tasks
@BenHenning BenHenning added this to the Beta MR2 milestone Jun 11, 2022
@KevinGitonga KevinGitonga mentioned this issue Jul 1, 2022
Merged
6 tasks
KevinGitonga added a commit to KevinGitonga/oppia-android that referenced this issue Jul 12, 2022
@KevinGitonga
Fix some of the failing tests after fixing oppia#2658.
6d72ea9
KevinGitonga added a commit to KevinGitonga/oppia-android that referenced this issue Jul 13, 2022
@KevinGitonga
Fix some of the failing tests after fixing oppia#2658.
e92b113
KevinGitonga added a commit to KevinGitonga/oppia-android that referenced this issue Jul 13, 2022
@KevinGitonga
Add changes to run tests directly by removing unnecessary verificatio…
903012a 
…n on initial click oppia#2658.
KevinGitonga added a commit to KevinGitonga/oppia-android that referenced this issue Jul 13, 2022
@KevinGitonga
Fix more failing tests for oppia#2658.
9e1bec0
KevinGitonga added a commit to KevinGitonga/oppia-android that referenced this issue Jul 18, 2022
@KevinGitonga
More updates as part of refactoring for issue oppia#2658.
20c1ff3
KevinGitonga added a commit to KevinGitonga/oppia-android that referenced this issue Jul 19, 2022
@KevinGitonga
Refactor PromotedStoryListView.kt BindableAdapterTest.kt classes to p…
dfed2cb 
…rovide Adapter through injection for isuue oppia#2658.
@Broppia Broppia added issue_type_infrastructure Impact: Low Low perceived user impact (e.g. edge cases). labels Jul 29, 2022
@BenHenning BenHenning added the Z-ibt Temporary label for Ben to keep track of issues he's triaged. label Sep 16, 2022
@BenHenning BenHenning removed this from the Beta MR2 milestone Sep 16, 2022
Repository owner moved this from Needs Triage to Done in [Team] Developer Workflow & Infrastructure - Android Oct 4, 2022
BenHenning added a commit that referenced this issue Oct 4, 2022
@KevinGitonga @BenHenning
Fix #2658, #299: Replace builder() with Factory in SingleTypeBuilder …
8594c9d 
…and MultiTypeBuilder (#4412)

* Replace builder() with Factory in SingleTypeBuilder and MultiTypeBuilder.

* Fix failing tests after refactor, Create Fragment for DragDropTestActivity.kt.

* Inject the BindableAdapter.kt factory into the fragment and listview instances.

* Inject the BindableAdapter.kt factory into the fragment and listview instances.

* Inject the BindableAdapter.kt factory into the fragment and listview instances.

* Inject the BindableAdapter.kt factory into the fragment and listview instances.

* Add 'androidx.work:work-testing:2.4.0' in androidTestImplementation to fix tests.

* Fix lint formatting issue in updated files.

* Fix some of the failing tests after fixing #2658.

* Fix some of the failing tests after fixing #2658.

* Fix more failing tests for #2658.

* Refactor StatePlayerRecyclerViewAssembler.kt to inject MultiTypeAdapterFactory

* Fix failing tests due to missed refactor of QuestionPlayerFragmentPresenter.kt to inject MultiTypeAdapter Factory.

* More updates as part of refactoring for issue #2658.

* Refactor PromotedStoryListView.kt BindableAdapterTest.kt classes to provide Adapter through injection for isuue #2658.

* Fix issues as advised by code review.

* Fix issue causing some of failing tests.

* Fix issue causing some of failing tests.

* Revert unused resources, to fix Static check test failure.

* Add KDocs to implemented public functions, to fix KDocs Static check test failure.

* Add exemption for test to DragDropTestFragmentPresenter and DragDropTestFragment since they are actual test classes.

* Fix nits and updates as advised during code review.

* Fix nits and updates as advised during code review.

* Fix nits and updates as advised during code review.

* Updates issues as advised on code review, fix some of the failing tests.

* Fix failing tests.

* Fix more failing tests by reverting DragDropSortInteractionView.kt and SelectionInteractionView.kt to initial state.

* Update multiple field namings for consistency as advised by code reviewer.

* Fix failing tests and revert from previous fix, which could not be used in alpha.

* Fix out-of-order binding for selection, drop/drop.

* Post-merge fixes.

Mainly, this fixing previous code as well as image region selection
(which whose breakage could only be detected via manually playing the
image region selection and by running the interaction's corresponding
test suite on Espresso, both of which caught different sets of issues).

The PR includes some cleanup work as well.

* Fix lint issues after resolving merge conflicts on CI and add lessons_chapter_view.xml which is not available on my branch even after several sync's causing build errors.

* Revert deleted deltas.

* Remove lessons_chapter_view.xml since it is no longer required.

Co-authored-by: Ben Henning <[email protected]>
BenHenning added a commit that referenced this issue Oct 10, 2022
@KevinGitonga @BenHenning
Fix #4186: Uncheck all selection on developer options not working (#4383
d1f5309 
)

* Fix #4186 Uncheck all selection on developer options not working

* Fix #4186 Add changes to code as suggested on code review.

* Fix #4186 Undo changes on line wrap as advised by code reviewer.

* Add tests for #4186.

* Add tests for #4186, Undo changes on build.gradle.

* Update and makes changes as advised by code reviewer.

* Update test names to deselect from unselect.

* Fix broken tests after update.

* Move the 'scrollToPosition' call inside of 'verifyItemCheckedOnRecyclerViewItemAtPosition', to reduce boilerplate code.

* Add changes to run tests directly by removing unnecessary verification on initial click #2658.

* Updates tests and fix missed issues supposed to be updated.

* Update tests to assert if Checkbox is actually Unchecked after unchecking.

* Add testCoroutinesDispatchers.runCurrent() line to avoid MarkChaptersCompletedFragmentTest.kt deselectsAllChapters test from failing.

* Fix lint failing test, after update.

* Fix broken view models.

Co-authored-by: Ben Henning <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@KevinGitonga KevinGitonga

Labels
Impact: Low Low perceived user impact (e.g. edge cases). Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Projects
[Team] Developer Workflow & Infrastructure - Android
Archived in project
Milestone
No milestone
7 participants
@BenHenning @KevinGitonga @anandwana001 @prayutsu @Sparsh1212 @Akshat-gour @Broppia