From 0e922a4dcb42cc55ef4c649ba49bacbef602641e Mon Sep 17 00:00:00 2001 From: Dave Rayment Date: Thu, 6 Nov 2025 11:55:47 +0000 Subject: [PATCH] [ImageResizer] Fix issue where settings could be changed during a batch resize (#42163) ## Summary of the Pull Request This fixes an issue where the Image Resizer settings were reloaded for every resize operation in a multi-file batch. This could result in inconsistent resize results if the Settings application or the user interacted with the settings and the properties were changed, even temporarily. ## PR Checklist - [x] Closes: #42116, #35114 - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [x] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## Detailed Description of the Pull Request / Additional comments Corrects an issue in `ResizeBatch.Execute()` where `Settings.Default` was used: ```csharp protected virtual void Execute(string file) => new ResizeOperation(file, DestinationDirectory, Settings.Default).Execute(); ``` Unfortunately, `Settings.Default` is poorly named, and is not a constant property. Instead it actually calls `Reload()`, which loads the settings JSON file and re-processes it fully: ```csharp [JsonIgnore] public static Settings Default { get { defaultInstance.Reload(); return defaultInstance; } } ``` If the settings are changed part-way through a resize batch operation (even scrolling through the presets will set `selectedIndex` and save it back to disk), the batch resize operation may switch to a different resize behaviour, giving inconsistent results. The chances of this occurring increase with the length of the batch operation. ### Solution The solution is to set the `Settings` outside of the main loop and use that for every resize operation, which is what this PR does. `Process` is altered to load the `Settings` at the start and pass that to `Execute` on each iteration of the batch loop: ```csharp public IEnumerable Process(Action reportProgress, CancellationToken cancellationToken) { double total = Files.Count; int completed = 0; var errors = new ConcurrentBag(); var settings = Settings.Default; ... Execute(file, settings); ``` `Execute` is updated to accept a `Settings` object instead of using `Settings.Default`: ```csharp protected virtual void Execute(string file, Settings settings) => new ResizeOperation(file, DestinationDirectory, settings).Execute(); ``` ### Additional changes The batch-related unit tests failed after the above change. I updated the Mock `Execute` to reflect the use of the `Settings` parameter. Also, the `Settings` class was unfortunately bound to the WPF UI, and could not be instantiated during unit testing. I've refactored this so it can be instantiated with or without an `App.Current.Dispatcher`. Ther's a new `ReloadCore()` method which just contains the extracted property setting code. None of the unit tests currently use the settings themselves, but at least the capability is now there. I also removed the setting of the `MaxDegreeOfParallelism` option in the `Parallel.For()`. When left at its default, the runtime will use the appropriate number of threads, and the heuristic it uses may not necessarily equal the number of processor cores in the system. ## Validation Steps Performed - Ensured all existing unit tests passed. - Tested with multiple runs of resizing 200 image files while browsing and changing the settings. This was no longer able to alter the ongoing resize operations. - Checked that the settings could still be amended via the Settings app. --------- Co-authored-by: Gordon Lam (SH) --- .../tests/Models/ResizeBatchTests.cs | 6 +- .../imageresizer/ui/Models/ResizeBatch.cs | 15 +++-- .../imageresizer/ui/Properties/Settings.cs | 57 +++++++++++-------- 3 files changed, 46 insertions(+), 32 deletions(-) diff --git a/src/modules/imageresizer/tests/Models/ResizeBatchTests.cs b/src/modules/imageresizer/tests/Models/ResizeBatchTests.cs index 36a17ceb19..f45fc28e6a 100644 --- a/src/modules/imageresizer/tests/Models/ResizeBatchTests.cs +++ b/src/modules/imageresizer/tests/Models/ResizeBatchTests.cs @@ -10,7 +10,7 @@ using System.Collections.Generic; using System.IO; using System.Linq; using System.Threading; - +using ImageResizer.Properties; using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; using Moq.Protected; @@ -101,7 +101,9 @@ namespace ImageResizer.Models private static ResizeBatch CreateBatch(Action executeAction) { var mock = new Mock { CallBase = true }; - mock.Protected().Setup("Execute", ItExpr.IsAny()).Callback(executeAction); + mock.Protected() + .Setup("Execute", ItExpr.IsAny(), ItExpr.IsAny()) + .Callback((string file, Settings settings) => executeAction(file)); return mock.Object; } diff --git a/src/modules/imageresizer/ui/Models/ResizeBatch.cs b/src/modules/imageresizer/ui/Models/ResizeBatch.cs index 1181395c09..87e0b84e7b 100644 --- a/src/modules/imageresizer/ui/Models/ResizeBatch.cs +++ b/src/modules/imageresizer/ui/Models/ResizeBatch.cs @@ -87,9 +87,14 @@ namespace ImageResizer.Models public IEnumerable Process(Action reportProgress, CancellationToken cancellationToken) { double total = Files.Count; - var completed = 0; + int completed = 0; var errors = new ConcurrentBag(); + // NOTE: Settings.Default is captured once before parallel processing. + // Any changes to settings on disk during this batch will NOT be reflected until the next batch. + // This improves performance and predictability by avoiding repeated mutex acquisition and behaviour change results in a batch. + var settings = Settings.Default; + // TODO: If we ever switch to Windows.Graphics.Imaging, we can get a lot more throughput by using the async // APIs and a custom SynchronizationContext Parallel.ForEach( @@ -97,13 +102,12 @@ namespace ImageResizer.Models new ParallelOptions { CancellationToken = cancellationToken, - MaxDegreeOfParallelism = Environment.ProcessorCount, }, (file, state, i) => { try { - Execute(file); + Execute(file, settings); } catch (Exception ex) { @@ -111,14 +115,13 @@ namespace ImageResizer.Models } Interlocked.Increment(ref completed); - reportProgress(completed, total); }); return errors; } - protected virtual void Execute(string file) - => new ResizeOperation(file, DestinationDirectory, Settings.Default).Execute(); + protected virtual void Execute(string file, Settings settings) + => new ResizeOperation(file, DestinationDirectory, settings).Execute(); } } diff --git a/src/modules/imageresizer/ui/Properties/Settings.cs b/src/modules/imageresizer/ui/Properties/Settings.cs index debb26a191..0f8690dcbb 100644 --- a/src/modules/imageresizer/ui/Properties/Settings.cs +++ b/src/modules/imageresizer/ui/Properties/Settings.cs @@ -461,33 +461,42 @@ namespace ImageResizer.Properties { } - // Needs to be called on the App UI thread as the properties are bound to the UI. - App.Current.Dispatcher.Invoke(() => + if (App.Current?.Dispatcher != null) { - ShrinkOnly = jsonSettings.ShrinkOnly; - Replace = jsonSettings.Replace; - IgnoreOrientation = jsonSettings.IgnoreOrientation; - RemoveMetadata = jsonSettings.RemoveMetadata; - JpegQualityLevel = jsonSettings.JpegQualityLevel; - PngInterlaceOption = jsonSettings.PngInterlaceOption; - TiffCompressOption = jsonSettings.TiffCompressOption; - FileName = jsonSettings.FileName; - KeepDateModified = jsonSettings.KeepDateModified; - FallbackEncoder = jsonSettings.FallbackEncoder; - CustomSize = jsonSettings.CustomSize; - SelectedSizeIndex = jsonSettings.SelectedSizeIndex; - - if (jsonSettings.Sizes.Count > 0) - { - Sizes.Clear(); - Sizes.AddRange(jsonSettings.Sizes); - - // Ensure Ids are unique and handle missing Ids - IdRecoveryHelper.RecoverInvalidIds(Sizes); - } - }); + // Needs to be called on the App UI thread as the properties are bound to the UI. + App.Current.Dispatcher.Invoke(() => ReloadCore(jsonSettings)); + } + else + { + ReloadCore(jsonSettings); + } _jsonMutex.ReleaseMutex(); } + + private void ReloadCore(Settings jsonSettings) + { + ShrinkOnly = jsonSettings.ShrinkOnly; + Replace = jsonSettings.Replace; + IgnoreOrientation = jsonSettings.IgnoreOrientation; + RemoveMetadata = jsonSettings.RemoveMetadata; + JpegQualityLevel = jsonSettings.JpegQualityLevel; + PngInterlaceOption = jsonSettings.PngInterlaceOption; + TiffCompressOption = jsonSettings.TiffCompressOption; + FileName = jsonSettings.FileName; + KeepDateModified = jsonSettings.KeepDateModified; + FallbackEncoder = jsonSettings.FallbackEncoder; + CustomSize = jsonSettings.CustomSize; + SelectedSizeIndex = jsonSettings.SelectedSizeIndex; + + if (jsonSettings.Sizes.Count > 0) + { + Sizes.Clear(); + Sizes.AddRange(jsonSettings.Sizes); + + // Ensure Ids are unique and handle missing Ids + IdRecoveryHelper.RecoverInvalidIds(Sizes); + } + } } }