From dc066970e389c94d8893622fe1beb784078f8395 Mon Sep 17 00:00:00 2001 From: OZAWA Sakuro <10973+sakuro@users.noreply.github.com> Date: Thu, 26 Feb 2026 11:00:58 +0900 Subject: [PATCH 1/2] :sparkles: Show combined sync plan and confirm once in mod sync - Plan all changes (install, enable/disable, conflict resolve, unlisted disable) before showing them together, then ask for confirmation once - Skip confirmation entirely when there is nothing to change - Extract plan_existing_mod_changes, plan_expansion_mod_changes, and apply_single_change helpers to reduce nesting and block length --- lib/factorix/cli/commands/mod/sync.rb | 303 ++++++++++++++------ spec/factorix/cli/commands/mod/sync_spec.rb | 83 +++++- 2 files changed, 282 insertions(+), 104 deletions(-) diff --git a/lib/factorix/cli/commands/mod/sync.rb b/lib/factorix/cli/commands/mod/sync.rb index 1ffabbd..62d5619 100644 --- a/lib/factorix/cli/commands/mod/sync.rb +++ b/lib/factorix/cli/commands/mod/sync.rb @@ -39,15 +39,14 @@ class Sync < Base # # @param save_file [String] Path to save file # @param jobs [Integer] Number of parallel downloads + # @param keep_unlisted [Boolean] Whether to keep unlisted MODs enabled # @return [void] def call(save_file:, jobs: "4", keep_unlisted: false, **) jobs = Integer(jobs) - # Load save file say "Loading save file: #{save_file}", prefix: :info save_data = SaveFile.load(Pathname(save_file)) say "Loaded save file (version: #{save_data.version}, MOD(s): #{save_data.mods.size})", prefix: :info - # Load current state mod_list = MODList.load presenter = Progress::Presenter.new(title: "\u{1F50D}\u{FE0E} Scanning MOD(s)", output: err) handler = Progress::ScanHandler.new(presenter) @@ -56,37 +55,30 @@ def call(save_file:, jobs: "4", keep_unlisted: false, **) raise DirectoryNotFoundError, "MOD directory does not exist: #{runtime.mod_dir}" unless runtime.mod_dir.exist? - # Find MODs that need to be installed + # Plan phase (no side effects) mods_to_install = find_mods_to_install(save_data.mods, installed_mods) + install_targets = mods_to_install.any? ? plan_installation(mods_to_install, graph, jobs) : [] + conflict_mods = find_conflict_mods(mod_list, save_data.mods, graph) + changes = plan_mod_list_changes(mod_list, save_data.mods) + unlisted_mods = keep_unlisted ? [] : find_unlisted_mods(mod_list, save_data.mods, conflict_mods) - if mods_to_install.any? - say "#{mods_to_install.size} MOD(s) need to be installed", prefix: :info + # Show combined plan and ask once + show_sync_plan(install_targets, conflict_mods, changes, unlisted_mods) - # Plan installation - install_targets = plan_installation(mods_to_install, graph, jobs) + return if needs_confirmation?(install_targets, conflict_mods, changes, unlisted_mods) && + !confirm?("Do you want to apply these changes?") - # Show plan - show_install_plan(install_targets) - return unless confirm?("Do you want to install these MOD(s)?") - - # Execute installation + # Execute phase + if install_targets.any? execute_installation(install_targets, jobs) say "Installed #{install_targets.size} MOD(s)", prefix: :success - else - say "All MOD(s) from save file are already installed", prefix: :info end - # Resolve conflicts: disable existing MODs that conflict with new ones - resolve_conflicts(mod_list, save_data.mods, graph) - - # Update mod-list.json - update_mod_list(mod_list, save_data.mods) - disable_unlisted_mods(mod_list, save_data.mods) unless keep_unlisted + apply_mod_list_changes(mod_list, conflict_mods, changes, unlisted_mods) backup_if_exists(runtime.mod_list_path) mod_list.save say "Updated mod-list.json", prefix: :success - # Update mod-settings.dat update_mod_settings(save_data.startup_settings, save_data.version) say "Updated mod-settings.dat", prefix: :success @@ -95,10 +87,8 @@ def call(save_file:, jobs: "4", keep_unlisted: false, **) private def find_mods_to_install(save_mods, installed_mods) save_mods.reject do |mod_name, _mod_state| - # Skip base MOD (always installed) next true if mod_name == "base" - # Check if MOD is installed mod = Factorix::MOD[name: mod_name] installed_mods.any? {|installed| installed.mod == mod } end @@ -111,18 +101,13 @@ def call(save_file:, jobs: "4", keep_unlisted: false, **) # @param jobs [Integer] Number of parallel jobs # @return [Array] Installation targets with MOD info and releases private def plan_installation(mods_to_install, graph, jobs) - # Create progress presenter for info fetching presenter = Progress::Presenter.new(title: "\u{1F50D}\u{FE0E} Fetching MOD info", output: err) - - # Fetch info for MODs to install target_infos = fetch_target_mod_info(mods_to_install, jobs, presenter) - # Add to graph target_infos.each do |info| graph.add_uninstalled_mod(info[:mod_info], info[:release]) end - # Build install targets build_install_targets(target_infos, runtime.mod_dir) end @@ -158,10 +143,7 @@ def call(save_file:, jobs: "4", keep_unlisted: false, **) # @param version [MODVersion] Target version # @return [Hash] {mod_name:, mod_info:, release:, version:} private def fetch_single_mod_info(mod_name, version) - # Fetch full MOD info from portal mod_info = portal.get_mod_full(mod_name) - - # Find the specific version release release = mod_info.releases.find {|r| r.version == version } unless release @@ -171,147 +153,280 @@ def call(save_file:, jobs: "4", keep_unlisted: false, **) {mod_name:, mod_info:, release:, version:} end - # Show the installation plan - # - # @param targets [Array] Installation targets - # @return [void] - private def show_install_plan(targets) - say "Planning to install #{targets.size} MOD(s):", prefix: :info - targets.each do |target| - say " - #{target[:mod]}@#{target[:release].version}" - end - end - # Execute the installation # # @param targets [Array] Installation targets # @param jobs [Integer] Number of parallel jobs # @return [void] private def execute_installation(targets, jobs) - # Download all MODs download_mods(targets, jobs) end - # Resolve conflicts between save file MODs and existing enabled MODs + # Find MODs that conflict with enabled MODs from the save file # # @param mod_list [MODList] Current MOD list # @param save_mods [Hash] MODs from save file # @param graph [Dependency::Graph] Dependency graph - # @return [void] - private def resolve_conflicts(mod_list, save_mods, graph) + # @return [Array] Conflict entries: {mod:, conflicts_with:} + private def find_conflict_mods(mod_list, save_mods, graph) + conflicts = [] + seen = Set.new + save_mods.each do |mod_name, mod_state| next unless mod_state.enabled? - mod = Factorix::MOD[name: mod_name] + save_mod = Factorix::MOD[name: mod_name] - graph.edges_from(mod).each do |edge| + graph.edges_from(save_mod).each do |edge| next unless edge.incompatible? conflicting_mod = edge.to_mod next unless mod_list.exist?(conflicting_mod) && mod_list.enabled?(conflicting_mod) + next unless seen.add?(conflicting_mod) - mod_list.disable(conflicting_mod) - say "Disabled #{conflicting_mod} (conflicts with #{mod} from save file)", prefix: :warn - logger.debug("Disabled conflicting MOD", mod_name: conflicting_mod.name, conflicts_with: mod.name) + conflicts << {mod: conflicting_mod, conflicts_with: save_mod} end - graph.edges_to(mod).each do |edge| + graph.edges_to(save_mod).each do |edge| next unless edge.incompatible? conflicting_mod = edge.from_mod next unless mod_list.exist?(conflicting_mod) && mod_list.enabled?(conflicting_mod) + next unless seen.add?(conflicting_mod) - mod_list.disable(conflicting_mod) - say "Disabled #{conflicting_mod} (conflicts with #{mod} from save file)", prefix: :warn - logger.debug("Disabled conflicting MOD", mod_name: conflicting_mod.name, conflicts_with: mod.name) + conflicts << {mod: conflicting_mod, conflicts_with: save_mod} end end + + conflicts end - # Update mod-list.json with MODs from save file + # Compute changes needed to bring mod-list.json in sync with save file # # @param mod_list [MODList] Current MOD list # @param save_mods [Hash] MODs from save file - # @return [void] - private def update_mod_list(mod_list, save_mods) + # @return [Array] Change entries: {mod:, action:, ...} + private def plan_mod_list_changes(mod_list, save_mods) + changes = [] + save_mods.each do |mod_name, mod_state| mod = Factorix::MOD[name: mod_name] - - # base MOD: don't update version or enabled state - if mod.base? - logger.debug("Skipping base MOD (no changes allowed)", mod_name:) - next - end + next if mod.base? if mod_list.exist?(mod) - # expansion MOD: only update enabled state (not version) - if mod.expansion? - if mod_state.enabled? && !mod_list.enabled?(mod) - mod_list.enable(mod) - logger.debug("Enabled expansion MOD in mod-list.json", mod_name:) - elsif !mod_state.enabled? && mod_list.enabled?(mod) - mod_list.disable(mod) - logger.debug("Disabled expansion MOD in mod-list.json", mod_name:) - end - else - # Regular MOD: update both version and enabled state - # Remove and re-add to update version - mod_list.remove(mod) - mod_list.add(mod, enabled: mod_state.enabled?, version: mod_state.version) - logger.debug("Updated MOD in mod-list.json", mod_name:, version: mod_state.version&.to_s, enabled: mod_state.enabled?) - end + changes.concat(plan_existing_mod_changes(mod_list, mod, mod_state)) else - # Add new entry (version from save file) - mod_list.add(mod, enabled: mod_state.enabled?, version: mod_state.version) - logger.debug("Added to mod-list.json", mod_name:, version: mod_state.version&.to_s) + # Not in list: add it (silently if disabled) + changes << {mod:, action: :add, to_enabled: mod_state.enabled?, to_version: mod_state.version} end end + + changes + end + + # Plan changes for a single MOD that already exists in mod-list.json + # + # @param mod_list [MODList] Current MOD list + # @param mod [MOD] The MOD to plan changes for + # @param mod_state [MODState] MOD state from save file + # @return [Array] Change entries for this MOD + private def plan_existing_mod_changes(mod_list, mod, mod_state) + current_enabled = mod_list.enabled?(mod) + return plan_expansion_mod_changes(mod, mod_state, current_enabled) if mod.expansion? + + current_version = mod_list.version(mod) + to_enabled = mod_state.enabled? + to_version = mod_state.version + enabled_changed = current_enabled != to_enabled + version_changed = current_version != to_version + + if enabled_changed + action = to_enabled ? :enable : :disable + [{mod:, action:, from_version: current_version, to_version:, from_enabled: current_enabled}] + elsif version_changed + [{mod:, action: :update, from_version: current_version, to_version:, from_enabled: current_enabled}] + else + [] + end + end + + # Plan enable/disable changes for an expansion MOD + # + # @param mod [MOD] The expansion MOD + # @param mod_state [MODState] MOD state from save file + # @param current_enabled [Boolean] Current enabled state in mod-list.json + # @return [Array] Change entries (0 or 1 element) + private def plan_expansion_mod_changes(mod, mod_state, current_enabled) + if mod_state.enabled? && !current_enabled + [{mod:, action: :enable}] + elsif !mod_state.enabled? && current_enabled + [{mod:, action: :disable}] + else + [] + end end - # Disable enabled MODs that are not listed in the save file + # Find enabled MODs not listed in the save file (excluding conflict MODs) # # @param mod_list [MODList] Current MOD list # @param save_mods [Hash] MODs from save file + # @param conflict_mods [Array] Already-planned conflict disables + # @return [Array] MODs to disable + private def find_unlisted_mods(mod_list, save_mods, conflict_mods) + conflict_mod_set = Set.new(conflict_mods.map {|c| c[:mod] }) + + mod_list.each_mod.select do |mod| + !mod.base? && + mod_list.enabled?(mod) && + !save_mods.key?(mod.name) && + !conflict_mod_set.include?(mod) + end + end + + # Show the combined sync plan + # + # @param install_targets [Array] MODs to install + # @param conflict_mods [Array] MODs to disable due to conflicts + # @param changes [Array] MOD list changes from save file + # @param unlisted_mods [Array] MODs to disable as unlisted # @return [void] - private def disable_unlisted_mods(mod_list, save_mods) - mod_list.each_mod do |mod| - next if mod.base? - next unless mod_list.enabled?(mod) - next if save_mods.key?(mod.name) + private def show_sync_plan(install_targets, conflict_mods, changes, unlisted_mods) + unless needs_confirmation?(install_targets, conflict_mods, changes, unlisted_mods) + say "Nothing to change", prefix: :info + return + end + + say "Planning to sync MOD(s):", prefix: :info + + if install_targets.any? + say " Install:" + install_targets.each {|t| say " - #{t[:mod]}@#{t[:release].version}" } + end + + enable_changes = changes.select {|c| c[:action] == :enable } + if enable_changes.any? + say " Enable:" + enable_changes.each {|c| say " - #{c[:mod]}" } + end + + disable_changes = changes.select {|c| c[:action] == :disable } + all_disables = conflict_mods.map {|c| {mod: c[:mod], reason: "(conflicts with #{c[:conflicts_with]})"} } + + disable_changes.map {|c| {mod: c[:mod], reason: "(disabled in save file)"} } + + unlisted_mods.map {|m| {mod: m, reason: "(not listed in save file)"} } + if all_disables.any? + say " Disable:" + all_disables.each {|d| say " - #{d[:mod]} #{d[:reason]}" } + end + + update_changes = changes.select {|c| c[:action] == :update } + return if update_changes.none? + say " Update:" + update_changes.each {|c| say " - #{c[:mod]} (#{c[:from_version]} \u2192 #{c[:to_version]})" } + end + + # Apply all mod-list.json changes + # + # @param mod_list [MODList] MOD list to modify + # @param conflict_mods [Array] Conflict entries to disable + # @param changes [Array] MOD list changes + # @param unlisted_mods [Array] Unlisted MODs to disable + # @return [void] + private def apply_mod_list_changes(mod_list, conflict_mods, changes, unlisted_mods) + conflict_mods.each do |conflict| + mod_list.disable(conflict[:mod]) + logger.debug("Disabled conflicting MOD", mod_name: conflict[:mod].name, conflicts_with: conflict[:conflicts_with].name) + end + + changes.each {|change| apply_single_change(mod_list, change) } + + unlisted_mods.each do |mod| mod_list.disable(mod) - say "Disabled #{mod} (not listed in save file)", prefix: :info logger.debug("Disabled unlisted MOD", mod_name: mod.name) end end + # Apply a single change entry to mod-list.json + # + # @param mod_list [MODList] MOD list to modify + # @param change [Hash] Change entry from plan_mod_list_changes + # @return [void] + private def apply_single_change(mod_list, change) + mod = change[:mod] + case change[:action] + when :enable + if mod_list.exist?(mod) + if mod.expansion? + mod_list.enable(mod) + else + mod_list.remove(mod) + mod_list.add(mod, enabled: true, version: change[:to_version]) + end + else + mod_list.add(mod, enabled: true, version: change[:to_version]) + end + logger.debug("Enabled MOD in mod-list.json", mod_name: mod.name) + when :disable + if mod_list.exist?(mod) + if mod.expansion? + mod_list.disable(mod) + else + mod_list.remove(mod) + mod_list.add(mod, enabled: false, version: change[:to_version]) + end + else + mod_list.add(mod, enabled: false, version: change[:to_version]) + end + logger.debug("Disabled MOD in mod-list.json", mod_name: mod.name) + when :update + mod_list.remove(mod) + mod_list.add(mod, enabled: change[:from_enabled], version: change[:to_version]) + logger.debug("Updated MOD in mod-list.json", mod_name: mod.name, version: change[:to_version]&.to_s) + when :add + mod_list.add(mod, enabled: change[:to_enabled], version: change[:to_version]) + logger.debug("Added MOD to mod-list.json", mod_name: mod.name, version: change[:to_version]&.to_s, enabled: change[:to_enabled]) + else + raise ArgumentError, "Unexpected change action: #{change[:action]}" + end + end + # Update mod-settings.dat with startup settings from save file # # @param startup_settings [MODSettings::Section] Startup settings from save file # @param game_version [GameVersion] Game version from save file # @return [void] private def update_mod_settings(startup_settings, game_version) - # Load existing settings or create new mod_settings = if runtime.mod_settings_path.exist? MODSettings.load(runtime.mod_settings_path) else - # Create new MODSettings with all sections sections = MODSettings::VALID_SECTIONS.to_h {|section_name| [section_name, MODSettings::Section.new(section_name)] } MODSettings.new(game_version, sections) end - # Merge startup settings from save file startup_section = mod_settings["startup"] startup_settings.each do |key, value| startup_section[key] = value end - # Save updated settings backup_if_exists(runtime.mod_settings_path) mod_settings.save(runtime.mod_settings_path) end + + # Check whether user-visible changes exist that require confirmation + # + # @param install_targets [Array] MODs to install + # @param conflict_mods [Array] MODs to disable due to conflicts + # @param changes [Array] MOD list changes + # @param unlisted_mods [Array] MODs to disable as unlisted + # @return [Boolean] + private def needs_confirmation?(install_targets, conflict_mods, changes, unlisted_mods) + install_targets.any? || + conflict_mods.any? || + changes.any? {|c| c[:action] != :add || c[:to_enabled] } || + unlisted_mods.any? + end end end end diff --git a/spec/factorix/cli/commands/mod/sync_spec.rb b/spec/factorix/cli/commands/mod/sync_spec.rb index d38351d..ca917f4 100644 --- a/spec/factorix/cli/commands/mod/sync_spec.rb +++ b/spec/factorix/cli/commands/mod/sync_spec.rb @@ -19,6 +19,14 @@ let(:mod_settings_path) { Pathname("/tmp/mod-settings.dat") } let(:game_version) { Factorix::GameVersion.from_string("2.0.72") } + let(:mod_settings) do + Factorix::MODSettings.new( + game_version, + Factorix::MODSettings::VALID_SECTIONS.to_h {|section_name| + [section_name, Factorix::MODSettings::Section.new(section_name)] + } + ) + end let(:base_mod_version) { Factorix::MODVersion.from_string("2.0.72") } let(:save_data) do @@ -87,32 +95,25 @@ allow(mod_dir).to receive(:exist?).and_return(true) allow(logger).to receive(:debug) - # Create a mock MODSettings if needed - mod_settings = Factorix::MODSettings.new( - game_version, - Factorix::MODSettings::VALID_SECTIONS.to_h {|section_name| - [section_name, Factorix::MODSettings::Section.new(section_name)] - } - ) allow(Factorix::MODSettings).to receive_messages(load: mod_settings, new: mod_settings) allow(mod_settings).to receive(:save) allow(mod_settings_path).to receive(:exist?).and_return(true) allow(mod_settings_path).to receive(:rename) - # Stub confirmation to always return true allow(command).to receive(:confirm?).and_return(true) end describe "#call" do - context "when all MODs from save file are already installed" do + context "when mod-list.json is already in sync with the save file" do before do mod_list.add(Factorix::MOD[name: "base"], enabled: true, version: base_mod_version) mod_list.add(Factorix::MOD[name: "test-mod"], enabled: true, version: Factorix::MODVersion.from_string("1.0.0")) end - it "updates mod-list.json" do + it "saves mod-list.json without asking for confirmation" do run_command(command, %W[#{save_file_path}]) + expect(command).not_to have_received(:confirm?) expect(mod_list).to have_received(:save).with(no_args) end end @@ -125,6 +126,12 @@ mod_list.add(Factorix::MOD[name: "space-age"], enabled: true) end + it "asks for confirmation exactly once before applying changes" do + run_command(command, %W[#{save_file_path}]) + + expect(command).to have_received(:confirm?).once + end + it "disables unlisted regular and expansion MODs by default" do run_command(command, %W[#{save_file_path}]) @@ -138,6 +145,62 @@ expect(mod_list.enabled?(Factorix::MOD[name: "extra-mod"])).to be true expect(mod_list.enabled?(Factorix::MOD[name: "space-age"])).to be true end + + context "when user declines confirmation" do + before do + allow(command).to receive(:confirm?).and_return(false) + end + + it "does not save mod-list.json" do + run_command(command, %W[#{save_file_path}]) + + expect(mod_list).not_to have_received(:save) + end + + it "does not update mod-settings.dat" do + run_command(command, %W[#{save_file_path}]) + + expect(mod_settings).not_to have_received(:save) + end + end + end + + context "when there are MODs to install and user declines confirmation" do + let(:fake_release) do + instance_double( + Factorix::API::Release, + version: Factorix::MODVersion.from_string("2.0.0"), + file_name: "new-mod_2.0.0.zip" + ) + end + + before do + fake_targets = [{ + mod: Factorix::MOD[name: "new-mod"], + release: fake_release, + output_path: mod_dir / "new-mod_2.0.0.zip" + }] + allow(command).to receive(:execute_installation) + allow(command).to receive_messages(find_mods_to_install: {"new-mod" => nil}, plan_installation: fake_targets, confirm?: false) + end + + it "does not install MODs" do + run_command(command, %W[#{save_file_path}]) + + expect(command).not_to have_received(:execute_installation) + end + + it "does not save mod-list.json" do + run_command(command, %W[#{save_file_path}]) + + expect(mod_list).not_to have_received(:save) + end + + it "does not update mod-settings.dat" do + run_command(command, %W[#{save_file_path}]) + + expect(mod_settings).not_to have_received(:save) + end end end end From a5e6a554c699d31e8aac33f6d0bdf9f1291814d4 Mon Sep 17 00:00:00 2001 From: OZAWA Sakuro <10973+sakuro@users.noreply.github.com> Date: Thu, 26 Feb 2026 11:21:08 +0900 Subject: [PATCH 2/2] :bug: Skip version update when mod-list.json has no recorded version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a MOD has no version in mod-list.json (nil), treat it as no change rather than a version mismatch — version is optional in mod-list.json and absence means "use whatever is installed". --- lib/factorix/cli/commands/mod/sync.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/factorix/cli/commands/mod/sync.rb b/lib/factorix/cli/commands/mod/sync.rb index 62d5619..35c2ea3 100644 --- a/lib/factorix/cli/commands/mod/sync.rb +++ b/lib/factorix/cli/commands/mod/sync.rb @@ -238,7 +238,7 @@ def call(save_file:, jobs: "4", keep_unlisted: false, **) to_enabled = mod_state.enabled? to_version = mod_state.version enabled_changed = current_enabled != to_enabled - version_changed = current_version != to_version + version_changed = current_version && current_version != to_version if enabled_changed action = to_enabled ? :enable : :disable