Add(nvim-setting): Support for leader key #124
Add(nvim-setting): Support for leader key #124erifranck wants to merge 4 commits intoGentleman-Programming:mainfrom
Conversation
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Hey @erifranck! The leader key concept is solid — letting veteran Vim users pick , or \ during installation is genuinely useful. Good work on the navigation flow and the proceedToBackupOrInstall() refactor 👏
However, there are 3 blocking issues that need fixing before this can be merged, plus a couple of design concerns.
🔴 Blocking Issues
1. iota shift breaks Trainer screen constants (model.go:52-54)
The new ScreenExperienceSelect and ScreenLeaderKeySelect are inserted in the middle of the const() iota block — before ScreenTrainerMenu. This silently shifts the numeric value of EVERY Trainer screen by +2.
Fix: Move the new constants to the end of the const() block (after ScreenTrainerBossResult), or at minimum after all Trainer screens.
2. TestFullInstallationFlow is broken (integration_test.go)
The existing integration test selects "Yes" for Nvim and immediately expects ScreenBackupConfirm or ScreenInstalling. But your PR adds two intermediate screens (ExperienceSelect → LeaderKeySelect) that the test never navigates through.
Fix: Update the test to navigate through the new screens:
// After selecting "Yes" for Nvim:
result, _ = m.Update(tea.KeyMsg{Type: tea.KeyEnter}) // → ScreenExperienceSelect
m = result.(Model)
result, _ = m.Update(tea.KeyMsg{Type: tea.KeyEnter}) // → ScreenLeaderKeySelect
m = result.(Model)
result, _ = m.Update(tea.KeyMsg{Type: tea.KeyEnter}) // → BackupConfirm or Installing
m = result.(Model)3. Screen.String() not updated (comprehensive_test.go:1561)
The String() method only knows 22 screen names but there are ~38 screens now. Everything past position 22 returns "Unknown" — which hides bugs in test output. Add the missing screen names including your two new ones.
🟡 Design Concerns
4. Experience field is dead code
The ScreenExperienceSelect screen asks the user for their experience level but it's only written as a Lua comment (-- Experience level: beginner). It doesn't influence which plugins are installed, which keymaps are configured, or any behavior.
Suggestion: Either remove the Experience screen entirely (keep the PR focused on leader key), or explain what it will be used for in a follow-up. As-is, it asks users a question and does nothing with the answer — that's bad UX.
5. Re-install duplicates leader config in options.lua
writeNvimLeaderConfig() prepends to options.lua without checking if a leader block already exists. Running the installer twice produces duplicate blocks. Lua uses the last definition, so the OLD value wins (it's at the bottom).
Fix: Check for the marker comment -- Leader key configuration (Generated by Gentleman.Dots) before prepending. If it exists, replace the block instead of duplicating.
✅ What's good
- Navigation flow and back-nav are clean
proceedToBackupOrInstall()extraction is a good refactor- Default
spaceleader is the right choice - Tests for the new screens are well structured
- No merge conflicts with main
Fix the 3 red flags and I'll re-review. The Experience screen removal would simplify this PR a lot — you could always add it back when it actually does something. Thanks! 🎩
Feat: Multiple Leader Key Support and Flash.nvim Keybinding Adjustment
This Pull Request introduces two coordinated changes to improve customization and avoid keybinding conflicts:
Motivation for the change:
The primary motivation is to resolve a keybinding conflict. By adding support for the comma () as a Leader Key in the installer, it directly conflicted with the navigation key used by .
To address this, the conflicting navigation key has been remapped to , maintaining the plugin's functionality while allowing the use of as a Leader Key without issues. This change ensures greater flexibility in personal configuration without sacrificing the usability of plugins.