Repository navigation
Persist preset names in Krokiet on Save Current Preset click - #1940
Open
johntrandall wants to merge 1 commit into
Open
johntrandall wants to merge 1 commit into
johntrandall wants to merge 1 commit into
Conversation
The Save Current Preset button writes the per-preset config file (config_preset_N.json) but never the base settings file (config_general.json). Preset names live in BasicSettings and are only flushed on clean app exit via save_all_settings_to_file. On a non-clean exit (window close that returns non-Ok, logger that did not flush, force quit) preset renames silently vanish on next launch. on_save_current_preset now writes both files, matching the expectation that Save means save everything visible — including any preset rename just made through the Edit name dialog. Adds a round-trip unit test for BasicSettings to lock in the data path the new call exercises. The GUI wiring itself was verified manually on macOS 15.7 (rename → Save → force-kill → relaunch; with the fix the rename persists, without it config_general.json is unchanged). Related: qarmin#1225 (closed 2024-02) was a previous preset-persistence bug, fixed for the per-preset config path. The base settings file (preset names) was not covered by that fix. Claude-Session-Id: a4668bec-f120-447b-8c91-4ce97a6ca187 Resume: claude --resume a4668bec-f120-447b-8c91-4ce97a6ca187
johntrandall
force-pushed
the
fix/save-preset-names-on-button-press
branch
from
May 12, 2026 20:21
03d4d02 to
a7ce6bf
Compare
qarmin
reviewed
May 14, 2026
| // on this round-trip: edit preset_names in memory → save_data_to_file → load_data_from_file | ||
| // must return the edited names. Without it, renaming a preset cannot survive a restart. | ||
| #[test] | ||
| fn basic_settings_round_trips_renamed_preset_names() { |
Owner
There was a problem hiding this comment.
This test don't do anything except saving data to file and reading it via fs and serde options, so it doesn't check almost any logic that could regress in future(this items are quite hide to broke) and this test isn't related in any way with this PR - please remove it.
qarmin
reviewed
May 14, 2026
| } | ||
| } | ||
| // Also persist base settings (preset names, default preset, window prefs). | ||
| // Without this, renaming a preset only takes effect after a clean app exit, |
Owner
There was a problem hiding this comment.
This code doesn't need a comment(it is easy to deduce, what this do) so comment needs to be removed
This branch has not been deployed
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
2 participants
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.
Problem
Krokiet's Save Current Preset button writes the per-preset config file (
config_preset_N.json) but does not write the base settings file (config_general.json). Preset names live inBasicSettingsand are only flushed on clean app exit viasave_all_settings_to_file(). On a non-clean exit (window close that returns non-Ok, logger that did not flush, force quit), preset renames silently vanish on next launch.Repro (Krokiet 11.0.1, macOS 15.7)
Fix
on_save_current_presetnow callssave_base_settings_to_filein addition to the existing per-preset write, so the Save button matches user expectations — Save means save everything visible, including any preset rename just made through the Edit name dialog.Adds a round-trip unit test for
BasicSettingsinkrokiet/src/settings/mod.rsto lock in the data path the new call exercises.Verification
cargo test -p krokiet— passes (new testbasic_settings_round_trips_renamed_preset_names)cargo +nightly fmt+cargo clippy --all-targets -- -D warnings— cleanPreset 1→TEST-FIX-VERIFY 1in the Edit name dialogconfig_general.jsonmtime updates,preset_names[0] = "TEST-FIX-VERIFY 1"pkill -9(worst-case force-quit) → file content survivesTEST-FIX-VERIFY 1config_general.jsonunchanged (still defaults).Related
Scope
Intentionally minimal: one new call inside the existing callback, plus one unit test. The narrower follow-up — saving base settings directly from the Edit name dialog's own Save button, so rename-then-force-quit-without-clicking-Save-Current-Preset doesn't lose the rename — is out of scope here. That would require a new
Callablercallback and wiring it fromkrokiet/ui/settings_list.slint.