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

Enhancement: Card Browser two taps to change column #17920

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

Enhancement: Card Browser two taps to change column #17920

wants to merge 4 commits into from
+230 −10

Conversation

Siddheshjondhale
Copy link
Contributor

Purpose / Description

Added dialog on single-click of column name to show available columns for change, as mentioned in the issue.

Fixes

Approach

  • Added a single-tap listener to each column heading (TextView inside browser_column_headings) to open a dialog.
  • Dialog now displays:
    • The title with the name of the currently selected column.
    • A list of available columns (those not currently in use).
  • On column selection:
    • Calls updateActiveColumns() from CardBrowserViewModel.kt to apply the change without reloading all columns.

How Has This Been Tested?

Tested by long-pressing the column heading to open the manage columns dialog and single-clicking to change columns, both functioning as expected. Physical Device: (Samsung Galaxy A6+)

Video:

WhatsApp.Video.2025-02-06.at.00.08.25_f20a952f.mp4

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.
  • UI Changes: You have tested your change using the Google Accessibility Scanner.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 5, 2025

Important

Maintainers: This PR contains Strings changes

  1. Sync Translations before merging this PR and wait for the action to complete
  2. Review and merge the auto-generated PR in order to sync all user-submitted translations
  3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

BrayanDSO
BrayanDSO reviewed Feb 5, 2025
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.

Particularly, I prefer the previews of the manage columns dialog because the previewed value is on a separate line

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Feb 5, 2025
@david-allison
Copy link
Member

Particularly, I prefer the previews of the manage columns dialog because the previewed value is on a separate line

Agreed

@Siddheshjondhale
Copy link
Contributor Author

Particularly, I prefer the previews of the manage columns dialog because the previewed value is on a separate line

okay, I will do the changes

@david-allison
Copy link
Member

Note: strongly prefer to rebase onto main rather than including a merge commit

We don't merge PRs with merge commits, so this would need to be squahsed

@Siddheshjondhale Siddheshjondhale force-pushed the enhancement/card-browser-two-taps-to-change-column branch 2 times, most recently from 3061d3c to 4b0012a Compare February 12, 2025 19:53
@Siddheshjondhale Siddheshjondhale marked this pull request as draft February 12, 2025 20:21
@Siddheshjondhale Siddheshjondhale changed the title Enhancement: Card Browser two taps to change column WIP: Enhancement: Card Browser two taps to change column Feb 12, 2025
@Siddheshjondhale Siddheshjondhale force-pushed the enhancement/card-browser-two-taps-to-change-column branch 2 times, most recently from bd70465 to 45afce7 Compare February 12, 2025 20:47
@Siddheshjondhale Siddheshjondhale force-pushed the enhancement/card-browser-two-taps-to-change-column branch from 45afce7 to b64ce1b Compare February 12, 2025 20:52
@Siddheshjondhale
Enhancement: Added two-tap column change in Card Browser
c09057a 
- Single tap selects the column, triggering a dialog.
- Dialog displays available columns along with a preview of their data.
- Selecting a column from the dialog instantly updates it without reloading all columns.
@Siddheshjondhale Siddheshjondhale changed the title WIP: Enhancement: Card Browser two taps to change column Enhancement: Card Browser two taps to change column Feb 12, 2025
@Siddheshjondhale Siddheshjondhale marked this pull request as ready for review February 12, 2025 22:10
@david-allison david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author Has Conflicts labels Feb 12, 2025
@david-allison david-allison self-requested a review February 12, 2025 22:16
david-allison
david-allison requested changes Feb 12, 2025
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Cheers!

  • There is no longer a ripple effect on the columns
  • The title of the dialog needs a little more thought, just 'Sort Field' on its own isn't useful enough to a user

AnkiDroid/src/main/java/com/ichi2/anki/browser/ColumnSelectionDialogFragment.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/browser/ColumnSelectionDialogFragment.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/browser/ColumnSelectionDialogFragment.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/browser/ColumnSelectionDialogFragment.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/browser/ColumnSelectionDialogFragment.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserViewModel.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserViewModel.kt Outdated Show resolved Hide resolved
@Siddheshjondhale @david-allison
Enhancement: Added two-tap column change in Card Browser
ae7a20b 
- Moved to a dedicated XML layout file.
- Replaced LiveData with Flow to simplify state management.
- Used a compat method for improved cross-version support.
- Updated dialog title to display 'Column: Sort Field' for better user clarity.

Co-authored-by: David Allison <davidallisongithub@gmail.com>
@Siddheshjondhale
Copy link
Contributor Author

Cheers!

  • There is no longer a ripple effect on the columns
  • The title of the dialog needs a little more thought, just 'Sort Field' on its own isn't useful enough to a user

Heyo @david-allison,
About the dialog title feedback I have updated it to Column: ColumnName. Let me know if this works or if there's something that would fit better!

david-allison
david-allison requested changes Feb 14, 2025
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

I was pinged on Discord about this, but it doesn't seem like this is ready for re-review.

A number of my comments don't seem resolved/replied to

AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/browser/ColumnSelectionDialogFragment.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/browser/ColumnSelectionDialogFragment.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/browser/ColumnSelectionDialogFragment.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserViewModel.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserViewModel.kt Outdated Show resolved Hide resolved
@Siddheshjondhale Siddheshjondhale force-pushed the enhancement/card-browser-two-taps-to-change-column branch from 6f7ce1d to 246ac6d Compare February 15, 2025 22:40
@Siddheshjondhale
Enhancement:Added two-tap column change in Card Browser
3208589 
 - Used bundleOf instead of Bundle().apply for cleaner argument handling.
- Made dialog title translatable for localization support.
- Reverted Flow usage in fetchAvailableColumns as it was unnecessary.
@Siddheshjondhale Siddheshjondhale force-pushed the enhancement/card-browser-two-taps-to-change-column branch from 246ac6d to 3208589 Compare February 15, 2025 22:59
@david-allison
Copy link
Member

@Siddheshjondhale When you get a chance, could you mark the comments which are resolved as 'resolved'? I'll get around to another review once this is done

@Siddheshjondhale
Enhancement: Added two-tap column change in Card Browser
7294d68 
- Reverted ColumnWithSample to ColumnHeading.
- Updated ColumnHeading data class to include ankiColumnKey.
@Siddheshjondhale
Copy link
Contributor Author

Siddheshjondhale commented Feb 17, 2025 •
edited
Loading

I have marked the comments as resolved that were resolved in the commits. I have also reverted the ColumnWithSample back to ColumnHeading. The changes are working as expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-allison david-allison david-allison requested changes

@BrayanDSO BrayanDSO BrayanDSO left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Needs Review Strings
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[UX Regression]: Card Browser - two taps to change column
3 participants
@Siddheshjondhale @david-allison @BrayanDSO