From 6bb16d014b99ed04adab5b56244c27c3ca0c2734 Mon Sep 17 00:00:00 2001 From: Niels Laute Date: Wed, 5 Aug 2026 09:02:52 +0200 Subject: [PATCH] [Settings][Image Resizer] Edit/add presets in a ContentDialog instead of a Flyout (#49161) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary of the Pull Request Replaces the inline preset-edit **Flyout** on the Image Resizer settings page with a **ContentDialog**, matching the add/edit pattern already used on the **Color Picker** page. This aligns the experience with the Windows 11 / Fluent paradigm and fixes preset settings being saved on every intermediate change. Editing now happens on a **working copy** of the preset (`ImageSize.Clone()`), which is committed only when the user presses **Save/Update**. As a side effect, the intermediate width/height spinner changes no longer persist `settings.json` / `sizes.json` on every value change — resolving #36938. The per-row **delete** action moves from an inline trash button into a **"..." (More options)** `MenuFlyout`, again matching the Color Picker page. Historically this editing was a Flyout rather than a ContentDialog due to known `ContentDialog` / `XamlRoot` issues back when Settings was a UWP app. Now that Settings is on WinUI 3 / Windows App SDK, `ContentDialog` works reliably (as Color Picker's `ColorFormatDialog` demonstrates), so the original constraint no longer applies. https://github.com/user-attachments/assets/bf71b0a9-c3f8-4078-95c7-c7ee9dc7b24b ## PR Checklist - [x] Closes: #49157 - [x] Closes: #36938 - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [ ] **Tests:** Added/updated and all pass - [x] **Localization:** All end-user-facing strings can be localized (added to `Resources.resw`; reused existing keys for the delete menu) - [ ] **Dev docs:** Added/updated - [x] **New binaries:** None added ## Detailed Description of the Pull Request / Additional comments - **Dialog:** Clicking a preset card (or **Add new size**) opens an `EditSizeDialog` `ContentDialog`. Fields are bound with compiled `{x:Bind}` against a working-copy `EditingSize`. The dimensions field header is dynamic — **"Width"** when height is used, **"Size"** for aspect-ratio-preserving percentage scaling — so the label isn't misleading. The dialog is widened for a less cramped layout. - **Save semantics:** - `ImageSize.Clone()` — builds the editable working copy. - `ImageResizerViewModel.CreateNewImageSizeModel()` — builds a default-valued model for the add dialog without adding it to `Sizes`. - `ImageResizerViewModel.AddImageSize(ImageSize)` — commits a new preset with the next unique ID. - `ImageResizerViewModel.UpdateImageSize(original, updated)` — applies edited values back onto the original, temporarily detaching the per-item `PropertyChanged` save handler so it persists **once** instead of on every property. This is what fixes the "saved too often" behavior in #36938. - **Delete:** per-row `Button` → `MenuFlyout` with a Delete `MenuFlyoutItem`; the `ImageSize` is passed via `CommandParameter="{x:Bind}"` (robust inside a flyout popup) and the Yes/No confirmation dialog is preserved. ## Validation Steps Performed - Built `PowerToys.Settings` (x64/Debug) — clean (exit 0). - Ran the runner from this build and manually validated in Settings → Image Resizer: - **Add new size** opens the dialog pre-filled; Save adds the preset; Cancel discards. - **Editing** a preset in the dialog and pressing Cancel leaves the original untouched (working-copy clone). - Selecting **Percent** shows the **Size** header (not a misleading "Width"). - Spinning width/height inside the dialog no longer writes settings files on each change; a single save occurs on Update (#36938). - The **"..."** menu shows **Delete**, with the confirmation dialog intact. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Settings.UI.Library/ImageSize.cs | 6 + .../ViewModelTests/ImageResizer.cs | 65 +++++++ ...ageResizerSizeToAccessibleTextConverter.cs | 1 - .../SettingsXAML/Views/ImageResizerPage.xaml | 165 +++++++++--------- .../Views/ImageResizerPage.xaml.cs | 130 ++++++++++---- .../Settings.UI/Strings/en-us/Resources.resw | 42 +++-- .../ViewModels/ImageResizerViewModel.cs | 49 +++++- 7 files changed, 320 insertions(+), 138 deletions(-) diff --git a/src/settings-ui/Settings.UI.Library/ImageSize.cs b/src/settings-ui/Settings.UI.Library/ImageSize.cs index c902294f2c..4125e3fa31 100644 --- a/src/settings-ui/Settings.UI.Library/ImageSize.cs +++ b/src/settings-ui/Settings.UI.Library/ImageSize.cs @@ -129,5 +129,11 @@ public partial class ImageSize : INotifyPropertyChanged, IHasId [JsonIgnore] public ImageSize AccessibleTextHelper => this; + /// + /// Creates a copy of this . Used to build a working copy for editing + /// so changes can be discarded (on cancel) without touching the original preset. + /// + public ImageSize Clone() => new ImageSize(_id, _name, _fit, _width, _height, _unit); + public string ToJsonString() => JsonSerializer.Serialize(this); } diff --git a/src/settings-ui/Settings.UI.UnitTests/ViewModelTests/ImageResizer.cs b/src/settings-ui/Settings.UI.UnitTests/ViewModelTests/ImageResizer.cs index 5d30cc433c..7b51b26353 100644 --- a/src/settings-ui/Settings.UI.UnitTests/ViewModelTests/ImageResizer.cs +++ b/src/settings-ui/Settings.UI.UnitTests/ViewModelTests/ImageResizer.cs @@ -240,6 +240,71 @@ namespace ViewModelTests Assert.AreEqual(ResizeUnit.Pixel, newTestSize.Unit); } + [TestMethod] + public void CreateNewImageSizeModelShouldNotAddToCollection() + { + // arrange + var mockSettingsUtils = ISettingsUtilsMocks.GetStubSettingsUtils(); + Func sendMockIPCConfigMSG = msg => { return 0; }; + ImageResizerViewModel viewModel = new ImageResizerViewModel(mockSettingsUtils.Object, SettingsRepository.GetInstance(_mockGeneralSettingsUtils.Object), sendMockIPCConfigMSG, (string name) => name); + int sizeOfOriginalArray = viewModel.Sizes.Count; + + // act + ImageSize workingCopy = viewModel.CreateNewImageSizeModel("New size"); + + // Assert - the working copy is populated but not committed to the collection + Assert.IsNotNull(workingCopy); + Assert.AreEqual("New size 1", workingCopy.Name); + Assert.AreEqual(sizeOfOriginalArray, viewModel.Sizes.Count); + } + + [TestMethod] + public void AddImageSizeWithModelShouldCommitPreparedSize() + { + // arrange + var mockSettingsUtils = ISettingsUtilsMocks.GetStubSettingsUtils(); + Func sendMockIPCConfigMSG = msg => { return 0; }; + ImageResizerViewModel viewModel = new ImageResizerViewModel(mockSettingsUtils.Object, SettingsRepository.GetInstance(_mockGeneralSettingsUtils.Object), sendMockIPCConfigMSG, (string name) => name); + int sizeOfOriginalArray = viewModel.Sizes.Count; + ImageSize workingCopy = viewModel.CreateNewImageSizeModel("New size"); + + // act + viewModel.AddImageSize(workingCopy); + + // Assert + Assert.AreEqual(sizeOfOriginalArray + 1, viewModel.Sizes.Count); + Assert.IsTrue(viewModel.Sizes.Contains(workingCopy)); + } + + [TestMethod] + public void UpdateImageSizeShouldApplyWorkingCopyValuesToOriginal() + { + // arrange + var mockSettingsUtils = ISettingsUtilsMocks.GetStubSettingsUtils(); + Func sendMockIPCConfigMSG = msg => { return 0; }; + ImageResizerViewModel viewModel = new ImageResizerViewModel(mockSettingsUtils.Object, SettingsRepository.GetInstance(_mockGeneralSettingsUtils.Object), sendMockIPCConfigMSG, (string name) => name); + viewModel.AddImageSize("Original"); + ImageSize original = viewModel.Sizes.First(x => x.Id == 0); + + ImageSize edited = original.Clone(); + edited.Name = "Edited"; + edited.Fit = ResizeFit.Stretch; + edited.Width = 320; + edited.Height = 240; + edited.Unit = ResizeUnit.Percent; + + // act + viewModel.UpdateImageSize(original, edited); + + // Assert - the edits are applied to the original preset in place + Assert.AreEqual("Edited", original.Name); + Assert.AreEqual(ResizeFit.Stretch, original.Fit); + Assert.AreEqual(320, original.Width); + Assert.AreEqual(240, original.Height); + Assert.AreEqual(ResizeUnit.Percent, original.Unit); + Assert.AreSame(original, viewModel.Sizes.First(x => x.Id == 0)); + } + [TestMethod] public void DeleteImageSizeShouldDeleteImageSizeWhenSuccessful() { diff --git a/src/settings-ui/Settings.UI/Converters/ImageResizerSizeToAccessibleTextConverter.cs b/src/settings-ui/Settings.UI/Converters/ImageResizerSizeToAccessibleTextConverter.cs index 3fd03b08d6..41224fd498 100644 --- a/src/settings-ui/Settings.UI/Converters/ImageResizerSizeToAccessibleTextConverter.cs +++ b/src/settings-ui/Settings.UI/Converters/ImageResizerSizeToAccessibleTextConverter.cs @@ -26,7 +26,6 @@ public sealed partial class ImageResizerSizeToAccessibleTextConverter : IValueCo private static readonly Dictionary AccessibilityFormats = new() { { "Edit", Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_EditButton_Accessibility_Name") }, - { "Remove", Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_RemoveButton_Accessibility_Name") }, }; private readonly ImageResizerFitToStringConverter _fitConverter = new(); diff --git a/src/settings-ui/Settings.UI/SettingsXAML/Views/ImageResizerPage.xaml b/src/settings-ui/Settings.UI/SettingsXAML/Views/ImageResizerPage.xaml index d8fdda09b5..e6ce9f1712 100644 --- a/src/settings-ui/Settings.UI/SettingsXAML/Views/ImageResizerPage.xaml +++ b/src/settings-ui/Settings.UI/SettingsXAML/Views/ImageResizerPage.xaml @@ -59,9 +59,16 @@ SelectionMode="None"> - + 42 + 0 - - - - - + Content="" + FontFamily="{ThemeResource SymbolThemeFontFamily}" + Style="{StaticResource SubtleButtonStyle}"> + + + + + + + + + @@ -283,6 +228,60 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/settings-ui/Settings.UI/SettingsXAML/Views/ImageResizerPage.xaml.cs b/src/settings-ui/Settings.UI/SettingsXAML/Views/ImageResizerPage.xaml.cs index 6a2068d5a8..9c5e0bf351 100644 --- a/src/settings-ui/Settings.UI/SettingsXAML/Views/ImageResizerPage.xaml.cs +++ b/src/settings-ui/Settings.UI/SettingsXAML/Views/ImageResizerPage.xaml.cs @@ -3,26 +3,58 @@ // See the LICENSE file in the project root for more information. using System; -using System.Globalization; +using System.ComponentModel; +using System.Windows.Input; +using CommunityToolkit.WinUI.Controls; using ManagedCommon; using Microsoft.PowerToys.Settings.UI.Helpers; using Microsoft.PowerToys.Settings.UI.Library; using Microsoft.PowerToys.Settings.UI.ViewModels; using Microsoft.UI.Xaml; using Microsoft.UI.Xaml.Controls; +using Microsoft.Windows.ApplicationModel.Resources; namespace Microsoft.PowerToys.Settings.UI.Views { - public sealed partial class ImageResizerPage : NavigablePage, IRefreshablePage + public sealed partial class ImageResizerPage : NavigablePage, IRefreshablePage, INotifyPropertyChanged { + public event PropertyChangedEventHandler PropertyChanged; + public ImageResizerViewModel ViewModel { get; set; } + public ICommand AddCommand => new RelayCommand(Add); + + public ICommand UpdateCommand => new RelayCommand(Update); + + private readonly ResourceLoader resourceLoader = ResourceLoaderInstance.ResourceLoader; + + // Working copy shown in the edit dialog, bound via x:Bind. Edits happen on this copy so a + // cancel simply discards it; for edits it is a clone of the original, for adds a new model. + private ImageSize _editingSize = new ImageSize(); + + // The original preset being edited, or null when adding a new one. + private ImageSize _editOriginal; + + public ImageSize EditingSize + { + get => _editingSize; + set + { + _editingSize = value; + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(EditingSize))); + } + } + + // Header for the dimensions field. When Height isn't used (e.g. percentage scaling that keeps + // the aspect ratio) the single value scales the whole image, so "Width" would be misleading. + public string GetDimensionHeader(bool isHeightUsed) => + resourceLoader.GetString(isHeightUsed ? "ImageResizer_Dimensions_Width" : "ImageResizer_Dimensions_Size"); + public ImageResizerPage() { InitializeComponent(); var settingsUtils = SettingsUtils.Default; - var resourceLoader = ResourceLoaderInstance.ResourceLoader; Func loader = resourceLoader.GetString; ViewModel = new ImageResizerViewModel(settingsUtils, SettingsRepository.GetInstance(settingsUtils), ShellPage.SendDefaultIPCMessage, loader); @@ -31,49 +63,81 @@ namespace Microsoft.PowerToys.Settings.UI.Views public async void DeleteCustomSize(object sender, RoutedEventArgs e) { - Button deleteRowButton = (Button)sender; - - if (deleteRowButton != null) + if (sender is not MenuFlyoutItem menuItem || menuItem.CommandParameter is not ImageSize size) { - ImageSize x = (ImageSize)deleteRowButton.DataContext; - var resourceLoader = Helpers.ResourceLoaderInstance.ResourceLoader; - - ContentDialog dialog = new ContentDialog(); - dialog.XamlRoot = RootPage.XamlRoot; - dialog.Title = x.Name; - dialog.PrimaryButtonText = resourceLoader.GetString("Yes"); - dialog.CloseButtonText = resourceLoader.GetString("No"); - dialog.DefaultButton = ContentDialogButton.Primary; - dialog.Content = new TextBlock() { Text = resourceLoader.GetString("Delete_Dialog_Description") }; - dialog.PrimaryButtonClick += (s, args) => - { - // Using InvariantCulture since this is internal and expected to be numerical - bool success = int.TryParse(deleteRowButton?.CommandParameter?.ToString(), NumberStyles.Integer, CultureInfo.InvariantCulture, out int rowNum); - if (success) - { - ViewModel.DeleteImageSize(rowNum); - } - else - { - Logger.LogError("Failed to delete custom image size."); - } - }; - var result = await dialog.ShowAsync(); + return; } + + ContentDialog dialog = new ContentDialog(); + dialog.XamlRoot = RootPage.XamlRoot; + dialog.Title = size.Name; + dialog.PrimaryButtonText = resourceLoader.GetString("Yes"); + dialog.CloseButtonText = resourceLoader.GetString("No"); + dialog.DefaultButton = ContentDialogButton.Primary; + dialog.Content = new TextBlock() { Text = resourceLoader.GetString("Delete_Dialog_Description") }; + dialog.PrimaryButtonClick += (s, args) => + { + ViewModel.DeleteImageSize(size.Id); + }; + await dialog.ShowAsync(); } - private void AddSizeButton_Click(object sender, RoutedEventArgs e) + private async void AddSizeButton_Click(object sender, RoutedEventArgs e) { try { - ViewModel.AddImageSize(); + _editOriginal = null; + EditingSize = ViewModel.CreateNewImageSizeModel(); + EditSizeDialog.Title = resourceLoader.GetString("ImageResizer_EditSizeDialog_AddTitle"); + EditSizeDialog.PrimaryButtonText = resourceLoader.GetString("ImageResizer_EditSizeDialog_Save"); + EditSizeDialog.PrimaryButtonCommand = AddCommand; + await EditSizeDialog.ShowAsync(); } catch (Exception ex) { - Logger.LogError("Exception encountered when adding a new image size.", ex); + Logger.LogError("Exception encountered when opening the add image size dialog.", ex); } } + private async void EditSize_Click(object sender, RoutedEventArgs e) + { + if (sender is not SettingsCard card || card.DataContext is not ImageSize original) + { + return; + } + + try + { + // Edit a working copy so changes can be discarded on cancel without touching the original. + _editOriginal = original; + EditingSize = original.Clone(); + EditSizeDialog.Title = resourceLoader.GetString("ImageResizer_EditSizeDialog_EditTitle"); + EditSizeDialog.PrimaryButtonText = resourceLoader.GetString("ImageResizer_EditSizeDialog_Update"); + EditSizeDialog.PrimaryButtonCommand = UpdateCommand; + await EditSizeDialog.ShowAsync(); + } + catch (Exception ex) + { + Logger.LogError("Exception encountered when opening the edit image size dialog.", ex); + } + } + + private void Add() + { + ViewModel.AddImageSize(EditingSize); + EditSizeDialog.Hide(); + } + + private void Update() + { + if (_editOriginal != null) + { + ViewModel.UpdateImageSize(_editOriginal, EditingSize); + } + + EditSizeDialog.Hide(); + } + private void ImagesSizesListView_ContainerContentChanging(ListViewBase sender, ContainerContentChangingEventArgs args) { if (ViewModel.IsListViewFocusRequested) diff --git a/src/settings-ui/Settings.UI/Strings/en-us/Resources.resw b/src/settings-ui/Settings.UI/Strings/en-us/Resources.resw index 2c1c0734d0..e4904b53c9 100644 --- a/src/settings-ui/Settings.UI/Strings/en-us/Resources.resw +++ b/src/settings-ui/Settings.UI/Strings/en-us/Resources.resw @@ -1218,9 +1218,6 @@ opera.exe Fit - - Width - Height @@ -1233,6 +1230,29 @@ opera.exe Add new size + + Cancel + + + Add new size + + + Edit size + + + Save + + + Update + + + Width + Header for the width input field in the Image Resizer edit size dialog + + + Size + Header shown instead of "Width" when a single value scales the whole image (e.g. percentage), in the Image Resizer edit size dialog + JPEG quality level (%) {Locked="JPEG"} @@ -2379,29 +2399,16 @@ From there, simply click on one of the supported files in the File Explorer and Pixels - - Edit - - - Edit size - ImageResizer presets Add a new preset - - Remove - Edit the {0} preset Expands to the AutomationProperties.Name value for the Edit button. Example: "Edit the Small preset". - - Remove the {0} preset - Expands to the AutomationProperties.Name value for the Remove button. Example: "Remove the Large preset". - No Label of a cancel button @@ -2642,9 +2649,6 @@ From there, simply click on one of the supported files in the File Explorer and Edit - - Remove - Cancel diff --git a/src/settings-ui/Settings.UI/ViewModels/ImageResizerViewModel.cs b/src/settings-ui/Settings.UI/ViewModels/ImageResizerViewModel.cs index 44a435271d..0fbfc38c98 100644 --- a/src/settings-ui/Settings.UI/ViewModels/ImageResizerViewModel.cs +++ b/src/settings-ui/Settings.UI/ViewModels/ImageResizerViewModel.cs @@ -318,19 +318,41 @@ public partial class ImageResizerViewModel : Observable } public void AddImageSize(string namePrefix = "") + { + AddImageSize(CreateNewImageSizeModel(namePrefix)); + } + + /// + /// Creates a new preset populated with default values and a generated unique name, without + /// adding it to the collection. Used as the working copy for the add dialog + /// (so nothing is committed until the user confirms) and as the source for . + /// + public ImageSize CreateNewImageSizeModel(string namePrefix = "") { if (string.IsNullOrEmpty(namePrefix)) { namePrefix = DefaultPresetNamePrefix; } - Sizes.Add(new ImageSize( + return new ImageSize( _nextId, GenerateNameForNewSize(namePrefix), _customSize.Fit, _customSize.Width, _customSize.Height, - _customSize.Unit)); + _customSize.Unit); + } + + /// + /// Commits a preset created via to the + /// collection, assigning it the next available unique ID. + /// + public void AddImageSize(ImageSize size) + { + ArgumentNullException.ThrowIfNull(size); + + size.Id = _nextId; + Sizes.Add(size); _nextId++; @@ -338,6 +360,29 @@ public partial class ImageResizerViewModel : Observable IsListViewFocusRequested = true; } + /// + /// Applies the values from an edited working copy back onto the original preset, saving once. + /// + public void UpdateImageSize(ImageSize original, ImageSize updated) + { + ArgumentNullException.ThrowIfNull(original); + ArgumentNullException.ThrowIfNull(updated); + + // Temporarily detach the per-item save handler so the individual property updates below + // don't each trigger a save; persist once at the end instead. + original.PropertyChanged -= SizePropertyChanged; + + original.Name = updated.Name; + original.Fit = updated.Fit; + original.Width = updated.Width; + original.Height = updated.Height; + original.Unit = updated.Unit; + + original.PropertyChanged += SizePropertyChanged; + + SaveImageSizes(); + } + public void DeleteImageSize(int id) { ImageSize size = _sizes.First(x => x.Id == id);