Skip to content

refactor: Move most CardBrowser methods to CardBrowserFragment#18723

Merged
mikehardy merged 21 commits intoankidroid:mainfrom
david-allison:browser-fragment-2
Aug 3, 2025
Merged

refactor: Move most CardBrowser methods to CardBrowserFragment#18723
mikehardy merged 21 commits intoankidroid:mainfrom
david-allison:browser-fragment-2

Conversation

@david-allison
Copy link
Member

@david-allison david-allison commented Jun 30, 2025

When I was looking into

I found that I wanted to:

  1. Move the menu state to a ViewModel
  2. Split the ViewModel into a Fragment-level, and an Activity-level ViewModel
    • When we added the NoteEditor ([GSoC'24] Add NoteEditor to CardBrowser #16764 ), a lot of bugs were introduced due to the complexity
    • The panes are an activity-level concern, and would be easier to focus on with a smaller class
    • Most of the rest of the logic is CardBrowserFragment-based
  3. Introduce the SearchBar/SearchView

These changes have been done on a branch for (1) and (3), but it feels like a 'huge' functional chance that I'll struggle to get reviewed with the Browser in its current state.

This PR moves us closer to (2), and closer to:

  • a separation of concerns, adding in a CardBrowserFragmentViewModel
  • splitting out a PR of (1) - as it would move the menu state into the future CardBrowserFragmentViewModel

Fixes

Approach

  • Go through method by method and move them to the fragment
  • Once a sufficient number of methods were moved, move both onKeyUp and onOptionsItemSelected for these methods into the CardBrowserFragment, removing

How Has This Been Tested?

CI

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 this to the 2.22 release milestone Jun 30, 2025
@david-allison david-allison marked this pull request as draft June 30, 2025 20:09
@david-allison david-allison added Blocked by dependency Currently blocked by some other dependent / related change 2.23 labels Jul 22, 2025
@david-allison david-allison removed Blocked by dependency Currently blocked by some other dependent / related change Has Conflicts labels Jul 28, 2025
@david-allison david-allison marked this pull request as ready for review July 28, 2025 18:05
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Jul 30, 2025
@mikehardy mikehardy modified the milestones: 2.22 release, 2.23 release Jul 30, 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.

Nice movement

@BrayanDSO BrayanDSO added Needs Second Approval Has one approval, one more approval to merge and removed Needs Author Reply Waiting for a reply from the original author Needs Review labels Jul 31, 2025
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.

nice

@mikehardy mikehardy added this pull request to the merge queue Aug 3, 2025
@mikehardy mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Aug 3, 2025
Merged via the queue into ankidroid:main with commit a999321 Aug 3, 2025
10 checks passed
@github-actions 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 Aug 3, 2025
@david-allison david-allison deleted the browser-fragment-2 branch October 16, 2025 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants