From 07bbd45a4e2820d62f6ea3f091af83e2abb29b35 Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Tue, 13 Oct 2026 17:54:46 +0800 Subject: [PATCH] Ask on Every Start and Take the User Presets Lock Once When Migrating Filament Presets The startup prompt no longer depends on the version the previous run recorded: whoever still has user filament presets that lost printers is asked, including users already on a 2.5.0 nightly or alpha. Answering No asks again at the next start; "Don't remind me again" stops it. Migration takes the user presets lock once, around the backup and every file write, instead of once per preset. A lock another instance holds is reported once ("another OrcaSlicer instance is saving presets") with nothing written, rather than as one failed preset after another once the lock's cool-down kicks in. Detection records each preset's file and the list it will get, so the writes need no collection lock while the user presets lock is held, the order InstanceLock requires; the loaded presets are updated afterwards. --- src/libslic3r/Preset.cpp | 75 ++++++++----------- src/libslic3r/Preset.hpp | 14 +++- src/libslic3r/PresetBundle.cpp | 34 ++++++--- src/libslic3r/PresetBundle.hpp | 13 +++- src/slic3r/GUI/GUI_App.cpp | 15 ++-- src/slic3r/GUI/GUI_App.hpp | 4 +- .../libslic3r/test_preset_bundle_loading.cpp | 51 ++++++++----- 7 files changed, 118 insertions(+), 88 deletions(-) diff --git a/src/libslic3r/Preset.cpp b/src/libslic3r/Preset.cpp index 2a64bed1db..f4e6de9b76 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -2786,7 +2786,7 @@ std::vector PresetCollection::legacy_user_filament_pre // files of presets whose parent has an entry under one of those are read. if (!has_entry(parent->name) && std::none_of(parent->renamed_from.begin(), parent->renamed_from.end(), has_entry)) continue; - LegacyUserFilamentPreset legacy{preset.name, parent->name, {}, {}}; + LegacyUserFilamentPreset legacy{preset.name, parent->name, {}, {}, preset.file, own}; try { const json j = read_user_preset_file(preset.file); // A preset whose file sets compatible_printers, even to an empty list, was given its printers on @@ -2802,8 +2802,10 @@ std::vector PresetCollection::legacy_user_filament_pre if (it == printers_by_parent.end()) continue; for (const std::string &printer : it->second) - if (std::find(own.begin(), own.end(), printer) == own.end()) + if (std::find(own.begin(), own.end(), printer) == own.end()) { legacy.printers.push_back(printer); + legacy.compatible_printers.push_back(printer); + } if (!legacy.printers.empty()) legacy_presets.push_back(std::move(legacy)); } @@ -2811,61 +2813,47 @@ std::vector PresetCollection::legacy_user_filament_pre return legacy_presets; } -bool PresetCollection::migrate_legacy_user_filament_preset(const LegacyUserFilamentPreset &legacy) +bool PresetCollection::write_legacy_user_filament_preset(const LegacyUserFilamentPreset &legacy) { - // Copied out under the lock: the preset sync thread can delete presets, which invalidates references into - // m_presets, while the file is written. - lock(); - auto it = this->find_preset_internal(legacy.preset); - if (it == m_presets.end() || it->name != legacy.preset || !it->is_user()) { - unlock(); - return false; - } - const std::string file = it->file; - std::vector printers = it->config.option("compatible_printers")->values; - unlock(); - for (const std::string &printer : legacy.printers) - if (std::find(printers.begin(), printers.end(), printer) == printers.end()) - printers.push_back(printer); - // Only compatible_printers changes in the file. Saving the whole preset would also rewrite an inherits that // names a renamed parent by its old name, the only name older releases know it by. - { - const std::string lock_path = user_presets_lock_path(); - InstanceLock instance_lock(lock_path); - // Without the lock another instance could save between the read and the write. Migrating can wait. - if (!lock_path.empty() && !instance_lock.locked()) + try { + json j = read_user_preset_file(legacy.file); + // Changed by hand or by another instance since the preset was found: a list of its own stands, and + // another parent would need other printers. + if (j.contains("compatible_printers") || j.value(BBL_JSON_KEY_INHERITS, std::string()) != legacy.inherits) return false; - try { - json j = read_user_preset_file(file); - // Changed by hand or by another instance since the preset was found: a list of its own stands, and - // another parent would need other printers. - if (j.contains("compatible_printers") || j.value(BBL_JSON_KEY_INHERITS, std::string()) != legacy.inherits) - return false; - j["compatible_printers"] = printers; - if (const std::error_code ec = write_file_atomically(file, j.dump(1, '\t') + "\n")) { - BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": failed to save " << file << ": " << ec.message(); - return false; - } - } catch (const std::exception &err) { - BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": can not migrate " << file << ": " << err.what(); + j["compatible_printers"] = legacy.compatible_printers; + if (const std::error_code ec = write_file_atomically(legacy.file, j.dump(1, '\t') + "\n")) { + BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": failed to save " << legacy.file << ": " << ec.message(); return false; } + } catch (const std::exception &err) { + BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": can not migrate " << legacy.file << ": " << err.what(); + return false; } + BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << ": user preset " << legacy.preset << " gets " << legacy.printers.size() + << " printer(s) its parent listed in an older release"; + return true; +} +void PresetCollection::apply_legacy_user_filament_presets(const std::vector &written) +{ lock(); - it = this->find_preset_internal(legacy.preset); - if (it != m_presets.end() && it->name == legacy.preset) { - std::vector &saved = it->config.option("compatible_printers", true)->values; + for (const LegacyUserFilamentPreset &legacy : written) { + auto it = this->find_preset_internal(legacy.preset); + if (it == m_presets.end() || it->name != legacy.preset) + continue; // deleted by the preset sync thread meanwhile + std::vector &saved = it->config.option("compatible_printers", true)->values; std::vector &edited = m_edited_preset.config.option("compatible_printers", true)->values; // A change to the list pending in the settings tab stays pending. Without one, the edited copy and the // snapshot unsaved changes are measured against take the written list, so nothing reads as modified. if (m_idx_selected == static_cast(it - m_presets.begin()) && edited == saved) { - edited = printers; + edited = legacy.compatible_printers; if (m_saved_preset.name == it->name) - m_saved_preset.config.option("compatible_printers", true)->values = printers; + m_saved_preset.config.option("compatible_printers", true)->values = legacy.compatible_printers; } - saved = printers; + saved = legacy.compatible_printers; // Push the change to the cloud copy, as an edit in the settings tab would. if (!it->setting_id.empty()) { it->sync_info = "update"; @@ -2873,9 +2861,6 @@ bool PresetCollection::migrate_legacy_user_filament_preset(const LegacyUserFilam } } unlock(); - BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << ": user preset " << legacy.preset << " gets " << legacy.printers.size() - << " printer(s) its parent listed in an older release"; - return true; } //BBS: validate_preset diff --git a/src/libslic3r/Preset.hpp b/src/libslic3r/Preset.hpp index cdbb5fc7d1..98e7e62e90 100644 --- a/src/libslic3r/Preset.hpp +++ b/src/libslic3r/Preset.hpp @@ -558,7 +558,11 @@ struct LegacyUserFilamentPreset { std::string parent; // The parent's name as the preset file states it: the name the older release knew it by. std::string inherits; + // The printers the preset lost: what the prompt lists. std::vector printers; + std::string file; + // The list the file gets: the parent's current printers plus the lost ones. + std::vector compatible_printers; }; // Collections of presets of the same type (one of the Print, Filament or Printer type). @@ -617,10 +621,12 @@ public: // no longer lists. printers_by_parent is keyed by the parent's name in that release, which is the name // the preset file's inherits still holds. std::vector legacy_user_filament_presets(const std::map> &printers_by_parent); - // Adds the printers to the preset's own compatible_printers and saves the preset to disk. False if the - // preset file sets compatible_printers or names another parent by now, if another instance holds the - // user presets lock, or if the file could not be saved. - bool migrate_legacy_user_filament_preset(const LegacyUserFilamentPreset &legacy); + // Writes legacy.compatible_printers into the preset's file and keeps every other key. The caller holds the + // user presets lock. False if the file has gained a compatible_printers key or another parent since the + // preset was found, or could not be written. + static bool write_legacy_user_filament_preset(const LegacyUserFilamentPreset &legacy); + // Gives the loaded presets the lists their files were just given, and marks cloud copies for upload. + void apply_legacy_user_filament_presets(const std::vector &written); //BBS: get user presets int get_user_presets(PresetBundle *preset_bundle, std::vector &result_presets); void set_sync_info_and_save(std::string name, std::string setting_id, std::string syncinfo, long long update_time, const std::string& user_id); diff --git a/src/libslic3r/PresetBundle.cpp b/src/libslic3r/PresetBundle.cpp index 13a02c80b7..0b00eb35d4 100644 --- a/src/libslic3r/PresetBundle.cpp +++ b/src/libslic3r/PresetBundle.cpp @@ -1101,16 +1101,32 @@ std::vector PresetBundle::legacy_user_filament_presets return filaments.legacy_user_filament_presets(legacy_compatible_printers()); } -std::string PresetBundle::migrate_legacy_user_filament_presets(const std::vector &legacy_presets, std::vector &failed) +PresetBundle::LegacyUserFilamentPresetMigration PresetBundle::migrate_legacy_user_filament_presets(const std::vector &legacy_presets) { - const std::string zip_path = data_dir() + "/filament_preset_migration_" + - Utils::time2str(Utils::TimeZone::utc, Utils::TimeFormat::iso8601Z) + "_backup.zip"; - if (!backup_user_folder_to_zip(zip_path)) - return {}; - for (const LegacyUserFilamentPreset &legacy : legacy_presets) - if (!filaments.migrate_legacy_user_filament_preset(legacy)) - failed.push_back(legacy.preset); - return zip_path; + LegacyUserFilamentPresetMigration result; + std::vector written; + { + // One hold for the backup and every write: no other instance can save a preset in between, and a lock + // the other instance keeps is one outcome rather than a failure per preset (the lock backs off for a + // while after a miss). Held without the collection mutex, the order InstanceLock requires. + const std::string lock_path = user_presets_lock_path(); + InstanceLock instance_lock(lock_path); + if (!lock_path.empty() && !instance_lock.locked()) + return result; + result.locked = true; + const std::string zip_path = data_dir() + "/filament_preset_migration_" + + Utils::time2str(Utils::TimeZone::utc, Utils::TimeFormat::iso8601Z) + "_backup.zip"; + if (!backup_user_folder_to_zip(zip_path)) + return result; + result.backup = zip_path; + for (const LegacyUserFilamentPreset &legacy : legacy_presets) + if (PresetCollection::write_legacy_user_filament_preset(legacy)) + written.push_back(legacy); + else + result.failed.push_back(legacy.preset); + } + filaments.apply_legacy_user_filament_presets(written); + return result; } std::optional PresetBundle::get_filament_by_filament_id(const std::string& filament_id, const std::string& printer_name) const diff --git a/src/libslic3r/PresetBundle.hpp b/src/libslic3r/PresetBundle.hpp index 53d96fcf6d..4a3dd057c2 100644 --- a/src/libslic3r/PresetBundle.hpp +++ b/src/libslic3r/PresetBundle.hpp @@ -382,10 +382,15 @@ public: // User filament presets made against an older release that no longer show on printers their parent // listed then (resources/info/legacy_compatible_printers.json). std::vector legacy_user_filament_presets(); - // Backs up the user preset folder into a zip in data_dir(), then writes each preset's printers into its - // file. Returns the zip's path, or an empty string when the backup failed and nothing was migrated. - // The presets that could not be migrated are added to failed. - std::string migrate_legacy_user_filament_presets(const std::vector &legacy_presets, std::vector &failed); + // Under one hold of the user presets lock, backs up the user preset folder into a zip in data_dir() and writes + // each preset's printers into its file. Nothing is written when the lock is held elsewhere (locked false) or + // the backup failed (backup empty); otherwise failed names the presets whose files could not be written. + struct LegacyUserFilamentPresetMigration { + bool locked{false}; + std::string backup; + std::vector failed; + }; + LegacyUserFilamentPresetMigration migrate_legacy_user_filament_presets(const std::vector &legacy_presets); //BBS: project embedded preset logic PresetsConfigSubstitutions load_project_embedded_presets(std::vector project_presets, ForwardCompatibilitySubstitutionRule substitution_rule); diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index d3eb49aeaf..a1f9abfe0a 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/src/slic3r/GUI/GUI_App.cpp @@ -1109,10 +1109,7 @@ void GUI_App::post_init() } this->check_new_version_sf(); - // Only presets made against 2.4.2 or older, the release legacy_compatible_printers.json is generated - // against, can be missing printers, so the prompt runs only on the first start after one of those. - if (!cw_showed && m_last_config_version && m_last_config_version->valid() - && *m_last_config_version <= Semver(2, 4, 2)) + if (!cw_showed) this->check_legacy_user_filament_presets(false); const auto cloud_provider = get_printer_cloud_provider(); if (is_user_login(cloud_provider) && !app_config->get_stealth_mode()) { @@ -6852,8 +6849,14 @@ void GUI_App::check_legacy_user_filament_presets(bool by_user) if (!migrate) return; - std::vector failed; - const std::string backup = preset_bundle->migrate_legacy_user_filament_presets(legacy_presets, failed); + const PresetBundle::LegacyUserFilamentPresetMigration migration = preset_bundle->migrate_legacy_user_filament_presets(legacy_presets); + if (!migration.locked) { + show_error(mainframe, _L("Another OrcaSlicer instance is saving presets right now, so nothing was migrated. " + "Try again later from Help > Migrate Filament Presets.")); + return; + } + const std::string &backup = migration.backup; + const std::vector &failed = migration.failed; if (backup.empty()) { show_error(mainframe, _L("Your presets could not be backed up, so they were not migrated.")); return; diff --git a/src/slic3r/GUI/GUI_App.hpp b/src/slic3r/GUI/GUI_App.hpp index 7ab9dd58a9..0dcdc8e36e 100644 --- a/src/slic3r/GUI/GUI_App.hpp +++ b/src/slic3r/GUI/GUI_App.hpp @@ -568,8 +568,8 @@ public: void check_new_version(bool show_tips = false, int by_user = 0); void check_new_version_sf(bool show_tips = false, int by_user = 0); // Offers to migrate user filament presets made against an older release that lost printers their parent - // listed then. At startup it runs only on the first start after 2.4.2 or older, unless the user chose not - // to be reminded; by_user also reports when there is nothing to migrate. + // listed then. At startup it asks on every start that finds such presets, unless the user chose not to be + // reminded; by_user also reports when there is nothing to migrate. void check_legacy_user_filament_presets(bool by_user); bool process_network_msg(std::string dev_id, std::string msg); void request_new_version(int by_user); diff --git a/tests/libslic3r/test_preset_bundle_loading.cpp b/tests/libslic3r/test_preset_bundle_loading.cpp index 64fde37396..edba12c2b1 100644 --- a/tests/libslic3r/test_preset_bundle_loading.cpp +++ b/tests/libslic3r/test_preset_bundle_loading.cpp @@ -7329,6 +7329,8 @@ TEST_CASE("User filaments made against an older release that lost printers their CHECK(legacy_presets[0].parent == k1_parent); CHECK(legacy_presets[0].inherits == k1_old_parent); CHECK(legacy_presets[0].printers == std::vector{ "Creality K1C 0.4 nozzle" }); + CHECK(legacy_presets[0].file == (temp_dir.path() / PRESET_FILAMENT_NAME / "Old PETG.json").string()); + CHECK(legacy_presets[0].compatible_printers == std::vector{ "Creality K1C 0.6 nozzle", "Creality K1C 0.4 nozzle" }); } TEST_CASE("Migrating a user filament backs up the user folder to a zip and writes the legacy printers to its file", "[Preset][Bundle]") @@ -7345,11 +7347,12 @@ TEST_CASE("Migrating a user filament backs up the user folder to a zip and write const std::vector legacy_presets = bundle.filaments.legacy_user_filament_presets(map); REQUIRE(legacy_presets.size() == 1); - std::vector failed; - const std::string backup = bundle.migrate_legacy_user_filament_presets(legacy_presets, failed); + const PresetBundle::LegacyUserFilamentPresetMigration migration = bundle.migrate_legacy_user_filament_presets(legacy_presets); + const std::string &backup = migration.backup; + CHECK(migration.locked); REQUIRE_FALSE(backup.empty()); CHECK(std::regex_match(fs::path(backup).filename().string(), std::regex("filament_preset_migration_\\d{8}T\\d{6}Z_backup\\.zip"))); - CHECK(failed.empty()); + CHECK(migration.failed.empty()); const fs::path extracted = temp_dir.path() / "extracted"; REQUIRE(extract_archive_confined(backup, extracted.string())); CHECK(read_file(extracted / PRESET_USER_DIR / "default" / PRESET_FILAMENT_NAME / "My PETG.json") == original); @@ -7361,7 +7364,8 @@ TEST_CASE("Migrating a user filament backs up the user folder to a zip and write const std::vector expected{ "Creality K1C 0.6 nozzle", "Creality K1C 0.4 nozzle" }; CHECK(reloaded.filaments.find_preset("My PETG")->config.option("compatible_printers")->values == expected); CHECK(reloaded.filaments.legacy_user_filament_presets(map).empty()); - CHECK_FALSE(reloaded.filaments.migrate_legacy_user_filament_preset(legacy_presets[0])); + // The file now sets its own list, so the migration found earlier no longer applies to it. + CHECK(reloaded.migrate_legacy_user_filament_presets(legacy_presets).failed == std::vector{ "My PETG" }); } TEST_CASE("Migrating a user filament keeps the old name of its renamed parent", "[Preset][Bundle]") @@ -7376,7 +7380,7 @@ TEST_CASE("Migrating a user filament keeps the old name of its renamed parent", const std::vector legacy_presets = filaments.legacy_user_filament_presets({ { k1_old_parent, { "Creality K1C 0.4 nozzle" } } }); REQUIRE(legacy_presets.size() == 1); - REQUIRE(filaments.migrate_legacy_user_filament_preset(legacy_presets[0])); + REQUIRE(PresetCollection::write_legacy_user_filament_preset(legacy_presets[0])); // An older release knows the parent only by its old name. const std::string saved = read_file(file); CHECK(saved.find("\"inherits\": \"" + k1_old_parent + "\"") != std::string::npos); @@ -7402,27 +7406,37 @@ TEST_CASE("Migrating a user filament leaves a file another instance changed afte write_user_filament(temp_dir.path(), "My PETG", "Generic PETG @System"); } const std::string edited = read_file(file); - CHECK_FALSE(filaments.migrate_legacy_user_filament_preset(legacy_presets[0])); + CHECK_FALSE(PresetCollection::write_legacy_user_filament_preset(legacy_presets[0])); CHECK(read_file(file) == edited); } -TEST_CASE("Migrating a user filament writes nothing while it can not take the user presets lock", "[Preset][Bundle]") +TEST_CASE("Migrating user filaments backs up and writes nothing while it can not take the user presets lock", "[Preset][Bundle]") { - ScopedTemporaryDir temp_dir; - RenameTestFilaments filaments; - add_system_filament(filaments, k1_parent, { "Creality K1C 0.6 nozzle" }); - const fs::path file = write_user_filament(temp_dir.path(), "My PETG", k1_parent); - const std::string original = read_file(file); - load_user_filaments(filaments, temp_dir.path()); + ScopedTemporaryDir temp_dir; + ScopedDataDir data(temp_dir.path()); + const fs::path user_dir = temp_dir.path() / PRESET_USER_DIR / "default"; + const fs::path file = write_user_filament(user_dir, "My PETG", k1_parent); + const std::string original = read_file(file); + PresetBundle bundle; + add_system_filament(bundle.filaments, k1_parent, { "Creality K1C 0.6 nozzle" }); + load_user_filaments(bundle.filaments, user_dir); const std::vector legacy_presets = - filaments.legacy_user_filament_presets({ { k1_parent, { "Creality K1C 0.4 nozzle" } } }); + bundle.filaments.legacy_user_filament_presets({ { k1_parent, { "Creality K1C 0.4 nozzle" } } }); REQUIRE(legacy_presets.size() == 1); - // A folder in the lock file's place can not be opened as the lock. - ScopedDataDir data(temp_dir.path() / "data"); + // A folder in the lock file's place can not be opened as the lock; a fresh data dir, since loading has + // already opened the lock file of the first one. One hold covers the backup and every write, so a lock + // held elsewhere is one outcome, not a failure per preset. + const fs::path other_data = temp_dir.path() / "data"; + ScopedDataDir data_for_migration(other_data); fs::create_directories(fs::path(user_presets_lock_path())); - CHECK_FALSE(filaments.migrate_legacy_user_filament_preset(legacy_presets[0])); + const PresetBundle::LegacyUserFilamentPresetMigration migration = bundle.migrate_legacy_user_filament_presets(legacy_presets); + CHECK_FALSE(migration.locked); + CHECK(migration.backup.empty()); + CHECK(migration.failed.empty()); CHECK(read_file(file) == original); + for (const fs::directory_entry &entry : fs::directory_iterator(other_data)) + CHECK(entry.path().extension() != ".zip"); } TEST_CASE("Migrating the selected user filament keeps a printer list edit pending in the settings tab", "[Preset][Bundle]") @@ -7449,7 +7463,8 @@ TEST_CASE("Migrating the selected user filament keeps a printer list edit pendin expected_edited = edited_printers(); expected_dirty = true; } - REQUIRE(filaments.migrate_legacy_user_filament_preset(legacy_presets[0])); + REQUIRE(PresetCollection::write_legacy_user_filament_preset(legacy_presets[0])); + filaments.apply_legacy_user_filament_presets(legacy_presets); // find_preset() returns the edited copy of the selected preset unless asked for the real one. CHECK(filaments.find_preset("My PETG", false, true)->config.option("compatible_printers")->values == migrated); CHECK(edited_printers() == expected_edited);