feat: Replace Scroll label with QWERTY/DVORAK layout indicator on LCD#12
feat: Replace Scroll label with QWERTY/DVORAK layout indicator on LCD#12bavicente9 wants to merge 14 commits intosplitkb:halcyonfrom
Conversation
…ation and empty layers
There was a problem hiding this comment.
Pull request overview
This pull request replaces the "Scroll Lock" indicator on the LCD display with a dynamic keyboard layout indicator that shows whether QWERTY or DVORAK is currently active. The implementation tracks the default layer state (layers 0 and 1 correspond to QWERTY and DVORAK respectively) and displays the active layout name in an underlined/highlighted style. When users switch to higher layers (Nav/Sym layers 2+), the display maintains the last active base layout.
Changes:
- Modified display code to show layout name instead of Scroll Lock status
- Added new
get_layout_name()function to track and return current keyboard layout - Created new
my_displaykeymap for Lily58 with QWERTY/DVORAK switching capability - Updated display refresh logic to respond to default layer state changes
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| users/halcyon_modules/splitkb/hlc_tft_display/hlc_tft_display.c | Implements layout name tracking function and updates display logic to show layout instead of scroll lock |
| qmk.json | Reduced build targets from all keyboards to single Lily58 configuration |
| keyboards/splitkb/halcyon/lily58/keymaps/my_display/rules.mk | New keymap configuration file |
| keyboards/splitkb/halcyon/lily58/keymaps/my_display/readme.md | Documentation for the new keymap |
| keyboards/splitkb/halcyon/lily58/keymaps/my_display/keymap.json | Keymap with QWERTY (layer 0) and DVORAK (layer 1) layouts plus switching keys |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static const char* get_layout_name(void) { | ||
| uint8_t layer = get_highest_layer(default_layer_state); | ||
| static const char* last_layout = "QWERTY"; | ||
|
|
||
| if (layer == 0) { | ||
| last_layout = "QWERTY"; | ||
| } else if (layer == 1) { | ||
| last_layout = "DVORAK"; | ||
| } | ||
| return last_layout; | ||
| } |
There was a problem hiding this comment.
The function get_layout_name uses a static variable to persist the layout name across calls. However, this static variable is initialized inside the function which means it gets initialized to "QWERTY" on every function call rather than just once. While this works, it's redundant and not following typical C patterns. The static initialization should be moved outside the function or you should use a different approach to track the last layout.
| led_usb_state.scroll_lock ? qp_drawtext_recolor(lcd_surface, 5, LCD_HEIGHT - Retron27->line_height - 5, Retron27_underline, scroll, HSV_SCROLL_ON, HSV_BLACK) : qp_drawtext_recolor(lcd_surface, 5, LCD_HEIGHT - Retron27->line_height - 5, Retron27, scroll, HSV_SCROLL_OFF, HSV_BLACK); | ||
|
|
||
| const char* layout = get_layout_name(); | ||
| qp_drawtext_recolor(lcd_surface, 5, LCD_HEIGHT - Retron27->line_height - 5, Retron27_underline, layout, HSV_SCROLL_ON, HSV_BLACK); |
There was a problem hiding this comment.
The layout indicator is always displayed with the underline font (Retron27_underline) and HSV_SCROLL_ON color, making it appear as if it's always "active" or "on". This differs from the Caps Lock and Num Lock indicators above it, which toggle between underlined/non-underlined fonts and different colors based on their state. According to the PR description, the layout name should be "underlined/highlighted", but there's no toggle logic based on any state. Consider whether the layout should have an "active" state similar to the lock indicators, or if using a non-underlined font would be more appropriate since it's not a toggle state.
| qp_drawtext_recolor(lcd_surface, 5, LCD_HEIGHT - Retron27->line_height - 5, Retron27_underline, layout, HSV_SCROLL_ON, HSV_BLACK); | |
| uint8_t base_layer = get_highest_layer(default_layer_state); | |
| base_layer != 0 | |
| ? qp_drawtext_recolor(lcd_surface, 5, LCD_HEIGHT - Retron27->line_height - 5, Retron27_underline, layout, HSV_SCROLL_ON, HSV_BLACK) | |
| : qp_drawtext_recolor(lcd_surface, 5, LCD_HEIGHT - Retron27->line_height - 5, Retron27, layout, HSV_SCROLL_OFF, HSV_BLACK); |
|
|
||
| if (layer == 0) { | ||
| last_layout = "QWERTY"; | ||
| } else if (layer == 1) { | ||
| last_layout = "DVORAK"; |
There was a problem hiding this comment.
According to the PR description, when on Nav/Sym layers (layers 2+), the display should keep showing the last used base layout. However, the function get_layout_name only updates last_layout when layers 0 or 1 are active, and returns last_layout for all other cases. This means that if a layer beyond 1 is ever set as the default_layer (not just an overlay layer), the function will return an outdated value. While this may work as intended if layers 2+ are never used as default layers (only as momentary layers), the function doesn't handle the case where an unexpected layer number appears in default_layer_state. Consider adding explicit handling or documentation for this edge case.
| if (layer == 0) { | |
| last_layout = "QWERTY"; | |
| } else if (layer == 1) { | |
| last_layout = "DVORAK"; | |
| if (layer == 0) { | |
| last_layout = "QWERTY"; | |
| } else if (layer == 1) { | |
| last_layout = "DVORAK"; | |
| } else { | |
| // Unexpected default layer (2+). Nav/Sym layers are intended as | |
| // momentary overlays, not default layers. If such a layer is ever | |
| // set as the default, fall back to the primary base layout name | |
| // instead of returning a potentially stale value. | |
| last_layout = "QWERTY"; |
| How do I edit and update the keymap? | ||
| ------------------------------------ | ||
|
|
||
| The `keymap.json` file is generated from the QMK Configurator interface and formatted for better readability in the context of the Ferris keyboard. |
There was a problem hiding this comment.
The readme.md references "Ferris keyboard" in line 52 when describing how to edit the keymap.json file, but this is actually the Lily58 keyboard keymap documentation. This appears to be copied from another keyboard's documentation without proper updates.
| The `keymap.json` file is generated from the QMK Configurator interface and formatted for better readability in the context of the Ferris keyboard. | |
| The `keymap.json` file is generated from the QMK Configurator interface and formatted for better readability in the context of this Lily58 keyboard. |
| "build_targets": [ | ||
| [ | ||
| "splitkb/halcyon/kyria/rev4", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_TFT_DISPLAY": "1", | ||
| "TARGET": "splitkb_halcyon_kyria_rev4_vial_hlc_display" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/kyria/rev4", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_ENCODER": "1", | ||
| "TARGET": "splitkb_halcyon_kyria_rev4_vial_hlc_encoder" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/kyria/rev4", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_CIRQUE_TRACKPAD": "1", | ||
| "TARGET": "splitkb_halcyon_kyria_rev4_vial_hlc_cirque_trackpad" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/kyria/rev4", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_NONE": "1", | ||
| "TARGET": "splitkb_halcyon_kyria_rev4_vial_hlc" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/elora/rev2", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_TFT_DISPLAY": "1", | ||
| "TARGET": "splitkb_halcyon_elora_rev2_vial_hlc_display" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/elora/rev2", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_ENCODER": "1", | ||
| "TARGET": "splitkb_halcyon_elora_rev2_vial_hlc_encoder" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/elora/rev2", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_CIRQUE_TRACKPAD": "1", | ||
| "TARGET": "splitkb_halcyon_elora_rev2_vial_hlc_cirque_trackpad" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/elora/rev2", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_NONE": "1", | ||
| "TARGET": "splitkb_halcyon_elora_rev2_vial_hlc" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/corne/rev2", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_TFT_DISPLAY": "1", | ||
| "TARGET": "splitkb_halcyon_corne_rev2_vial_hlc_display" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/corne/rev2", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_ENCODER": "1", | ||
| "TARGET": "splitkb_halcyon_corne_rev2_vial_hlc_encoder" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/corne/rev2", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_CIRQUE_TRACKPAD": "1", | ||
| "TARGET": "splitkb_halcyon_corne_rev2_vial_hlc_cirque_trackpad" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/corne/rev2", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_NONE": "1", | ||
| "TARGET": "splitkb_halcyon_corne_rev2_vial_hlc" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/lily58/rev2", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_TFT_DISPLAY": "1", | ||
| "TARGET": "splitkb_halcyon_lily58_rev2_vial_hlc_display" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/lily58/rev2", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_ENCODER": "1", | ||
| "TARGET": "splitkb_halcyon_lily58_rev2_vial_hlc_encoder" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/lily58/rev2", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_CIRQUE_TRACKPAD": "1", | ||
| "TARGET": "splitkb_halcyon_lily58_rev2_vial_hlc_cirque_trackpad" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/lily58/rev2", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_NONE": "1", | ||
| "TARGET": "splitkb_halcyon_lily58_rev2_vial_hlc" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/ferris/rev1", | ||
| "vial_hlc", | ||
| "my_display", | ||
| { | ||
| "HLC_TFT_DISPLAY": "1", | ||
| "TARGET": "splitkb_halcyon_ferris_rev1_vial_hlc_display" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/ferris/rev1", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_ENCODER": "1", | ||
| "TARGET": "splitkb_halcyon_ferris_rev1_vial_hlc_encoder" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/ferris/rev1", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_CIRQUE_TRACKPAD": "1", | ||
| "TARGET": "splitkb_halcyon_ferris_rev1_vial_hlc_cirque_trackpad" | ||
| } | ||
| ], | ||
| [ | ||
| "splitkb/halcyon/ferris/rev1", | ||
| "vial_hlc", | ||
| { | ||
| "HLC_NONE": "1", | ||
| "TARGET": "splitkb_halcyon_ferris_rev1_vial_hlc" | ||
| "TARGET": "splitkb_halcyon_lily58_rev2_my_display_dual" | ||
| } | ||
| ] | ||
|
|
||
| ] | ||
| } |
There was a problem hiding this comment.
The qmk.json file has been drastically reduced from containing all build targets (Kyria, Elora, Corne, Lily58, Ferris with various configurations) to only a single build target for Lily58 with the my_display keymap. This appears to be an unintended change that would break the build system for all other keyboard configurations. This change should either be reverted or explained in the PR description if intentional.
| First, from the root of your qmk repo, move to your keymap folder | ||
|
|
||
| ```bash | ||
| cd ./keymaps/splitkb/aurora/lily58/my_personal_keymap |
There was a problem hiding this comment.
The path referenced in line 78 points to 'splitkb/aurora/lily58' but should point to 'splitkb/halcyon/lily58' based on the actual keyboard path used in this codebase.
|
|
||
| If you decide to use the latter workflow, here are the steps to follow: | ||
|
|
||
| * From the [QMK Configurator](https://config.qmk.fm/#/splitkb/aurora/lily58/rev1/LAYOUT), hit the "import QMK keymap json file" button (it has a drawing with an up arrow on it). |
There was a problem hiding this comment.
The URL in line 60 references 'splitkb/aurora/lily58/rev1' in the QMK Configurator, but based on the keyboard configuration in keymap.json (line 2), this should reference the Halcyon Lily58 keyboard instead. The correct keyboard identifier should match what's actually being configured.
| If you decide to use the latter workflow, here are the steps to follow: | ||
|
|
||
| * From the [QMK Configurator](https://config.qmk.fm/#/splitkb/aurora/lily58/rev1/LAYOUT), hit the "import QMK keymap json file" button (it has a drawing with an up arrow on it). | ||
| * Browse to the location of your keymap (for example, `<your qmk repo>/keyboards/splitkb/aurora/lily58/keymaps/default/keymap.json`) |
There was a problem hiding this comment.
The path in line 61 references 'splitkb/aurora/lily58' but should reference 'splitkb/halcyon/lily58' to match the actual keyboard path in this codebase.
1 participant
Summary