Skip to content

Add option to display card ID in card list#3174

Open
mvanhorn wants to merge 1 commit into
CatimaLoyalty:mainfrom
mvanhorn:feat/2742-display-card-id
Open

Add option to display card ID in card list#3174
mvanhorn wants to merge 1 commit into
CatimaLoyalty:mainfrom
mvanhorn:feat/2742-display-card-id

Conversation

@mvanhorn

Copy link
Copy Markdown

Summary

Adds a "Show card ID" option to the card list Display Options menu. When enabled, each card row shows its card ID (barcode value) below the existing fields, using the same render path as the other display options (note, balance, validity). The option is disabled by default.

Why this matters

Resolves #2742. Users currently work around the missing feature by copying the card ID into the note field so it shows in the list. As discussed on the issue, this is a natural second entry in Display Options, rendered with the same logic the rest of the row uses, and kept off by default because the raw ID is somewhat technical.

The implementation follows the existing showNote / showBalance / showValidity pattern verbatim:

  • LoyaltyCardListDisplayOptionsManager: new mShowCardId boolean backed by a sharedpreference_card_details_show_card_id key (default false), with showCardId(boolean) / showingCardId() and a new entry in the Display Options dialog.
  • LoyaltyCardCursorAdapter: binds a new cardId row and renders loyaltyCard.cardId via the existing setExtraField helper when the option is on and the ID is non-empty; hidden otherwise.
  • loyalty_card_layout.xml: new cardId TextView appended below expiry.
  • strings.xml: show_card_id label and the translatable="false" preference key (English only; translations come from Weblate).
  • New ic_card_id_24dp drawable for the row icon.

Testing

Added two Robolectric tests in MainActivityTest:

  • cardIdHiddenByDefault: with the option off (default), the card ID field stays GONE.
  • showCardIdDisplayOption: with the preference enabled before the activity is built, the field renders VISIBLE with the card's ID text.

Full Gradle build/tests were not run locally (no Android SDK/JDK in this environment); the changes mirror the existing display-option pattern and CI runs the unit/lint jobs.

AI disclosure

Per CONTRIBUTING.md: an LLM assisted with writing this change, so I want to be open about it. What was generated was the boilerplate mirroring of the existing showNote / showBalance / showValidity flow (the manager field/getter/setter, the dialog entry, the adapter binding, the layout row, the strings, and the tests). I reviewed every line against the existing pattern to confirm it fits the project's design rather than introducing a new approach: no new libraries, the render reuses the existing setExtraField helper, the preference plumbing matches the other options exactly, and only values/strings.xml gains strings (translations left to Weblate). I also caught and fixed one correctness issue in the first draft of the test, where the preference was set after the adapter had already cached it in the constructor; the test now sets the preference before the activity is built so it actually exercises the visible path. The changes are scoped to this feature with no unrelated edits. I was not able to run the Gradle suite locally (no Android SDK in my environment), so I am relying on CI for the build/lint/unit-test verification and am happy to iterate on any review feedback.

Fixes #2742

@TheLastProject TheLastProject left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems reasonable, just some nitpicks and then I think I can merge this :)

public class LoyaltyCardListItemViewHolder extends RecyclerView.ViewHolder {

public TextView mCardText, mStoreField, mNoteField, mBalanceField, mValidFromField, mExpiryField;
public TextView mCardText, mStoreField, mNoteField, mBalanceField, mValidFromField, mExpiryField, mCardIdField;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the card ID should probably be displayed as one of the first options, as for those who will use this feature it is probably something they want to refer to often ("yeah my customer ID is 123456").

I'm not quite sure if before or after the note makes most sense, but I'd say it is probably easiest and most consistent to put it after the note but before everything else.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this barcode from the Google/Android Studio built in fonts? https://fonts.google.com/icons?selected=Material+Symbols+Outlined:barcode:FILL@0;wght@400;GRAD@0;opsz@24&icon.query=barc&icon.size=24&icon.color=%231f1f1f&icon.platform=android

It makes sense and we can keep this, I'm just checking to make sure for copyright reasons that it's just one of the default icons we can freely use.

Comment on lines 211 to 212
android:textAppearance="?attr/textAppearanceBody2"
app:drawableLeftCompat="@drawable/ic_baseline_access_time_24"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some cards have extremely long IDs, especially pkpass files and the likes have it very commonly.

I think we should put a limit on here. I feel tempted to suggest just a single line because I would logically expect that any card ID that anyone would ever read to a shop employee ("my ID is 12345678") would most likely fit on a single line, as otherwise scanning would be more sensible anyway.

Suggested change
android:textAppearance="?attr/textAppearanceBody2"
android:maxLines="1"
android:ellipsize="end"
app:drawableLeftCompat="@drawable/ic_baseline_access_time_24"

Comment on lines +140 to +141
list.measure(0, 0);
list.layout(0, 0, 100, 1000);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is exactly necessary, it's used in the addFourLoyaltyCardsTwoStarred test because those are four cards and the RecyclerView might otherwise not show them, but I think keeping it is also not necessarily an issue. But might be good to also copy the comment from the other test ( // Make sure there is enough space to render all) over so it's clear this is to ensure the RecyclerView is given enough space

Comment on lines +178 to +179
list.measure(0, 0);
list.layout(0, 0, 100, 1000);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here

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.

[Feature request] Display Options: Show Card ID / Barcode Value

2 participants