mirror of
https://github.com/microsoft/PowerToys.git
synced 2025-12-16 03:37:59 +01:00
[ImageResizer] Fix issue where settings could be changed during a batch resize (#42163)
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## 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. <!-- Please review the items on the PR checklist before submitting--> ## 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 <!-- Provide a more detailed description of the PR, other things fixed, or any additional comments/features here --> ## 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<ResizeError> Process(Action<int, double> reportProgress, CancellationToken cancellationToken) { double total = Files.Count; int completed = 0; var errors = new ConcurrentBag<ResizeError>(); 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. <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## 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) <yeelam@microsoft.com>
This commit is contained in:
@@ -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<string> executeAction)
|
||||
{
|
||||
var mock = new Mock<ResizeBatch> { CallBase = true };
|
||||
mock.Protected().Setup("Execute", ItExpr.IsAny<string>()).Callback(executeAction);
|
||||
mock.Protected()
|
||||
.Setup("Execute", ItExpr.IsAny<string>(), ItExpr.IsAny<Settings>())
|
||||
.Callback((string file, Settings settings) => executeAction(file));
|
||||
|
||||
return mock.Object;
|
||||
}
|
||||
|
||||
@@ -87,9 +87,14 @@ namespace ImageResizer.Models
|
||||
public IEnumerable<ResizeError> Process(Action<int, double> reportProgress, CancellationToken cancellationToken)
|
||||
{
|
||||
double total = Files.Count;
|
||||
var completed = 0;
|
||||
int completed = 0;
|
||||
var errors = new ConcurrentBag<ResizeError>();
|
||||
|
||||
// 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();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -461,8 +461,20 @@ namespace ImageResizer.Properties
|
||||
{
|
||||
}
|
||||
|
||||
if (App.Current?.Dispatcher != null)
|
||||
{
|
||||
// Needs to be called on the App UI thread as the properties are bound to the UI.
|
||||
App.Current.Dispatcher.Invoke(() =>
|
||||
App.Current.Dispatcher.Invoke(() => ReloadCore(jsonSettings));
|
||||
}
|
||||
else
|
||||
{
|
||||
ReloadCore(jsonSettings);
|
||||
}
|
||||
|
||||
_jsonMutex.ReleaseMutex();
|
||||
}
|
||||
|
||||
private void ReloadCore(Settings jsonSettings)
|
||||
{
|
||||
ShrinkOnly = jsonSettings.ShrinkOnly;
|
||||
Replace = jsonSettings.Replace;
|
||||
@@ -485,9 +497,6 @@ namespace ImageResizer.Properties
|
||||
// Ensure Ids are unique and handle missing Ids
|
||||
IdRecoveryHelper.RecoverInvalidIds(Sizes);
|
||||
}
|
||||
});
|
||||
|
||||
_jsonMutex.ReleaseMutex();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user