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

@@ -2786,7 +2786,7 @@ std::vector<LegacyUserFilamentPreset> 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<LegacyUserFilamentPreset> 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<LegacyUserFilamentPreset> 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<std::string> printers = it->config.option<ConfigOptionStrings>("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<LegacyUserFilamentPreset> &written)
+{
     lock();
-    it = this->find_preset_internal(legacy.preset);
-    if (it != m_presets.end() && it->name == legacy.preset) {
-        std::vector<std::string> &saved = it->config.option<ConfigOptionStrings>("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<std::string> &saved  = it->config.option<ConfigOptionStrings>("compatible_printers", true)->values;
         std::vector<std::string> &edited = m_edited_preset.config.option<ConfigOptionStrings>("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<size_t>(it - m_presets.begin()) && edited == saved) {
-            edited = printers;
+            edited = legacy.compatible_printers;
             if (m_saved_preset.name == it->name)
-                m_saved_preset.config.option<ConfigOptionStrings>("compatible_printers", true)->values = printers;
+                m_saved_preset.config.option<ConfigOptionStrings>("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

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<std::string> printers;
+    std::string              file;
+    // The list the file gets: the parent's current printers plus the lost ones.
+    std::vector<std::string> 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<LegacyUserFilamentPreset> legacy_user_filament_presets(const std::map<std::string, std::vector<std::string>> &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<LegacyUserFilamentPreset> &written);
     //BBS: get user presets
     int  get_user_presets(PresetBundle *preset_bundle, std::vector<Preset> &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);

src/libslic3r/PresetBundle.cpp

@@ -1101,16 +1101,32 @@ std::vector<LegacyUserFilamentPreset> 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<LegacyUserFilamentPreset> &legacy_presets, std::vector<std::string> &failed)
+PresetBundle::LegacyUserFilamentPresetMigration PresetBundle::migrate_legacy_user_filament_presets(const std::vector<LegacyUserFilamentPreset> &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<LegacyUserFilamentPreset> 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<FilamentBaseInfo> PresetBundle::get_filament_by_filament_id(const std::string& filament_id, const std::string& printer_name) const

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<LegacyUserFilamentPreset> 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<LegacyUserFilamentPreset> &legacy_presets, std::vector<std::string> &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<std::string> failed;
+    };
+    LegacyUserFilamentPresetMigration migrate_legacy_user_filament_presets(const std::vector<LegacyUserFilamentPreset> &legacy_presets);
 
     //BBS: project embedded preset logic
     PresetsConfigSubstitutions load_project_embedded_presets(std::vector<Preset*> project_presets, ForwardCompatibilitySubstitutionRule substitution_rule);

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<std::string> 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<std::string> &failed = migration.failed;
     if (backup.empty()) {
         show_error(mainframe, _L("Your presets could not be backed up, so they were not migrated."));
         return;

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);

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<std::string>{ "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<std::string>{ "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<LegacyUserFilamentPreset> legacy_presets = bundle.filaments.legacy_user_filament_presets(map);
     REQUIRE(legacy_presets.size() == 1);
 
-    std::vector<std::string> 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<std::string> expected{ "Creality K1C 0.6 nozzle", "Creality K1C 0.4 nozzle" };
     CHECK(reloaded.filaments.find_preset("My PETG")->config.option<ConfigOptionStrings>("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<std::string>{ "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<LegacyUserFilamentPreset> 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<LegacyUserFilamentPreset> 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<ConfigOptionStrings>("compatible_printers")->values == migrated);
     CHECK(edited_printers() == expected_edited);