diff --git a/src/libslic3r/Preset.cpp b/src/libslic3r/Preset.cpp index f4e6de9b76..3f4eb787eb 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -2846,16 +2846,23 @@ void PresetCollection::apply_legacy_user_filament_presets(const std::vector &saved = it->config.option("compatible_printers", true)->values; std::vector &edited = m_edited_preset.config.option("compatible_printers", true)->values; + // Added to the list the preset holds now rather than replacing it: the cloud sync may have given the + // preset a newer list since it was found, and that list is still to be written out. + std::vector printers = saved; + for (const std::string &printer : legacy.printers) + if (std::find(printers.begin(), printers.end(), printer) == printers.end()) + printers.push_back(printer); // 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 = legacy.compatible_printers; + edited = printers; if (m_saved_preset.name == it->name) - m_saved_preset.config.option("compatible_printers", true)->values = legacy.compatible_printers; + m_saved_preset.config.option("compatible_printers", true)->values = 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()) { + saved = printers; + // Push the change to the cloud copy, as an edit in the settings tab would. A sync already pending for + // the preset (a download waiting to be saved, a deletion) keeps its state. + if (!it->setting_id.empty() && it->sync_info.empty()) { it->sync_info = "update"; it->save_info(); } diff --git a/src/libslic3r/PresetBundle.cpp b/src/libslic3r/PresetBundle.cpp index 0b00eb35d4..b339449c9d 100644 --- a/src/libslic3r/PresetBundle.cpp +++ b/src/libslic3r/PresetBundle.cpp @@ -1105,20 +1105,21 @@ PresetBundle::LegacyUserFilamentPresetMigration PresetBundle::migrate_legacy_use { LegacyUserFilamentPresetMigration result; std::vector written; + const std::string zip_path = data_dir() + "/filament_preset_migration_" + + Utils::time2str(Utils::TimeZone::utc, Utils::TimeFormat::iso8601Z) + "_backup.zip"; + // Before the lock: zipping a large folder takes longer than a guard may be held. + if (!backup_user_folder_to_zip(zip_path)) + return result; + result.backup = zip_path; { - // 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. + // One hold for every write, so a lock another 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; each write re-reads its file, so a save that slipped in before is respected. 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); diff --git a/src/libslic3r/PresetBundle.hpp b/src/libslic3r/PresetBundle.hpp index 4a3dd057c2..8da0e0e196 100644 --- a/src/libslic3r/PresetBundle.hpp +++ b/src/libslic3r/PresetBundle.hpp @@ -382,9 +382,9 @@ 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(); - // 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. + // Backs up the user preset folder into a zip in data_dir(), then writes each preset's printers into its file + // under one hold of the user presets lock. Nothing is written when the backup failed (backup empty) or the + // lock could not be taken (locked false); otherwise failed names the presets whose files could not be written. struct LegacyUserFilamentPresetMigration { bool locked{false}; std::string backup; diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index a1f9abfe0a..045bcf2f3c 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/src/slic3r/GUI/GUI_App.cpp @@ -6850,17 +6850,18 @@ void GUI_App::check_legacy_user_filament_presets(bool by_user) return; 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; } + if (!migration.locked) { + // The lock is also refused for a while after a miss, and when the lock file itself cannot be opened. + show_error(mainframe, _L("The user presets folder could not be locked, so nothing was migrated. Another OrcaSlicer " + "instance may be saving presets; try again later from Help > Migrate Filament Presets.")); + return; + } if (failed.size() == legacy_presets.size()) { show_error(mainframe, format_wxstr(_L("Your filament presets could not be migrated. A backup of your presets was saved to:\n%1%"), backup)); return; diff --git a/tests/libslic3r/test_preset_bundle_loading.cpp b/tests/libslic3r/test_preset_bundle_loading.cpp index edba12c2b1..a611ea0a84 100644 --- a/tests/libslic3r/test_preset_bundle_loading.cpp +++ b/tests/libslic3r/test_preset_bundle_loading.cpp @@ -7425,18 +7425,39 @@ TEST_CASE("Migrating user filaments backs up and writes nothing while it can not REQUIRE(legacy_presets.size() == 1); // 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. + // already opened the lock file of the first one. The backup is taken before the lock, then one hold covers + // 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(other_data / PRESET_USER_DIR); + ScopedDataDir data_for_migration(other_data); fs::create_directories(fs::path(user_presets_lock_path())); const PresetBundle::LegacyUserFilamentPresetMigration migration = bundle.migrate_legacy_user_filament_presets(legacy_presets); + CHECK_FALSE(migration.backup.empty()); 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("Applying a migration adds the printers to the list the preset holds now and keeps a pending cloud sync", "[Preset][Bundle]") +{ + ScopedTemporaryDir temp_dir; + RenameTestFilaments filaments; + add_system_filament(filaments, k1_parent, { "Creality K1C 0.6 nozzle" }); + write_user_filament(temp_dir.path(), "My PETG", k1_parent); + load_user_filaments(filaments, temp_dir.path()); + const std::vector legacy_presets = + filaments.legacy_user_filament_presets({ { k1_parent, { "Creality K1C 0.4 nozzle" } } }); + REQUIRE(legacy_presets.size() == 1); + + // Between detection and the write, the cloud sync gave the preset a newer list that is still to be saved. + Preset *preset = filaments.find_preset("My PETG", false, true); + preset->config.option("compatible_printers", true)->values = { "Creality K1C 0.8 nozzle" }; + preset->setting_id = "GFSA01"; + preset->sync_info = "save"; + filaments.apply_legacy_user_filament_presets(legacy_presets); + CHECK(preset->config.option("compatible_printers")->values == + std::vector{ "Creality K1C 0.8 nozzle", "Creality K1C 0.4 nozzle" }); + CHECK(preset->sync_info == "save"); } TEST_CASE("Migrating the selected user filament keeps a printer list edit pending in the settings tab", "[Preset][Bundle]")