Skip to content

refactor(card-browser): Convert to fragment (and fix a few issues)#18463

Merged
BrayanDSO merged 11 commits intoankidroid:mainfrom
david-allison:browser-fragment
Jun 10, 2025
Merged

refactor(card-browser): Convert to fragment (and fix a few issues)#18463
BrayanDSO merged 11 commits intoankidroid:mainfrom
david-allison:browser-fragment

Conversation

@david-allison
Copy link
Member

Purpose / Description

Requested by @BrayanDSO

Fixes

Approach

See commits:

  1. Create fragment and move UI elements there
  2. define accessors for UI elements via cardBrowserFragment
  3. Go through each usage of accessors and move inside fragment
  4. finally, remove the cardBrowserFragment, it became test only

  • fix: maintain selection after selecting flag
  • fix: don't show focused row if non-fragmented

How Has This Been Tested?

Learning (optional, can help others)

Easier than expected. There's more rounds if we want to tackle the menu

I feel I did something wrong....

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

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Jun 9, 2025
@david-allison

This comment was marked as resolved.

@david-allison david-allison removed the Needs Author Reply Waiting for a reply from the original author label Jun 10, 2025
@david-allison david-allison requested a review from BrayanDSO June 10, 2025 09:10
@david-allison david-allison force-pushed the browser-fragment branch 3 times, most recently from 8741967 to a0fd651 Compare June 10, 2025 13:05
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.

LGTM and I'd prefer to get this in ASAP so it receives quickest wide testing as possible

One thought regarding possible memory leak - I'll trust your analysis and any possible changes

setDrawable(ContextCompat.getDrawable(requireContext(), R.drawable.browser_divider)!!)
cardsListView.addItemDecoration(this)
}
cardsAdapter =
Copy link
Member

Choose a reason for hiding this comment

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

interesting - while doing the review on #18467 I saw a lot of chatter on memory leaks related to the combination of Fragment + RecyclerView + Adapter, and it looks like this may suffer from the leak described

https://stackoverflow.com/a/46957469/9910298

the adapter will continue to live on in the fragment after the RecyclerView goes away, which will leak perhaps?

during the fragment lifecycle (ie. in onDestroyView) the adapter may be set to null and cleared out of the recyclerview to allow it to GC

there's an interaction with the var declaration as lateinit vs nullable though - and maybe I'm mis-reading it and this doesn't apply ? fine with being wrong and this being comment being closeable

Copy link
Member Author

Choose a reason for hiding this comment

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

Not enough motivation to deep dive today. May not have been a problem due to the lifecycles of the activity and fragment being equal.

Fixed:

    override fun onDestroyView() {
        super.onDestroyView()
        if (::cardsListView.isInitialized) {
            cardsListView.adapter = null
        }
    }

@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Review labels Jun 10, 2025
@mikehardy mikehardy added this to the 2.21 release milestone Jun 10, 2025
@david-allison david-allison added Review High Priority Request for high priority review and removed Needs Author Reply Waiting for a reply from the original author labels Jun 10, 2025
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.

Mike's comment was addressed, and we can keep looking if it causes other leaks after the merge

@BrayanDSO BrayanDSO added this pull request to the merge queue Jun 10, 2025
Merged via the queue into ankidroid:main with commit 03ec0ec Jun 10, 2025
9 checks passed
@github-actions github-actions bot removed Review High Priority Request for high priority review Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) labels Jun 10, 2025
@david-allison david-allison deleted the browser-fragment branch June 11, 2025 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate the Browser into a Fragment

3 participants