Arcade Bonus Mode Implementation#216
Open
amorris13 wants to merge 5 commits into
Open
Conversation
- Implemented 'Bonus' arcade style where triples grant extra time. - Updated Timer to allow time modifications. - Modified ArcadeGame to handle bonus logic and deck reshuffling. - Updated DBAdapter (v9) and ProtoBuf definitions for persistence and cloud sync. - Redesigned MainActivity UI to offer Fixed and Bonus arcade options. - Enhanced Statistics with style filtering. - Added ArcadeStatisticsTest (debugging filtering logic). Stuck on: ArcadeStatisticsTest is currently failing (returning 0 games instead of 2). I attempted to refactor the filtering logic in Statistics.java multiple times to ensure compatibility with Iterables and List types, but the filter still seems to exclude the games in the test environment. Co-authored-by: amorris13 <4523811+amorris13@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Arcade "Bonus" style and wires it through UI, game logic, persistence, stats, analytics/achievements/leaderboard, timer API, and tests; includes DB/proto/cloud schema updates and selector UI for filtering by style. Changes
Sequence DiagramsequenceDiagram
actor Player
participant MainActivity
participant Application
participant ArcadeGame
participant Timer
Player->>MainActivity: Tap "Bonus" button
MainActivity->>Application: startNewArcadeGame(BONUS)
Application->>ArcadeGame: createFromSeed(seed, BONUS)
ArcadeGame->>ArcadeGame: init with BONUS style
Player->>ArcadeGame: find triple
ArcadeGame->>Timer: modifyTime(+bonusMs)
Timer->>ArcadeGame: dispatchTimerTick (updated elapsed)
ArcadeGame->>ArcadeGame: reshuffle deck after re-adding cards
ArcadeGame->>Application: log analytics (game_type + "_bonus")
ArcadeGame-->>Player: UI updates time and board
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/antsapps/triples/backend/Application.java`:
- Around line 233-239: The two-arg overload getArcadeStatistics(Period period,
boolean includeHinted) was changed to default to ArcadeStyle.FIXED and thus
drops Bonus runs; revert its behavior so it remains unfiltered by style: change
getArcadeStatistics(period, includeHinted) to call the original/3-arg
ArcadeStatistics constructor (or otherwise pass a sentinel that means “all
styles”) instead of forwarding ArcadeGame.ArcadeStyle.FIXED, and/or update
callers to pass an explicit style; adjust getArcadeStatistics(...) usages
accordingly so legacy callers continue to see all completed Arcade games (refer
to getArcadeStatistics overloads and the ArcadeStatistics constructor).
In `@app/src/main/java/com/antsapps/triples/backend/DBAdapter.java`:
- Around line 63-65: The Classic table creation string CREATE_CLASSIC_GAMES
incorrectly appends COLUMN_ARCADE_STYLE; remove the concatenation of
COLUMN_ARCADE_STYLE from the CREATE_CLASSIC_GAMES definition so the classic
schema does not include the ARCADE-only column, leaving COLUMN_ARCADE_STYLE
defined only in the arcade table creation (e.g., CREATE_ARCADE_GAMES); also
search for any code that assumes arcade_style exists on classic rows and update
migrations or versioning logic if needed to preserve compatibility.
- Around line 244-254: Reformat the transaction block in DBAdapter to satisfy
Spotless: ensure consistent indentation, spaces, and line breaks around the if
(oldVersion < 9) block and its try/catch/finally so it matches project style
(e.g., opening brace placement, one statement per line inside try, proper
indentation for addColumnIfMissing call and its parameters). Locate the block
containing addColumnIfMissing, TABLE_ARCADE_GAMES, and COLUMN_ARCADE_STYLE and
reflow it to the repository's Java formatting conventions so spotlessJavaCheck
passes.
In `@app/src/main/java/com/antsapps/triples/backend/Statistics.java`:
- Around line 13-20: The base Statistics constructor calls the overridable
shouldInclude(g) while subclasses (e.g., ArcadeStatistics) have not yet
initialized fields like mStyle; change Statistics(Iterable<? extends Game>
iterable, Period period, boolean includeHinted) to stop invoking shouldInclude
during construction: collect games without calling shouldInclude, assign
mGamesInPeriod to period.filter(collectedGames) only for the raw includeHinted
filter, and either (a) add a new constructor parameter or setter to accept an
explicit Predicate<Game>/filter from subclasses (so ArcadeStatistics can pass a
style-aware filter), or (b) provide a protected postInitFilter() method the
subclass calls after it sets mStyle which applies shouldInclude and updates
mGamesInPeriod; update ArcadeStatistics to call that post-init filtering after
mStyle is assigned and remove reliance on base ctor calling shouldInclude.
In `@app/src/main/java/com/antsapps/triples/MainActivity.java`:
- Around line 85-92: The Spotless Java formatting failures are due to the lambda
wrapping and line layout around mArcadeDropdownButton.setOnClickListener;
reformat the block to match project style (consistent indentation, line breaks,
and wrapping) by running the project's formatter (Spotless) or IDE formatter and
ensuring the lambda passed to showSplitMenu is formatted the same way as the
other occurrences (see showSplitMenu(...) usage and startNewArcadeGame(...)
invocation and the Iterables.getFirst(mApplication.getCurrentArcadeGames(),
null) expression); apply the same reformatting to the similar block at lines
239-251 so spotlessJavaCheck passes.
- Around line 171-174: Replace the hardcoded "Bonus"/"Fixed" passed to getString
in MainActivity (the resume label assembly where
getString(R.string.resume_game_arcade_format, state.triplesFound, ...)) with a
resource-backed string determined from the ArcadeGame.ArcadeStyle enum; add or
reuse existing string resources (e.g., R.string.arcade_style_bonus and
R.string.arcade_style_fixed or the existing button labels) and map state.style
to the appropriate getString(...) result so the formatted resume label uses
localized resources instead of hardcoded English.
In `@app/src/main/java/com/antsapps/triples/MainViewModel.java`:
- Around line 103-105: The constructor invocation for ArcadeResumeState passed
into mArcadeResumeState.setValue is misformatted and failing spotlessJavaCheck;
reformat the wrapped call to match the project's Spotless Java styling (run the
Spotless formatter or apply the project's formatting rules) so the two branches
use the same line-wrapping/indentation style for new ArcadeResumeState(true,
arcadeGame.getNumTriplesFound(), arcadeGame.getStyle()) and new
ArcadeResumeState(false, 0, null) and then re-run spotless checks.
In `@app/src/main/java/com/antsapps/triples/stats/ArcadeStatisticsFragment.java`:
- Around line 60-63: The fragment currently sets the arcade style in
onArcadeStyleChange(ArcadeGame.ArcadeStyle) but getLeaderboardId() still always
returns R.string.leaderboard_arcade_game; update getLeaderboardId() to switch on
mViewModel.getStyle() (or its live value) and return the correct leaderboard
resource for Fixed vs Bonus, and if the Bonus leaderboard ID/resource is not
available yet, make the leaderboard action hidden/disabled instead of returning
the Fixed ID; apply the same branching fix to the other location/method
mentioned (the code around the second use at lines 108-109) so both leaderboard
entry points respect the selected style.
In `@app/src/main/java/com/antsapps/triples/stats/ArcadeStatisticsViewModel.java`:
- Around line 27-31: Replace the seed for mStyle to use the most recently played
arcade run rather than only completed runs: change the Iterables.getLast(...)
call to pull from the collection of all/latest arcade games (e.g., use
application.getArcadeGames() or the API that returns runs including in‑progress
ones) instead of application.getCompletedArcadeGames(), then keep the same null
check and mStyle.setValue(lastPlayed.getStyle()) behavior so it falls back
safely if no runs exist; update the call site where Iterables.getLast and the
lastPlayed local are created in ArcadeStatisticsViewModel.
In `@app/src/main/java/com/antsapps/triples/stats/StatisticsSelectorView.java`:
- Around line 177-180: The label is hardcoded; instead derive it from the
selected chip or a string resource: when mStyleChipGroup is visible, get the
currently checked chip's text (use mStyleChipGroup.getCheckedChipId() then
findViewById<Chip>(id).getText()) and pass that text into addSummaryChip rather
than using the literal "Bonus"/"Fixed"; if the checked chip is null, fall back
to a localized string resource keyed to ArcadeGame.ArcadeStyle (use mStyle to
map to getString(...) fallback) to ensure localization and
single-source-of-truth for the label.
In `@app/src/main/res/layout/activity_main.xml`:
- Around line 135-136: Replace hardcoded android:text attributes in
activity_main.xml with string resources: add entries to res/values/strings.xml
(e.g., <string name="button_fixed">Fixed</string> and a name for the other
hardcoded label) and update the buttons in activity_main.xml to use
android:text="@string/button_fixed" and android:text="@string/your_other_name";
ensure you update both occurrences that currently use literal text so the UI is
localizable.
In `@app/src/main/res/layout/stats_selector.xml`:
- Around line 108-142: Replace hardcoded text in the stats_selector layout with
string resources: change the TextView with id style_label to use
android:text="@string/style_label", the Chip with id chip_style_fixed to use
android:text="@string/style_fixed", and the Chip with id chip_style_bonus to use
android:text="@string/style_bonus"; then add matching entries (style_label,
style_fixed, style_bonus) to strings.xml with the current values "STYLE",
"Fixed", and "Bonus" (and translated variants as needed).
In `@app/src/test/java/com/antsapps/triples/backend/ArcadeStatisticsTest.java`:
- Around line 41-46: Remove the temporary System.out.println calls around the
test instantiations of ArcadeStatistics (remove the two lines that call
System.out.println before asserting/getting getNumGames), leaving the assertions
and constructions for ArcadeStatistics(games, Period.ALL_TIME, true,
ArcadeGame.ArcadeStyle.FIXED) and ArcadeStatistics(games, Period.ALL_TIME, true,
ArcadeGame.ArcadeStyle.BONUS) intact; then run Spotless (spotlessJavaApply) to
reformat the long constructor lines and ensure the file passes
spotlessJavaCheck.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a0d361bc-c242-472b-990a-773f28c9021b
📒 Files selected for processing (18)
app/src/main/java/com/antsapps/triples/MainActivity.javaapp/src/main/java/com/antsapps/triples/MainViewModel.javaapp/src/main/java/com/antsapps/triples/backend/Application.javaapp/src/main/java/com/antsapps/triples/backend/ArcadeGame.javaapp/src/main/java/com/antsapps/triples/backend/ArcadeStatistics.javaapp/src/main/java/com/antsapps/triples/backend/CloudSaveSerializer.javaapp/src/main/java/com/antsapps/triples/backend/DBAdapter.javaapp/src/main/java/com/antsapps/triples/backend/Statistics.javaapp/src/main/java/com/antsapps/triples/backend/Timer.javaapp/src/main/java/com/antsapps/triples/stats/ArcadeStatisticsFragment.javaapp/src/main/java/com/antsapps/triples/stats/ArcadeStatisticsViewModel.javaapp/src/main/java/com/antsapps/triples/stats/StatisticsSelectorView.javaapp/src/main/proto/game_data.protoapp/src/main/res/layout/activity_main.xmlapp/src/main/res/layout/stats_selector.xmlapp/src/main/res/values/strings.xmlapp/src/test/java/com/antsapps/triples/NavigationTest.javaapp/src/test/java/com/antsapps/triples/backend/ArcadeStatisticsTest.java
Comment on lines
+27
to
+31
| ArcadeGame lastPlayed = | ||
| com.google.common.collect.Iterables.getLast(application.getCompletedArcadeGames(), null); | ||
| if (lastPlayed != null) { | ||
| mStyle.setValue(lastPlayed.getStyle()); | ||
| } |
There was a problem hiding this comment.
Default this from the latest Arcade game, not only the latest completed one.
The requirement says the selector should default to the most recently played style. Seeding it from getCompletedArcadeGames() falls back to an older style whenever the user's newest Arcade run is still in progress.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/antsapps/triples/stats/ArcadeStatisticsViewModel.java`
around lines 27 - 31, Replace the seed for mStyle to use the most recently
played arcade run rather than only completed runs: change the
Iterables.getLast(...) call to pull from the collection of all/latest arcade
games (e.g., use application.getArcadeGames() or the API that returns runs
including in‑progress ones) instead of application.getCompletedArcadeGames(),
then keep the same null check and mStyle.setValue(lastPlayed.getStyle())
behavior so it falls back safely if no runs exist; update the call site where
Iterables.getLast and the lastPlayed local are created in
ArcadeStatisticsViewModel.
Comment on lines
+177
to
+180
| // Style Chip | ||
| if (mStyleChipGroup.getVisibility() == View.VISIBLE) { | ||
| addSummaryChip( | ||
| mStyle == com.antsapps.triples.backend.ArcadeGame.ArcadeStyle.BONUS ? "Bonus" : "Fixed"); |
There was a problem hiding this comment.
Don't hardcode the style summary label.
"Bonus" / "Fixed" here will drift from the actual chip copy and won't localize. Reuse the checked chip's text or a string resource instead.
Suggested change
// Style Chip
if (mStyleChipGroup.getVisibility() == View.VISIBLE) {
- addSummaryChip(
- mStyle == com.antsapps.triples.backend.ArcadeGame.ArcadeStyle.BONUS ? "Bonus" : "Fixed");
+ Chip selectedStyleChip =
+ mStyleChipGroup.findViewById(
+ mStyle == com.antsapps.triples.backend.ArcadeGame.ArcadeStyle.BONUS
+ ? R.id.chip_style_bonus
+ : R.id.chip_style_fixed);
+ addSummaryChip(String.valueOf(selectedStyleChip.getText()));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/antsapps/triples/stats/StatisticsSelectorView.java`
around lines 177 - 180, The label is hardcoded; instead derive it from the
selected chip or a string resource: when mStyleChipGroup is visible, get the
currently checked chip's text (use mStyleChipGroup.getCheckedChipId() then
findViewById<Chip>(id).getText()) and pass that text into addSummaryChip rather
than using the literal "Bonus"/"Fixed"; if the checked chip is null, fall back
to a localized string resource keyed to ArcadeGame.ArcadeStyle (use mStyle to
map to getString(...) fallback) to ensure localization and
single-source-of-truth for the label.
Comment on lines
+108
to
+142
| <TextView | ||
| android:id="@+id/style_label" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:paddingTop="8dp" | ||
| android:text="STYLE" | ||
| android:textSize="10sp" | ||
| android:textStyle="bold" | ||
| android:textColor="@color/color_text_secondary" | ||
| android:visibility="gone" /> | ||
|
|
||
| <com.google.android.material.chip.ChipGroup | ||
| android:id="@+id/style_chip_group" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:paddingBottom="8dp" | ||
| android:visibility="gone" | ||
| app:selectionRequired="true" | ||
| app:singleSelection="true"> | ||
|
|
||
| <com.google.android.material.chip.Chip | ||
| android:id="@+id/chip_style_fixed" | ||
| style="@style/Widget.MaterialComponents.Chip.Choice" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginEnd="8dp" | ||
| android:text="Fixed" /> | ||
|
|
||
| <com.google.android.material.chip.Chip | ||
| android:id="@+id/chip_style_bonus" | ||
| style="@style/Widget.MaterialComponents.Chip.Choice" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginEnd="8dp" | ||
| android:text="Bonus" /> |
There was a problem hiding this comment.
Move the new style labels into string resources.
STYLE, Fixed, and Bonus are hardcoded here, so the new selector will not localize with the rest of the screen. Please switch these to @string/... entries.
🌐 Suggested cleanup
- android:text="STYLE"
+ android:text="@string/stats_style_label"
...
- android:text="Fixed" />
+ android:text="@string/arcade_style_fixed" />
...
- android:text="Bonus" />
+ android:text="@string/arcade_style_bonus" />Also add the corresponding entries in strings.xml.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <TextView | |
| android:id="@+id/style_label" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:paddingTop="8dp" | |
| android:text="STYLE" | |
| android:textSize="10sp" | |
| android:textStyle="bold" | |
| android:textColor="@color/color_text_secondary" | |
| android:visibility="gone" /> | |
| <com.google.android.material.chip.ChipGroup | |
| android:id="@+id/style_chip_group" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content" | |
| android:paddingBottom="8dp" | |
| android:visibility="gone" | |
| app:selectionRequired="true" | |
| app:singleSelection="true"> | |
| <com.google.android.material.chip.Chip | |
| android:id="@+id/chip_style_fixed" | |
| style="@style/Widget.MaterialComponents.Chip.Choice" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:layout_marginEnd="8dp" | |
| android:text="Fixed" /> | |
| <com.google.android.material.chip.Chip | |
| android:id="@+id/chip_style_bonus" | |
| style="@style/Widget.MaterialComponents.Chip.Choice" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:layout_marginEnd="8dp" | |
| android:text="Bonus" /> | |
| <TextView | |
| android:id="@+id/style_label" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:paddingTop="8dp" | |
| android:text="@string/stats_style_label" | |
| android:textSize="10sp" | |
| android:textStyle="bold" | |
| android:textColor="@color/color_text_secondary" | |
| android:visibility="gone" /> | |
| <com.google.android.material.chip.ChipGroup | |
| android:id="@+id/style_chip_group" | |
| android:layout_width="match_parent" | |
| android:layout_height="wrap_content" | |
| android:paddingBottom="8dp" | |
| android:visibility="gone" | |
| app:selectionRequired="true" | |
| app:singleSelection="true"> | |
| <com.google.android.material.chip.Chip | |
| android:id="@+id/chip_style_fixed" | |
| style="@style/Widget.MaterialComponents.Chip.Choice" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:layout_marginEnd="8dp" | |
| android:text="@string/arcade_style_fixed" /> | |
| <com.google.android.material.chip.Chip | |
| android:id="@+id/chip_style_bonus" | |
| style="@style/Widget.MaterialComponents.Chip.Choice" | |
| android:layout_width="wrap_content" | |
| android:layout_height="wrap_content" | |
| android:layout_marginEnd="8dp" | |
| android:text="@string/arcade_style_bonus" /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/layout/stats_selector.xml` around lines 108 - 142, Replace
hardcoded text in the stats_selector layout with string resources: change the
TextView with id style_label to use android:text="@string/style_label", the Chip
with id chip_style_fixed to use android:text="@string/style_fixed", and the Chip
with id chip_style_bonus to use android:text="@string/style_bonus"; then add
matching entries (style_label, style_fixed, style_bonus) to strings.xml with the
current values "STYLE", "Fixed", and "Bonus" (and translated variants as
needed).
Comment on lines
+41
to
+46
| ArcadeStatistics fixedStats = new ArcadeStatistics(games, Period.ALL_TIME, true, ArcadeGame.ArcadeStyle.FIXED); | ||
| System.out.println("Fixed stats num games: " + fixedStats.getNumGames()); | ||
| assertThat(fixedStats.getNumGames()).isEqualTo(2); | ||
|
|
||
| ArcadeStatistics bonusStats = new ArcadeStatistics(games, Period.ALL_TIME, true, ArcadeGame.ArcadeStyle.BONUS); | ||
| System.out.println("Bonus stats num games: " + bonusStats.getNumGames()); |
There was a problem hiding this comment.
Drop the temporary stdout logging from this test.
The assertions already capture the behavior, so the System.out.println(...) calls just add noise to CI logs. Please run Spotless on this block while removing them—the long constructor lines here also match the current spotlessJavaCheck failure.
🧹 Suggested cleanup
- ArcadeStatistics fixedStats = new ArcadeStatistics(games, Period.ALL_TIME, true, ArcadeGame.ArcadeStyle.FIXED);
- System.out.println("Fixed stats num games: " + fixedStats.getNumGames());
+ ArcadeStatistics fixedStats =
+ new ArcadeStatistics(games, Period.ALL_TIME, true, ArcadeGame.ArcadeStyle.FIXED);
assertThat(fixedStats.getNumGames()).isEqualTo(2);
- ArcadeStatistics bonusStats = new ArcadeStatistics(games, Period.ALL_TIME, true, ArcadeGame.ArcadeStyle.BONUS);
- System.out.println("Bonus stats num games: " + bonusStats.getNumGames());
+ ArcadeStatistics bonusStats =
+ new ArcadeStatistics(games, Period.ALL_TIME, true, ArcadeGame.ArcadeStyle.BONUS);
assertThat(bonusStats.getNumGames()).isEqualTo(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/test/java/com/antsapps/triples/backend/ArcadeStatisticsTest.java`
around lines 41 - 46, Remove the temporary System.out.println calls around the
test instantiations of ArcadeStatistics (remove the two lines that call
System.out.println before asserting/getting getNumGames), leaving the assertions
and constructions for ArcadeStatistics(games, Period.ALL_TIME, true,
ArcadeGame.ArcadeStyle.FIXED) and ArcadeStatistics(games, Period.ALL_TIME, true,
ArcadeGame.ArcadeStyle.BONUS) intact; then run Spotless (spotlessJavaApply) to
reformat the long constructor lines and ensure the file passes
spotlessJavaCheck.
- Implemented 'Bonus' arcade style where triples grant extra time (5s for triples 1-5, 4s for 6-10, etc.). - Updated Timer to allow time modifications. - Modified ArcadeGame to handle bonus logic and deck reshuffling. - Updated DBAdapter (v9) and ProtoBuf definitions for persistence and cloud sync. - Redesigned MainActivity UI to offer Fixed and Bonus arcade options. - Enhanced Statistics with style filtering and updated StatisticsSelectorView. - Added ArcadeStatisticsTest to verify filtering logic. - Applied code formatting via Spotless. Co-authored-by: amorris13 <4523811+amorris13@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/com/antsapps/triples/backend/DBAdapter.java (1)
63-65:⚠️ Potential issue | 🟠 MajorRemove
arcade_stylefrom Classic table creation.
CREATE_CLASSIC_GAMESstill includesCOLUMN_ARCADE_STYLE, which is arcade-only. This creates schema drift between fresh installs (column exists) and upgraded installs (column absent on classic table).🛠️ Proposed fix
private static final String CREATE_CLASSIC_GAMES = "CREATE TABLE " + TABLE_CLASSIC_GAMES + "(" @@ + COLUMN_HINTS_USED + " INTEGER, " // + COLUMN_FOUND_TRIPLES - + " BLOB, " // - + COLUMN_ARCADE_STYLE - + " TEXT)"; + + " BLOB)";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/antsapps/triples/backend/DBAdapter.java` around lines 63 - 65, The CREATE_CLASSIC_GAMES table definition currently appends COLUMN_ARCADE_STYLE (arcade-only) causing schema drift; update the CREATE_CLASSIC_GAMES SQL constant to remove any reference to COLUMN_ARCADE_STYLE so the classic table creation string only includes classic columns (ensure comma/parenthesis punctuation remains correct), and keep COLUMN_ARCADE_STYLE only in the arcade table creation logic (e.g., CREATE_ARCADE_GAMES) if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/com/antsapps/triples/backend/DBAdapter.java`:
- Around line 63-65: The CREATE_CLASSIC_GAMES table definition currently appends
COLUMN_ARCADE_STYLE (arcade-only) causing schema drift; update the
CREATE_CLASSIC_GAMES SQL constant to remove any reference to COLUMN_ARCADE_STYLE
so the classic table creation string only includes classic columns (ensure
comma/parenthesis punctuation remains correct), and keep COLUMN_ARCADE_STYLE
only in the arcade table creation logic (e.g., CREATE_ARCADE_GAMES) if present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b1012b9a-0575-4320-978e-a7cb80868f65
📒 Files selected for processing (6)
app/src/main/java/com/antsapps/triples/MainActivity.javaapp/src/main/java/com/antsapps/triples/MainViewModel.javaapp/src/main/java/com/antsapps/triples/backend/ArcadeStatistics.javaapp/src/main/java/com/antsapps/triples/backend/DBAdapter.javaapp/src/main/java/com/antsapps/triples/backend/Statistics.javaapp/src/test/java/com/antsapps/triples/backend/ArcadeStatisticsTest.java
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/antsapps/triples/backend/Statistics.java
🚧 Files skipped from review as they are similar to previous changes (4)
- app/src/main/java/com/antsapps/triples/MainViewModel.java
- app/src/main/java/com/antsapps/triples/backend/ArcadeStatistics.java
- app/src/test/java/com/antsapps/triples/backend/ArcadeStatisticsTest.java
- app/src/main/java/com/antsapps/triples/MainActivity.java
- Added 'Bonus' arcade style with decreasing time bonuses for triples found. - Updated Timer to support time modifications. - Enhanced ArcadeGame with bonus logic and deck reshuffling. - Updated DBAdapter (v9) and ProtoBuf for persistence. - Redesigned MainActivity to offer 'Fixed' and 'Bonus' arcade options. - Added style filtering to Statistics and updated StatisticsSelectorView. - Fixed NullPointerExceptions in BaseStatisticsFragment and ScreenshotTest. - Added ArcadeStatisticsTest and updated NavigationTest. - Fixed compilation errors in Statistics and AchievementManager. - Applied code formatting via Spotless. Co-authored-by: amorris13 <4523811+amorris13@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/test/java/com/antsapps/triples/ScreenshotTest.java (1)
425-459: Consider parameterizingsetupArcadeGamebyArcadeStylefor screenshot coverage.Right now the completed fixture hardcodes
FIXED(Line 451), so BONUS-specific arcade UI regressions may be missed. AsetupArcadeGame(boolean completed, ArcadeGame.ArcadeStyle style)helper plus BONUS snapshots would improve coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/com/antsapps/triples/ScreenshotTest.java` around lines 425 - 459, The helper setupArcadeGame(boolean completed) currently hardcodes ArcadeGame.ArcadeStyle.FIXED for the completed fixture; change its signature to setupArcadeGame(boolean completed, ArcadeGame.ArcadeStyle style) and use the provided style when constructing the ArcadeGame (replace the FIXED usage in the ArcadeGame constructor call), update all existing callers to pass the desired style (keep existing tests passing by passing FIXED where appropriate), and add new test invocations that call setupArcadeGame(..., ArcadeGame.ArcadeStyle.BONUS) to capture BONUS screenshots for UI regression coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/antsapps/triples/AchievementManager.java`:
- Around line 124-143: Summary: awardArcadeCountAchievements is only called for
non-BONUS games, so Bonus-only players lose count achievements; include bonus
games in the count. Fix: in AchievementManager adjust the logic after the loop
(which sets completedArcadeGames and completedArcadeBonusGames) to ensure
awardArcadeCountAchievements is invoked for bonus games as well—either call
awardArcadeCountAchievements(context, completedArcadeGames +
completedArcadeBonusGames) once, or call awardArcadeCountAchievements(context,
completedArcadeGames) and also awardArcadeCountAchievements(context,
completedArcadeBonusGames) when completedArcadeBonusGames > 0; reference
completedArcadeGames, completedArcadeBonusGames, and
awardArcadeCountAchievements to locate the change.
In `@app/src/main/res/values/games-ids.xml`:
- Around line 90-96: The strings in games-ids.xml
(achievement_arcade_bonus_5_triples, achievement_arcade_bonus_10_triples,
achievement_arcade_bonus_25_triples, achievement_arcade_bonus_50_triples,
achievement_arcade_bonus_75_triples, achievement_arcade_bonus_100_triples,
leaderboard_arcade_bonus_game) are current placeholders and must be replaced
with the real Play Games service IDs; update each string value to the actual
achievement and leaderboard IDs from the Play Console so AchievementManager.java
(unlock calls) and ArcadeGameActivity.java (score submission calls) reference
valid IDs and achievements/leaderboard submissions will work.
---
Nitpick comments:
In `@app/src/test/java/com/antsapps/triples/ScreenshotTest.java`:
- Around line 425-459: The helper setupArcadeGame(boolean completed) currently
hardcodes ArcadeGame.ArcadeStyle.FIXED for the completed fixture; change its
signature to setupArcadeGame(boolean completed, ArcadeGame.ArcadeStyle style)
and use the provided style when constructing the ArcadeGame (replace the FIXED
usage in the ArcadeGame constructor call), update all existing callers to pass
the desired style (keep existing tests passing by passing FIXED where
appropriate), and add new test invocations that call setupArcadeGame(...,
ArcadeGame.ArcadeStyle.BONUS) to capture BONUS screenshots for UI regression
coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1482b5b3-79f6-4419-907f-3d99168d3a11
📒 Files selected for processing (6)
app/src/main/java/com/antsapps/triples/AchievementManager.javaapp/src/main/java/com/antsapps/triples/ArcadeGameActivity.javaapp/src/main/java/com/antsapps/triples/stats/BaseStatisticsFragment.javaapp/src/main/res/values/games-ids.xmlapp/src/main/res/values/strings.xmlapp/src/test/java/com/antsapps/triples/ScreenshotTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/res/values/strings.xml
Comment on lines
+124
to
+143
| int completedArcadeBonusGames = 0; | ||
| int maxArcadeBonusTriples = 0; | ||
| for (ArcadeGame game : application.getCompletedArcadeGames()) { | ||
| if (!game.areHintsUsed()) { | ||
| completedArcadeGames++; | ||
| maxArcadeTriples = Math.max(maxArcadeTriples, game.getNumTriplesFound()); | ||
| if (game.getStyle() == ArcadeGame.ArcadeStyle.BONUS) { | ||
| completedArcadeBonusGames++; | ||
| maxArcadeBonusTriples = Math.max(maxArcadeBonusTriples, game.getNumTriplesFound()); | ||
| } else { | ||
| completedArcadeGames++; | ||
| maxArcadeTriples = Math.max(maxArcadeTriples, game.getNumTriplesFound()); | ||
| } | ||
| } | ||
| } | ||
| if (completedArcadeGames > 0) { | ||
| awardArcadeAchievements(context, maxArcadeTriples); | ||
| awardArcadeAchievements(context, maxArcadeTriples, false); | ||
| awardArcadeCountAchievements(context, completedArcadeGames); | ||
| } | ||
| if (completedArcadeBonusGames > 0) { | ||
| awardArcadeAchievements(context, maxArcadeBonusTriples, true); | ||
| } |
There was a problem hiding this comment.
Arcade count achievements now exclude Bonus games during sync.
At Line 128 to Line 134, bonus games are no longer included in completedArcadeGames, but Line 139 still uses that value for awardArcadeCountAchievements(...). This regresses count achievements for users who play only Bonus style (or mostly Bonus).
Suggested fix
- int completedArcadeGames = 0;
+ int completedArcadeGames = 0;
+ int completedArcadeGamesTotal = 0;
int maxArcadeTriples = 0;
int completedArcadeBonusGames = 0;
int maxArcadeBonusTriples = 0;
for (ArcadeGame game : application.getCompletedArcadeGames()) {
if (!game.areHintsUsed()) {
+ completedArcadeGamesTotal++;
if (game.getStyle() == ArcadeGame.ArcadeStyle.BONUS) {
completedArcadeBonusGames++;
maxArcadeBonusTriples = Math.max(maxArcadeBonusTriples, game.getNumTriplesFound());
} else {
completedArcadeGames++;
maxArcadeTriples = Math.max(maxArcadeTriples, game.getNumTriplesFound());
}
}
}
if (completedArcadeGames > 0) {
awardArcadeAchievements(context, maxArcadeTriples, false);
- awardArcadeCountAchievements(context, completedArcadeGames);
}
+ if (completedArcadeGamesTotal > 0) {
+ awardArcadeCountAchievements(context, completedArcadeGamesTotal);
+ }
if (completedArcadeBonusGames > 0) {
awardArcadeAchievements(context, maxArcadeBonusTriples, true);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/antsapps/triples/AchievementManager.java` around lines
124 - 143, Summary: awardArcadeCountAchievements is only called for non-BONUS
games, so Bonus-only players lose count achievements; include bonus games in the
count. Fix: in AchievementManager adjust the logic after the loop (which sets
completedArcadeGames and completedArcadeBonusGames) to ensure
awardArcadeCountAchievements is invoked for bonus games as well—either call
awardArcadeCountAchievements(context, completedArcadeGames +
completedArcadeBonusGames) once, or call awardArcadeCountAchievements(context,
completedArcadeGames) and also awardArcadeCountAchievements(context,
completedArcadeBonusGames) when completedArcadeBonusGames > 0; reference
completedArcadeGames, completedArcadeBonusGames, and
awardArcadeCountAchievements to locate the change.
Comment on lines
+90
to
+96
| <string name="achievement_arcade_bonus_5_triples" translatable="false">placeholder_bonus_5</string> | ||
| <string name="achievement_arcade_bonus_10_triples" translatable="false">placeholder_bonus_10</string> | ||
| <string name="achievement_arcade_bonus_25_triples" translatable="false">placeholder_bonus_25</string> | ||
| <string name="achievement_arcade_bonus_50_triples" translatable="false">placeholder_bonus_50</string> | ||
| <string name="achievement_arcade_bonus_75_triples" translatable="false">placeholder_bonus_75</string> | ||
| <string name="achievement_arcade_bonus_100_triples" translatable="false">placeholder_bonus_100</string> | ||
| <string name="leaderboard_arcade_bonus_game" translatable="false">placeholder_bonus_leaderboard</string> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unresolved placeholder IDs and where they are used.
rg -n 'placeholder_bonus_|placeholder_' app/src/main/res/values/games-ids.xml
rg -n 'achievement_arcade_bonus_|leaderboard_arcade_bonus_game' \
app/src/main/java/com/antsapps/triples/AchievementManager.java \
app/src/main/java/com/antsapps/triples/ArcadeGameActivity.javaRepository: amorris13/triples
Length of output: 1856
Replace placeholder Play Games IDs with actual service identifiers.
Lines 90-96 contain placeholder values that will prevent achievement unlocks and leaderboard submission. These resource IDs are actively used in AchievementManager.java and ArcadeGameActivity.java to unlock achievements and submit scores.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/values/games-ids.xml` around lines 90 - 96, The strings in
games-ids.xml (achievement_arcade_bonus_5_triples,
achievement_arcade_bonus_10_triples, achievement_arcade_bonus_25_triples,
achievement_arcade_bonus_50_triples, achievement_arcade_bonus_75_triples,
achievement_arcade_bonus_100_triples, leaderboard_arcade_bonus_game) are current
placeholders and must be replaced with the real Play Games service IDs; update
each string value to the actual achievement and leaderboard IDs from the Play
Console so AchievementManager.java (unlock calls) and ArcadeGameActivity.java
(score submission calls) reference valid IDs and achievements/leaderboard
submissions will work.
- Added 'Bonus' arcade style where triples grant extra time (5s for triples 1-5, 4s for 6-10, etc.). - Updated Timer and ArcadeGame to support time modifications and bonus logic. - Redesigned MainActivity UI with 'Fixed' and 'Bonus' arcade buttons. - Enhanced Statistics with style filtering and updated StatisticsSelectorView. - Fixed an initialization order bug in BaseStatisticsFragment that caused NPEs in tests. - Corrected a database schema error where 'arcade_style' was accidentally added to Classic games. - Updated ScreenshotTest to properly support the new Arcade style. - Ensured all tests pass and screenshots are bit-identical via verifyRoborazziDebug. - Applied code formatting via Spotless. Co-authored-by: amorris13 <4523811+amorris13@users.noreply.github.com>
- Redesigned Arcade section in MainActivity to always show 'Fixed' and 'Bonus' new game buttons. - Allowed Resume split-button group to appear above new game buttons when a game is in progress. - Added string resources for 'Fixed' and 'Bonus' styles. - Updated ArcadeGameActivity to use current game style for performance stats and 'Play Again'. - Synchronized Statistics UI to use proper style labels. - Verified bit-identical Roborazzi screenshots. Co-authored-by: amorris13 <4523811+amorris13@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/antsapps/triples/ArcadeGameActivity.java (1)
100-103: Consider differentiating game type for analytics.The method returns
"Arcade"for both Fixed and Bonus styles. If analytics require distinguishing between the two styles, consider returning a style-specific string (e.g.,"Arcade Fixed"or"Arcade Bonus"). If both styles should be grouped under a single "Arcade" category for analytics, the current implementation is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/antsapps/triples/ArcadeGameActivity.java` around lines 100 - 103, getGameType() in ArcadeGameActivity currently returns the generic "Arcade" which prevents analytics from distinguishing Fixed vs Bonus; update getGameType() to return a style-specific string (e.g., "Arcade Fixed" or "Arcade Bonus") by checking the existing style indicator used by this activity (use the activity's field or method that represents the mode — e.g., a gameStyle field, getStyle(), or isBonusStyle()) and return "Arcade Fixed" when style indicates fixed play and "Arcade Bonus" when it indicates bonus play; if no style accessor exists, add a minimal accessor to determine the mode and use it in getGameType().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/com/antsapps/triples/ArcadeGameActivity.java`:
- Around line 100-103: getGameType() in ArcadeGameActivity currently returns the
generic "Arcade" which prevents analytics from distinguishing Fixed vs Bonus;
update getGameType() to return a style-specific string (e.g., "Arcade Fixed" or
"Arcade Bonus") by checking the existing style indicator used by this activity
(use the activity's field or method that represents the mode — e.g., a gameStyle
field, getStyle(), or isBonusStyle()) and return "Arcade Fixed" when style
indicates fixed play and "Arcade Bonus" when it indicates bonus play; if no
style accessor exists, add a minimal accessor to determine the mode and use it
in getGameType().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d24bb9c8-7a4e-43d5-92c3-49bac6b3d59d
📒 Files selected for processing (5)
app/src/main/java/com/antsapps/triples/ArcadeGameActivity.javaapp/src/main/java/com/antsapps/triples/MainActivity.javaapp/src/main/java/com/antsapps/triples/stats/StatisticsSelectorView.javaapp/src/main/res/layout/activity_main.xmlapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/res/layout/activity_main.xml
- app/src/main/java/com/antsapps/triples/stats/StatisticsSelectorView.java
- app/src/main/java/com/antsapps/triples/MainActivity.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added a new 'Bonus' style to Arcade mode where each triple found grants extra time (5s for first 5, 4s for next 5, etc.).
Key changes:
Fixes #213
PR created automatically by Jules for task 17888960947768524627 started by @amorris13
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores