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

improvement: use AnkiActivity where possible & refactor dialogs out of AnkiActivity #17511

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

improvement: use AnkiActivity where possible & refactor dialogs out of AnkiActivity #17511

wants to merge 7 commits into from
+731 −248

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Nov 27, 2024 •
edited
Loading

Purpose / Description

Refactor to make CustomStudyDialog more compatible with the Fragment Result API

I then continued on and removed some methods from AnkiActivity which no longer needed to be there due to Kotlin-related improvements

Approach

  • use AnkiActivity where possible

Note: this is a net negative line count when the 517 lines of test code are taken into account

How Has This Been Tested?

CongratsPage & DeckPicker

  • Brief issue on main with limit to tags not working

Learning

Time might be well spent adding automated testing on this functionality, it seems to have atrophied badly

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Nov 27, 2024
@david-allison

This comment was marked as outdated.

Sign in to view
@david-allison david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Nov 29, 2024
@david-allison david-allison changed the title improvement: use AnkiActivity where possible improvement: use AnkiActivity where possible & refactor dialogs out of AnkiActivity Nov 29, 2024
@david-allison david-allison mentioned this pull request Nov 29, 2024
Merged
5 tasks
@david-allison david-allison added the Blocked by dependency Currently blocked by some other dependent / related change label Nov 29, 2024
@david-allison

This comment was marked as resolved.

Sign in to view
@david-allison david-allison removed Blocked by dependency Currently blocked by some other dependent / related change Has Conflicts labels Nov 30, 2024
david-allison
david-allison commented Nov 30, 2024
View reviewed changes
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/CustomStudyDialog.kt
Comment on lines 90 to 91
fun onCreateCustomStudySession()
fun onExtendStudyLimits()
Copy link
Member Author

@david-allison david-allison Nov 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is the main point of the change: we can convert this into a Fragment Result

mikehardy
mikehardy approved these changes Nov 30, 2024
View reviewed changes
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Wow, that's a nicely shaved yak you've got there
It's a lot, but it all seems okay

@mikehardy mikehardy added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Nov 30, 2024
@mikehardy mikehardy added this to the 2.20 Release milestone Nov 30, 2024
BrayanDSO
BrayanDSO reviewed Dec 3, 2024
View reviewed changes
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

Tbh, I think that most of those dialog utils stink so I avoid using them. But the PR is a positive change

AnkiDroid/src/main/java/com/ichi2/anki/utils/ext/FragmentActivity.kt Outdated Show resolved Hide resolved
@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Dec 3, 2024
@david-allison
Copy link
Member Author

Tbh, I think that most of those dialog utils stink so I avoid using them. But the PR is a positive change

Agreed, especially dismissAllDialogFragments. This work is primarily to move towards the Fragment Result API for a number of dialogs

@david-allison
test: introduce AnkiFragmentScenario
3440123 
`CustomStudyDialog` will depend on `AnkiActivity` in a later
commit, and Android test infrastructure does not allow a custom
activity to be used in `FragmentScenario`, so we need our own

Code mostly copied from FragmentScenario, changes in the file header
@david-allison
cleanup: AnkiFragmentScenario
3e40a63 
Code was copied from Android source with minimal modification
to adhere to the Apache license in spirit

This cleans up the code:

* Remove redundant public modifier
* Remove deprecated methods
* suppress unused methods
* use `Closeable by activityScenario`
* Don't redefine `FragmentAction`
* remove `themeResId` and `containerResId`
@david-allison
improvement: use AnkiActivity where possible
5bcd3c9 
The only question here is whether `showDialogFragment`
in CongratsPage is necessary

showDialogFragment in CongratsPage should not be necessary
as the lifetime of the fragment should match the activity
@david-allison
test: remove FragmentTestActivity
4f39792 
Unused
@david-allison
refactor: extract dialog fragments from Anki[Activity/Fragment]
1e164d2 
* dismissAllDialogFragments
* showDialogFragment

**AnkiFragment**
* showDialogFragment

This affects all callers, as we now use extension methods
@david-allison
improvement: getCurrentDialogFragment
43069f4 
This was previously a test method, but also used in production
@david-allison
refactor: 'dismissAllDialogFragments'
6af149c 
Since it's now a method on FragmentActivity,
we no longer need interface casting
@david-allison david-allison removed Needs Author Reply Waiting for a reply from the original author Has Conflicts labels Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikehardy mikehardy mikehardy approved these changes

@BrayanDSO BrayanDSO Awaiting requested review from BrayanDSO

Assignees
No one assigned
Labels
Needs Second Approval Has one approval, one more approval to merge
Projects
None yet
Milestone
2.20 Release
Development

Successfully merging this pull request may close these issues.
3 participants
@david-allison @mikehardy @BrayanDSO