refactor: move most of the Preferences activity into a fragment #17487

BrayanDSO · 2024-11-23T19:08:18Z

Purpose / Description

I want to create a preferences screen for the new reviewer toolbar menu buttons, but the Preferences activity is not flexible enough for what I want to do, so I went on and refactored a good chunk of it to reduce the dependency on Preferences and have a bigger fragment modularity.

Fixes

Approach

In the commits

How Has This Been Tested?

Emulator 34

uno.webm

developers.developers.developers.webm

tablets.webm

highlight.webm

Checklist

Please, go through these checks before submitting the PR.

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

Really solid, thank you!

I don't think any of the comments are blocking

AnkiDroid/src/main/java/com/ichi2/anki/utils/Resources.kt

AnkiDroid/src/test/java/com/ichi2/anki/preferences/PreferenceTestUtils.kt

AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt

david-allison · 2024-11-26T15:38:55Z

@BrayanDSO This is going to conflict with the following, which would be (optionally) nice to get into the 2.19 series

[BUG]: Tablet: Settings highlight does not follow 'back button' #17414
- fix: Settings highlight does not follow 'back button' #17447

Maintainer's choice if we base this PR on top of the above (incurring a slight delay), or defer the above to 2.20 (also acceptable, minor visual bug)

BrayanDSO · 2024-11-26T16:33:29Z

That bug also happens when clicking a search result, and that PR doesn't fix that. I could fix both issues without much hassle in a commit here.

lukstbit

LGTM, squash into two commits and merge at will.

Left a minor point about Up if you want to consider it, but I don't consider it blocking.

lukstbit · 2024-12-03T11:45:58Z

AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt

-        setTransparentStatusBar()
+    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
+        view.findViewById<MaterialToolbar>(R.id.toolbar)
+            .setNavigationOnClickListener { onBackPressedCallback.handleOnBackPressed() }


I haven't flagged this a review request because the previous code has the same behavior, but I think the navigation listener shouldn't behave as the BACK button and it should close the activity:

.setNavigationOnClickListener { requireActivity().finish() }

The Up button should be used to navigate the app screens and not got through all user actions like the BACK button does. It's not something really problematic but I would still go with the platform behavior which the average android user would expect.

Done a change that will lead to the same behavior.

As the toolbar is reused through screens, .setNavigationOnClickListener { requireActivity().finish() } can't be used without more refactoring.

Since I don't want to wait for a re-review, I'll leave the change to a next PR

refactor: set fragment tag in SingleFragmentActivity refactor: set back bar in preferences sw600dp layout statically in the XML refactor: set Preferences toolbar title in parent layout Changing the title from child fragments isn't appropriate refactor: simplify getAllCustomButtonKeys refactor: inline HeaderFragment::setDevOptionsVisibility refactor: replace hasLateralNavigation() refactor: move Preferences companion content to top level refactor: replace MINIMUM_CARDS_DUE_FOR_NOTIFICATION with Android resource refactor: replace Preferences::setDevOptionsEnabled removes DevOptionsFragment's dependency on Preferences # Conflicts: # AnkiDroid/src/main/java/com/ichi2/anki/preferences/DevOptionsFragment.kt refactor: inline restartWithNewDeckPicker Used only once. Also, this refactor removes AdvancedSettingsFragment's dependency on Preferences remove Preferences dependency in AboutFragment Using recreate() forces HeaderFragment to be recreated and update the dev options header visibility The snackbar was replaced with a toast because of the recreation. Since dev options is aimed at developers, the UX difference shouldn't matter refactor: inline Preferences::hasAnkiWebAccount used only once

the previous approach only changes the highlight when tapping a HeaderPreference It doesn't listen to backstack changes done by other things, like the back button and the search bar.

mikehardy · 2024-12-03T16:57:24Z

I will cut the release-2.20 branch just prior to this merge commit just in case, hopefully no offense @BrayanDSO

But! I'm mostly commenting to say that I think we can also flip on the new reviewer by default in a PR as well? line it up for 2.21, get that thing out there

BrayanDSO · 2024-12-03T18:22:17Z

hopefully no offense

None taken.

we can also flip on the new reviewer by default in a PR as well?

Still a bit far from done.

github-actions · 2025-01-02T12:26:58Z

Hi there @BrayanDSO! This is the OpenCollective Notice for PRs merged from 2024-12-01 through 2024-12-31

If you are interested in compensation for this work, the process with details is here:

https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

Important

PLEASE NOTE: The process was updated in August 2024. Re-read the Payment Process page if you have not already.

We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month

Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.

Thanks!

BrayanDSO changed the title ~~Prefs refactor~~ refactor: move most of the Preferences activity into a fragment Nov 23, 2024

BrayanDSO marked this pull request as draft November 23, 2024 19:10

BrayanDSO force-pushed the prefs-refactor branch 5 times, most recently from 259df21 to d4c9fd9 Compare November 23, 2024 21:04

BrayanDSO added squash-merge The pull request currently requires maintainers to "Squash Merge" Needs Review labels Nov 23, 2024

BrayanDSO marked this pull request as ready for review November 23, 2024 21:11

BrayanDSO removed the squash-merge The pull request currently requires maintainers to "Squash Merge" label Nov 23, 2024

BrayanDSO force-pushed the prefs-refactor branch 2 times, most recently from 8bb5907 to f42fa90 Compare November 23, 2024 21:55

BrayanDSO changed the title ~~refactor: move most of the Preferences activity into a fragment~~ refactor: a lot of cleanups around Preferences Nov 23, 2024

BrayanDSO changed the title ~~refactor: a lot of cleanups around Preferences~~ refactor: move most of the Preferences activity into a fragment Nov 23, 2024

BrayanDSO force-pushed the prefs-refactor branch 2 times, most recently from cbb557b to c015a66 Compare November 23, 2024 22:24

BrayanDSO added the squash-merge The pull request currently requires maintainers to "Squash Merge" label Nov 23, 2024

BrayanDSO force-pushed the prefs-refactor branch from c015a66 to 5b0d123 Compare November 25, 2024 15:31

david-allison approved these changes Nov 26, 2024

View reviewed changes

david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Nov 26, 2024

BrayanDSO force-pushed the prefs-refactor branch 3 times, most recently from 5b0ae77 to d3c3e62 Compare November 26, 2024 12:07

This comment was marked as resolved.

Sign in to view

david-allison mentioned this pull request Nov 26, 2024

fix: Settings highlight does not follow 'back button' #17447

Closed

5 tasks

BrayanDSO force-pushed the prefs-refactor branch from d3c3e62 to 631a139 Compare November 26, 2024 16:37

BrayanDSO force-pushed the prefs-refactor branch from 3099d89 to 3ec71f9 Compare November 27, 2024 09:58

BrayanDSO removed the squash-merge The pull request currently requires maintainers to "Squash Merge" label Nov 27, 2024

BrayanDSO force-pushed the prefs-refactor branch 2 times, most recently from 166cfbd to 4261190 Compare November 27, 2024 11:16

github-actions bot added the Has Conflicts label Nov 29, 2024

BrayanDSO force-pushed the prefs-refactor branch from 4261190 to f7ba260 Compare December 2, 2024 00:58

BrayanDSO removed the Has Conflicts label Dec 2, 2024

lukstbit approved these changes Dec 3, 2024

View reviewed changes

BrayanDSO force-pushed the prefs-refactor branch from f7ba260 to 2649375 Compare December 3, 2024 15:37

BrayanDSO added 2 commits December 3, 2024 12:37

fix: highlight header in tablets

2649375

the previous approach only changes the highlight when tapping a HeaderPreference It doesn't listen to backstack changes done by other things, like the back button and the search bar.

BrayanDSO removed Needs Author Reply Waiting for a reply from the original author squash-merge The pull request currently requires maintainers to "Squash Merge" labels Dec 3, 2024

BrayanDSO enabled auto-merge December 3, 2024 15:40

BrayanDSO disabled auto-merge December 3, 2024 15:42

BrayanDSO enabled auto-merge December 3, 2024 15:46

BrayanDSO added this pull request to the merge queue Dec 3, 2024

Merged via the queue into ankidroid:main with commit a18eae6 Dec 3, 2024
9 checks passed

github-actions bot added this to the 2.20 Release milestone Dec 3, 2024

github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Dec 3, 2024

BrayanDSO deleted the prefs-refactor branch December 3, 2024 16:02

BrayanDSO mentioned this pull request Feb 7, 2025

Regression: Settings status bar color #17931

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move most of the Preferences activity into a fragment #17487

refactor: move most of the Preferences activity into a fragment #17487

BrayanDSO commented Nov 23, 2024 •

edited

Loading

david-allison left a comment

This comment was marked as resolved.

david-allison commented Nov 26, 2024

BrayanDSO commented Nov 26, 2024 •

edited

Loading

lukstbit left a comment

lukstbit Dec 3, 2024

BrayanDSO Dec 3, 2024 •

edited

Loading

mikehardy commented Dec 3, 2024

BrayanDSO commented Dec 3, 2024

github-actions bot commented Jan 2, 2025

refactor: move most of the Preferences activity into a fragment #17487

refactor: move most of the Preferences activity into a fragment #17487

Conversation

BrayanDSO commented Nov 23, 2024 • edited Loading

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

david-allison left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

david-allison commented Nov 26, 2024

BrayanDSO commented Nov 26, 2024 • edited Loading

lukstbit left a comment

Choose a reason for hiding this comment

lukstbit Dec 3, 2024

Choose a reason for hiding this comment

BrayanDSO Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

mikehardy commented Dec 3, 2024

BrayanDSO commented Dec 3, 2024

github-actions bot commented Jan 2, 2025

BrayanDSO commented Nov 23, 2024 •

edited

Loading

BrayanDSO commented Nov 26, 2024 •

edited

Loading

BrayanDSO Dec 3, 2024 •

edited

Loading