Refactor: Generic Layout Strategy and Custom Layout Support#16
Refactor: Generic Layout Strategy and Custom Layout Support#16
Conversation
…out. Extracted hardcoded layout logic (Jammer, Sonome, Janko) into `LayoutStrategy` interface and `LayoutStrategies` implementation. Added `CustomStrategy` and `CustomKey` to support user-defined tonal steps and grouping. Updated `HexKeyboard` to use the generic strategy for layout generation. Added preferences for Custom layout configuration (steps, grouping, base note). Co-authored-by: lrq3000 <1118942+lrq3000@users.noreply.github.com>
|
👋 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. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis pull request introduces a new "Custom" keyboard layout to the StarChord application by implementing a generalized strategy pattern for layout management. The changes include a new CustomKey class, a LayoutStrategy interface with multiple implementations, comprehensive refactoring of HexKeyboard to eliminate per-board conditional logic, and supporting preferences and UI strings for the custom layout configuration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef42cde910
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pitch += params.pitch1; | ||
|
|
||
| jankoColumnNumber++; // for Janko only | ||
| if (jankoColumnNumber % groupSize == 0) octaveGroupNumber++; // for Janko only | ||
| colCounter++; | ||
| if (params.groupSize > 0 && colCounter % params.groupSize == 0) { | ||
| octaveGroupNumber++; | ||
| pitch += params.groupJumpPitch; | ||
| } |
There was a problem hiding this comment.
The Custom layout UI states that group size is “Number of columns (Vertical)” (strings.xml lines 201–202), but in the vertical loop colCounter increments after both the kitty and main key, so a “column” advances the counter twice. With customGroupSize > 0, groupJumpPitch and octaveGroupNumber trigger at half the intended column count, which shifts pitches too early and mis-groups octaves for Custom vertical layouts. Consider incrementing once per column (for example after the main key) or using a dedicated column counter for grouping.
Useful? React with 👍 / 👎.
| rowCounter += 2; | ||
| if (params.groupSize > 0 && rowCounter % params.groupSize == 0) | ||
| { | ||
| pitch -= 12; | ||
| pitch += params.groupJumpPitch; | ||
| rowFirstPitch = pitch; | ||
| octaveGroupNumber++; |
There was a problem hiding this comment.
The Custom layout description defines group size as “Number of … rows (Horizontal)” (strings.xml lines 201–202), but rowCounter is incremented by 2 per loop iteration. That means groupJumpPitch fires after half the number of rows a user configured in customGroupSize, causing unexpected pitch jumps in Custom horizontal layouts. If the double-step is only required for Janko, the counter should be layout-specific, or increment by 1 for Custom to match the documented “rows” semantics.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@StarChord/src/main/java/opensource/starchord/CustomKey.java`:
- Around line 13-18: The getPrefs method sets mKeyOrientation from mPrefs
without a safe default, causing NPEs when code later calls
mKeyOrientation.equals(...); update CustomKey.getPrefs to supply a non-null
default (e.g., "Horizontal" or a sensible default) when calling
mPrefs.getString("customKeyOrientation", ...), so mKeyOrientation is never null;
apply the same change to other HexKey subclasses (SonomeKey, JankoKey,
JammerKey) to ensure getHexagonPath and other methods that call
mKeyOrientation.equals(...) cannot throw NullPointerException.
- Around line 28-36: The if block checking sharpName.contains("G") inside the
CustomKey logic is dead code; either delete that empty conditional entirely to
remove noise, or implement the intended G# highlighting by adding the actual
state change (e.g., set a highlight flag or assign a highlightColor property)
within the CustomKey constructor/initializer where sharpName is evaluated.
Locate the sharpName check in the CustomKey class and either remove the whole if
(sharpName.contains("G")) { /*...*/ } branch, or replace the comments with
concrete behavior such as this.setHighlighted(true) or this.color =
HIGHLIGHT_COLOR (use the class's existing color/highlight fields or methods).
In `@StarChord/src/main/java/opensource/starchord/HexKeyboard.java`:
- Around line 481-487: The horizontal grouping code in HexKeyboard (variables
rowCounter, params.groupSize, params.groupJumpPitch, rowFirstPitch,
octaveGroupNumber) applies the group jump after incrementing rowCounter at row
boundaries, which differs from vertical grouping that applies jumps inside the
column loop; add a clear inline comment near this block explaining that
rowCounter += 2 accounts for both kitty-corner and regular keys and that the
group jump is intentionally applied after completing a row (not per-column) due
to the hex grid layout, and mention the effect on pitch progression so future
maintainers understand the asymmetry.
In `@StarChord/src/main/java/opensource/starchord/LayoutStrategies.java`:
- Around line 107-144: The createKey method currently calls
HexKey.getKeyOrientation(context) causing redundant preference lookups and
potential side effects; change createKey to accept an orientation parameter
(e.g., String orientation) and use that instead of calling
HexKey.getKeyOrientation inside createKey, update all callers (notably where
getParameters() constructs keys) to pass the orientation value obtained once in
getParameters(), and keep the existing behavior: if orientation equals
"Vertical" add 12 * groupNum to pitch when constructing a new JankoKey,
otherwise pass pitch unchanged; this removes the hidden mutation from
HexKey.getKeyOrientation and avoids inconsistent reads.
- Around line 186-200: The commented-out pitchPre logic should be cleaned up and
the assumed behavior made explicit: inside LayoutStrategies where you check if
p.groupSize > 0 && orientation.equals("Vertical"), remove the long exploratory
comments and either implement the behavior or mark it as TODO; implement the
current assumed behavior by explicitly setting p.pitchPre = 0 (so the code
documents intent) and compute groupCount from columnCount / p.groupSize as
shown, and add a single-line TODO comment referencing that Janko-like centering
is unimplemented (e.g., TODO: consider Janko-like pitchPre adjustment) so the
feature is tracked.
In `@StarChord/src/main/java/opensource/starchord/LayoutStrategy.java`:
- Around line 7-15: Parameters currently exposes mutable public fields
(pitchPre, pitch1, pitch2, pitchPost, groupSize, initialPitchOffset,
groupJumpPitch) which can be accidentally changed after creation; make the class
immutable by declaring these fields final and adding a constructor that
initializes all of them (or implement a simple builder inside Parameters to
construct instances immutably), then update any call sites that instantiate
Parameters to use the new constructor/builder—this preserves internal use while
preventing accidental modification.
In `@StarChord/src/main/res/values/strings.xml`:
- Around line 112-114: Add the missing default orientation string for the Custom
layout by defining the resource key default_custom_orientation and setting its
value to "Landscape"; update the same block that contains default_custom_note,
default_custom_octave, and default_custom_key_orientation so the Custom layout
matches the pattern used by
default_sonome_orientation/default_jammer_orientation/default_janko_orientation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
StarChord/src/main/java/opensource/starchord/CustomKey.javaStarChord/src/main/java/opensource/starchord/HexKey.javaStarChord/src/main/java/opensource/starchord/HexKeyboard.javaStarChord/src/main/java/opensource/starchord/LayoutStrategies.javaStarChord/src/main/java/opensource/starchord/LayoutStrategy.javaStarChord/src/main/java/opensource/starchord/Prefer.javaStarChord/src/main/res/values/strings.xmlStarChord/src/main/res/xml/preferences.xml
🧰 Additional context used
🧬 Code graph analysis (2)
StarChord/src/main/java/opensource/starchord/LayoutStrategies.java (1)
StarChord/src/main/java/opensource/starchord/HexKey.java (1)
HexKey(39-867)
StarChord/src/main/java/opensource/starchord/HexKeyboard.java (2)
StarChord/src/main/java/opensource/starchord/LayoutStrategies.java (1)
LayoutStrategies(7-214)StarChord/src/main/java/opensource/starchord/HexKey.java (1)
HexKey(39-867)
🔇 Additional comments (13)
StarChord/src/main/java/opensource/starchord/Prefer.java (1)
561-565: LGTM!The custom octave preference listener is correctly wired following the same pattern as the existing
baseJammerOctaveandbaseJankoOctavepreferences. The null check ensures robustness if the preference is not found.StarChord/src/main/java/opensource/starchord/HexKey.java (1)
256-260: LGTM!The Custom layout branch correctly follows the established pattern for Sonome and Janko. Setting
mKeyOverlap = falseis appropriate since the Custom layout doesn't require overlap functionality.StarChord/src/main/java/opensource/starchord/LayoutStrategy.java (1)
17-19: LGTM!The interface design is clean with a clear contract. The three methods provide a good separation of concerns:
getName()for identification,getParameters()for configuration, andcreateKey()for instantiation.StarChord/src/main/res/xml/preferences.xml (1)
67-80: LGTM!The Custom layout preferences are well-structured and follow the established patterns from Jammer, Janko, and Sonome categories. The numeric constraints are appropriate:
customGroupJumpcorrectly usesinteger|signedto allow negative values (default -12)customGroupSizeuses unsigned integer since group sizes should be non-negative- All fields have appropriate
maxLengthconstraintsStarChord/src/main/java/opensource/starchord/LayoutStrategies.java (2)
9-16: LGTM!The factory method provides a clean way to select the appropriate layout strategy based on user preferences. The fallback to
JammerStrategyas the default is appropriate.
209-212: CustomStrategy.createKey() ignoresgroupNumparameter and CustomKey cannot accept it.The review's observation is accurate:
JankoStrategy.createKey()usesgroupNumto adjust pitch for vertical mode (pitch + 12 * groupNumat line 140), whileCustomStrategy.createKey()ignores this parameter entirely. Additionally,CustomKey's constructor does not accept agroupNumparameter (unlikeJankoKey), so the parameter cannot be passed even if intended.
CustomStrategy.getParameters()does readcustomGroupSize(line 171), suggesting custom layouts should support grouping similar to Janko. However, withoutCustomKeyacceptinggroupNumandcreateKey()not passing it, vertical orientation grouping is not implemented. This may be intentional (custom layout designed differently than Janko) or an incomplete feature—clarify the intended design and update bothCustomKeyconstructor andCustomStrategy.createKey()accordingly if octave grouping should be supported.StarChord/src/main/java/opensource/starchord/HexKeyboard.java (4)
178-182: Clean strategy pattern integration.The initialization of the layout strategy and retrieval of parameters is well-structured. Caching the
orientationvalue here prevents redundant preference lookups in the loop.
446-461: Horizontal mode applies group jump before key creation - correct for Janko-like behavior.The logic correctly calculates
jpitchandogn(octave group number) before creating the key, allowing the pitch to be adjusted at group boundaries. This matches the original Janko horizontal behavior where the pitch was decremented by 12 at group transitions.
732-732: Excellent simplification with the strategy pattern.Replacing layout-specific
setUpJammerBoard(),setUpSonomeBoard(), etc. with a singlesetUpKeyBoard(layoutPref)call that delegates to the strategy significantly reduces code duplication and makes adding new layouts straightforward.
271-294: Verify the colCounter increment semantics for grouping behavior.The
colCounterincrements twice per column iteration (after kittyCornerKey and after the regular key), meaning it increments by 2 for each logical column. This affects howparams.groupSizebehaves: when a user setsjankoRowCount=4(for Janko layout) orcustomGroupSize=4(for Custom layout), the grouping boundary checkcolCounter % params.groupSize == 0will trigger atcolCounter=4, 8, 12..., which corresponds to every 2 columns rather than every 4 columns, creating a 2:1 ratio discrepancy.The original
jankoRowNumbervariable has been removed in the LayoutStrategy refactor. Verify that this double-increment pattern correctly implements the intended grouping behavior for both Janko and Custom layouts, and clarify whether the preference semantics (jankoRowCount,customGroupSize) should represent increments or actual columns/rows.StarChord/src/main/res/values/strings.xml (3)
51-51: LGTM!The "Custom" layout option is correctly added to the layouts array, consistent with the existing naming pattern.
169-169: LGTM!The heading follows the established pattern and appropriately uses a simple name for the user-configurable layout.
188-205: The "Kitty" terminology is correct and consistently used throughout the codebase to refer to corner keys in the hex keyboard layout. This is intentional domain-specific terminology, not unclear jargon.The description of group_size as "Number of columns (Vertical) or rows (Horizontal)" warrants verification against the actual implementation to confirm it matches the intended hex key geometry behavior, though this cannot be confirmed from available codebase references.
Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @Override | ||
| protected void getPrefs() | ||
| { | ||
| mKeyOrientation = mPrefs.getString("customKeyOrientation", null); | ||
| // We could add customKeyOverlap if needed | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how HexKey subclasses handle key orientation preference defaults
rg -n "getString.*KeyOrientation" --type=java -A 1Repository: lrq3000/starchord
Length of output: 1708
🏁 Script executed:
#!/bin/bash
# Find the CustomKey and related HexKey classes
fd ".*Key\.java$" --type=f StarChord/src/main/java/opensource/starchord/Repository: lrq3000/starchord
Length of output: 479
🏁 Script executed:
#!/bin/bash
# Look for getHexagonPath method to understand the NPE concern
rg -n "getHexagonPath\|mKeyOrientation\.equals" --type=java -B 2 -A 5Repository: lrq3000/starchord
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for getHexagonPath method definition
rg -n "getHexagonPath" --type=java -B 2 -A 10Repository: lrq3000/starchord
Length of output: 2392
🏁 Script executed:
#!/bin/bash
# Search for all uses of mKeyOrientation in the codebase
rg -n "mKeyOrientation" --type=java -B 1 -A 1Repository: lrq3000/starchord
Length of output: 2521
Add a default value for key orientation to prevent NullPointerException.
If the customKeyOrientation preference is not set, mKeyOrientation will be null, which will cause a NullPointerException in getHexagonPath() when calling mKeyOrientation.equals("Horizontal") (HexKey.java:269) and in other methods (lines 427, 521).
Proposed fix
`@Override`
protected void getPrefs()
{
- mKeyOrientation = mPrefs.getString("customKeyOrientation", null);
+ mKeyOrientation = mPrefs.getString("customKeyOrientation", "Horizontal");
// We could add customKeyOverlap if needed
}Note: This pattern (using null as default) exists in all HexKey subclasses (SonomeKey, JankoKey, JammerKey), which means they all share this vulnerability. Consider applying the same fix to all subclasses.
📝 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.
| @Override | |
| protected void getPrefs() | |
| { | |
| mKeyOrientation = mPrefs.getString("customKeyOrientation", null); | |
| // We could add customKeyOverlap if needed | |
| } | |
| `@Override` | |
| protected void getPrefs() | |
| { | |
| mKeyOrientation = mPrefs.getString("customKeyOrientation", "Horizontal"); | |
| // We could add customKeyOverlap if needed | |
| } |
🤖 Prompt for AI Agents
In `@StarChord/src/main/java/opensource/starchord/CustomKey.java` around lines 13
- 18, The getPrefs method sets mKeyOrientation from mPrefs without a safe
default, causing NPEs when code later calls mKeyOrientation.equals(...); update
CustomKey.getPrefs to supply a non-null default (e.g., "Horizontal" or a
sensible default) when calling mPrefs.getString("customKeyOrientation", ...), so
mKeyOrientation is never null; apply the same change to other HexKey subclasses
(SonomeKey, JankoKey, JammerKey) to ensure getHexagonPath and other methods that
call mKeyOrientation.equals(...) cannot throw NullPointerException.
| if (sharpName.contains("G")) | ||
| { | ||
| // Mimic Jammer highlighting for now, or just standard black | ||
| // Maybe allow customization? For now, standard piano colors (Black/White). | ||
| // Jammer uses G# as Highlight. | ||
| // Standard piano has no highlight on black keys usually. | ||
| // But let's stick to Black/White logic. | ||
| // If I want to be safe, just standard. | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Dead code: Empty conditional block for G# highlighting.
The if (sharpName.contains("G")) block contains only comments and has no functional effect. The comments suggest potential future highlighting behavior for G# keys (like Jammer), but currently this code does nothing.
Consider either:
- Removing the empty branch if highlighting is intentionally omitted
- Implementing the highlighting if it's intended functionality
🔧 Option 1: Remove dead code if highlighting not needed
public int getColor()
{
String sharpName = mNote.getSharpName();
int color = mWhiteColor;
if (sharpName.contains("#"))
{
color = mBlackColor;
- if (sharpName.contains("G"))
- {
- // Mimic Jammer highlighting for now, or just standard black
- // Maybe allow customization? For now, standard piano colors (Black/White).
- // Jammer uses G# as Highlight.
- // Standard piano has no highlight on black keys usually.
- // But let's stick to Black/White logic.
- // If I want to be safe, just standard.
- }
- // Some specific highlighting for roots (C)?
}📝 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.
| if (sharpName.contains("G")) | |
| { | |
| // Mimic Jammer highlighting for now, or just standard black | |
| // Maybe allow customization? For now, standard piano colors (Black/White). | |
| // Jammer uses G# as Highlight. | |
| // Standard piano has no highlight on black keys usually. | |
| // But let's stick to Black/White logic. | |
| // If I want to be safe, just standard. | |
| } |
🤖 Prompt for AI Agents
In `@StarChord/src/main/java/opensource/starchord/CustomKey.java` around lines 28
- 36, The if block checking sharpName.contains("G") inside the CustomKey logic
is dead code; either delete that empty conditional entirely to remove noise, or
implement the intended G# highlighting by adding the actual state change (e.g.,
set a highlight flag or assign a highlightColor property) within the CustomKey
constructor/initializer where sharpName is evaluated. Locate the sharpName check
in the CustomKey class and either remove the whole if (sharpName.contains("G"))
{ /*...*/ } branch, or replace the comments with concrete behavior such as
this.setHighlighted(true) or this.color = HIGHLIGHT_COLOR (use the class's
existing color/highlight fields or methods).
| rowCounter += 2; | ||
| if (params.groupSize > 0 && rowCounter % params.groupSize == 0) | ||
| { | ||
| pitch -= 12; | ||
| pitch += params.groupJumpPitch; | ||
| rowFirstPitch = pitch; | ||
| octaveGroupNumber++; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Horizontal grouping logic applies jump after row completion.
The rowCounter += 2 (line 481) accounts for both the kitty corner and regular keys in each column iteration. The group jump (line 484) is applied at row boundaries when rowCounter % params.groupSize == 0, which differs from the vertical mode that applies jumps within the column loop.
This asymmetry between vertical and horizontal grouping behavior is intentional based on the hexagonal grid layout, but worth documenting for maintainability.
🤖 Prompt for AI Agents
In `@StarChord/src/main/java/opensource/starchord/HexKeyboard.java` around lines
481 - 487, The horizontal grouping code in HexKeyboard (variables rowCounter,
params.groupSize, params.groupJumpPitch, rowFirstPitch, octaveGroupNumber)
applies the group jump after incrementing rowCounter at row boundaries, which
differs from vertical grouping that applies jumps inside the column loop; add a
clear inline comment near this block explaining that rowCounter += 2 accounts
for both kitty-corner and regular keys and that the group jump is intentionally
applied after completing a row (not per-column) due to the hex grid layout, and
mention the effect on pitch progression so future maintainers understand the
asymmetry.
| @Override | ||
| public HexKey createKey(Context context, int radius, Point center, int pitch, Instrument instru, int keyCount, int groupNum) { | ||
| // Vertical mode logic relies on adding 12 * groupNum here. | ||
| // Horizontal mode logic relies on pitch being shifted by 12, so groupNum impact here? | ||
| // Existing HexKeyboard logic for Horizontal: | ||
| // key = new JankoKey(..., jpitch, ..., ogn); | ||
| // where jpitch was decremented by 12. | ||
| // So createKey should pass pitch as is. | ||
| // But JankoKey uses groupNum for Coloring. | ||
| // AND for Vertical, pitch is NOT shifted in the loop, but key adds 12*groupNum. | ||
| // | ||
| // We can detect orientation inside here via getKeyOrientation? | ||
| // Or assume the pitch passed in is the base pitch, and we modify it if needed? | ||
| // | ||
| // Vertical Loop passes 'ipitch' which is 'pitch'. 'pitch' is not jumped. | ||
| // So we need to add 12*groupNum. | ||
| // | ||
| // Horizontal Loop passes 'jpitch' which IS jumped. | ||
| // So we should NOT add 12*groupNum? | ||
| // But JankoKey constructor: | ||
| // super(..., midiNoteNumber, ...); | ||
| // It doesn't modify midiNoteNumber. | ||
| // Wait, look at HexKeyboard.java again. | ||
|
|
||
| /* | ||
| Vertical: | ||
| key = new JankoKey(..., ipitch + 12 * octaveGroupNumber, ...); | ||
| Horizontal: | ||
| key = new JankoKey(..., jpitch, ...); | ||
| */ | ||
|
|
||
| String orient = HexKey.getKeyOrientation(context); // Note: this uses static prefs lookup which handles Janko | ||
| if (orient.equals("Vertical")) { | ||
| return new JankoKey(context, radius, center, pitch + 12 * groupNum, instru, keyCount, groupNum); | ||
| } else { | ||
| return new JankoKey(context, radius, center, pitch, instru, keyCount, groupNum); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant orientation lookup in createKey - consider passing orientation as parameter.
The createKey method calls HexKey.getKeyOrientation(context) to determine orientation, but this information is already available when getParameters() is called. This creates:
- Redundant preference lookups
- Potential inconsistency if preferences change between calls
- Hidden side effects (
getKeyOrientationmodifiesmKeyOverlap)
Consider adding orientation as a parameter to createKey() to avoid this redundancy.
🤖 Prompt for AI Agents
In `@StarChord/src/main/java/opensource/starchord/LayoutStrategies.java` around
lines 107 - 144, The createKey method currently calls
HexKey.getKeyOrientation(context) causing redundant preference lookups and
potential side effects; change createKey to accept an orientation parameter
(e.g., String orientation) and use that instead of calling
HexKey.getKeyOrientation inside createKey, update all callers (notably where
getParameters() constructs keys) to pass the orientation value obtained once in
getParameters(), and keep the existing behavior: if orientation equals
"Vertical" add 12 * groupNum to pitch when constructing a new JankoKey,
otherwise pass pitch unchanged; this removes the hidden mutation from
HexKey.getKeyOrientation and avoids inconsistent reads.
| if (p.groupSize > 0 && orientation.equals("Vertical")) { | ||
| int groupCount = columnCount / p.groupSize; | ||
| if (columnCount % p.groupSize > 0) groupCount++; | ||
| // Should we assume Janko-like logic for pitchPre? | ||
| // "Generalize options". If groupSize is used, pitchPre usually needs adjustment to center or start correct? | ||
| // Janko logic: pitchPre = -(groupCount - 1) * 12. | ||
| // Custom user sets steps. They might not expect this automatic offset. | ||
| // But if they use grouping, they probably want multiple octaves side-by-side. | ||
| // Let's NOT touch pitchPre automatically for Custom unless requested. | ||
| // User can set p.pitchPre if we expose it? We didn't expose it. | ||
| // Janko calculates it. | ||
| // Maybe add `customPitchPre`? | ||
| // But that's getting complicated. | ||
| // Let's assume for now pitchPre is 0 for Custom. | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove or implement the commented-out pitchPre logic.
This block contains extensive comments about potential pitchPre adjustments for Custom layout grouping but no actual implementation. The comments discuss Janko-like behavior but conclude with "Let's assume for now pitchPre is 0 for Custom."
If this functionality is intentionally deferred, consider:
- Creating a TODO/issue to track the feature
- Removing the verbose comments to reduce noise
🤖 Prompt for AI Agents
In `@StarChord/src/main/java/opensource/starchord/LayoutStrategies.java` around
lines 186 - 200, The commented-out pitchPre logic should be cleaned up and the
assumed behavior made explicit: inside LayoutStrategies where you check if
p.groupSize > 0 && orientation.equals("Vertical"), remove the long exploratory
comments and either implement the behavior or mark it as TODO; implement the
current assumed behavior by explicitly setting p.pitchPre = 0 (so the code
documents intent) and compute groupCount from columnCount / p.groupSize as
shown, and add a single-line TODO comment referencing that Janko-like centering
is unimplemented (e.g., TODO: consider Janko-like pitchPre adjustment) so the
feature is tracked.
| public static class Parameters { | ||
| public int pitchPre = 0; | ||
| public int pitch1 = 0; | ||
| public int pitch2 = 0; | ||
| public int pitchPost = 0; | ||
| public int groupSize = 0; // 0 or 1 means no grouping, >1 means grouping enabled | ||
| public int initialPitchOffset = 0; | ||
| public int groupJumpPitch = 0; // Pitch change when wrapping a group (e.g. Janko Horizontal) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making Parameters fields final or using a builder.
The Parameters class uses public mutable fields which works for this internal use case, but could lead to accidental modification after creation. For better encapsulation, consider making fields final and initializing via constructor or using a builder pattern.
This is a minor concern given the class is used internally within strategy implementations.
🤖 Prompt for AI Agents
In `@StarChord/src/main/java/opensource/starchord/LayoutStrategy.java` around
lines 7 - 15, Parameters currently exposes mutable public fields (pitchPre,
pitch1, pitch2, pitchPost, groupSize, initialPitchOffset, groupJumpPitch) which
can be accidentally changed after creation; make the class immutable by
declaring these fields final and adding a constructor that initializes all of
them (or implement a simple builder inside Parameters to construct instances
immutably), then update any call sites that instantiate Parameters to use the
new constructor/builder—this preserves internal use while preventing accidental
modification.
| <string name="default_custom_note">C</string> | ||
| <string name="default_custom_octave">3</string> | ||
| <string name="default_custom_key_orientation">Horizontal</string> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full strings.xml file to see all layout defaults
cat -n StarChord/src/main/res/values/strings.xml | head -150Repository: lrq3000/starchord
Length of output: 6941
🏁 Script executed:
# Search for all orientation-related strings
rg -n "orientation" StarChord/src/main/res/values/strings.xmlRepository: lrq3000/starchord
Length of output: 961
🏁 Script executed:
# Search for custom-related preferences in the codebase to understand how orientation is used
rg -n "customLandscape|custom.*orientation|custom.*Orientation" --type=xml --type=javaRepository: lrq3000/starchord
Length of output: 995
Custom layout is missing the default screen orientation string.
Sonome, Jammer, and Janko layouts all define a default_*_orientation string (lines 106-108), but Custom layout lacks this. The Custom layout should include:
<string name="default_custom_orientation">Landscape</string>This is needed to match the pattern established by other layouts and to support proper screen orientation configuration for the Custom layout.
🤖 Prompt for AI Agents
In `@StarChord/src/main/res/values/strings.xml` around lines 112 - 114, Add the
missing default orientation string for the Custom layout by defining the
resource key default_custom_orientation and setting its value to "Landscape";
update the same block that contains default_custom_note, default_custom_octave,
and default_custom_key_orientation so the Custom layout matches the pattern used
by
default_sonome_orientation/default_jammer_orientation/default_janko_orientation.
… Preference Refactored `HexKeyboard` layout generation into `LayoutStrategy` implementations (`Jammer`, `Sonome`, `Janko`, `Custom`). Added `CustomStrategy` and `CustomKey` to support fully customizable tonal steps and grouping. Unified screen orientation preferences into a single `landscape` checkbox in `preferences.xml`. Consolidated Custom layout step preferences to be orientation-agnostic, dynamically mapping to Vertical/Horizontal algorithms based on the global landscape setting. Co-authored-by: lrq3000 <1118942+lrq3000@users.noreply.github.com>
Refactored `HexKeyboard` layout generation into `LayoutStrategy` implementations (`Jammer`, `Sonome`, `Janko`, `Custom`). Added `CustomStrategy` and `CustomKey` to support user-defined tonal steps and grouping. Unified screen orientation (`landscape`) and key orientation (`keyOrientation`) into global preferences, replacing layout-specific settings. Generalized `keyOverlap` (single-touch chording) to be a global option available to all layouts. Refactored `HexKey` subclasses to inherit global preference logic. Co-authored-by: lrq3000 <1118942+lrq3000@users.noreply.github.com>
Refactored `HexKeyboard` layout generation into `LayoutStrategy` implementations (`Jammer`, `Sonome`, `Janko`, `Custom`). Added `CustomStrategy` and `CustomKey` to support user-defined tonal steps and grouping. Unified screen orientation (`landscape`), key orientation (`keyOrientation`), and key overlap (`keyOverlap`) into global preferences. Refactored `HexKey` subclasses to safely inherit global preference logic with safe defaults. Optimized `JankoStrategy` to avoid redundant preference lookups. Implemented parameter immutability using a Builder pattern. Co-authored-by: lrq3000 <1118942+lrq3000@users.noreply.github.com>
1 participant
This PR refactors the keyboard layout generation in
HexKeyboard.javato be generic using a Strategy pattern. It introducesLayoutStrategyinterface and implementations for Jammer, Sonome, Janko, and a new Custom layout. The Custom layout allows users to configure tonal steps in 3 directions (vertical/horizontal steps and row wrapping) and grouping logic (similar to Janko), satisfying the request for a fully customizable layout. Android resources (strings, arrays, preferences) were updated to expose these new options.PR created automatically by Jules for task 5972893957033624005 started by @lrq3000
Summary by CodeRabbit