From 438d17302ed6e31b470c2eb1c241fbc6d7c4a841 Mon Sep 17 00:00:00 2001 From: Dave Rayment Date: Tue, 21 Jan 2025 11:55:02 +0000 Subject: [PATCH] [Settings]ImageResizer settings accessibility updates, fixes and refactor (#36903) * Fix issue with missing Image Resizer unit and fit information in settings description. * Fix accessibility issues on Edit and Remove buttons. Fix various issues and refactor view model and ImageSize. New resources for accessibility text formats. * Fix unit test because of change to new preset width and height. Fix 2 unit tests having incorrect expected/actual orderings. * Post-review update: accessibility strings now formatted within the converter, instead of via format strings; simplified encoder GUID collection declaration and retrieval. * Minor example text fix. --- .../Settings.UI.Library/ImageSize.cs | 294 +++------ .../ViewModelTests/ImageResizer.cs | 32 +- .../ImageResizerFitToStringConverter.cs | 53 +- ...ageResizerSizeToAccessibleTextConverter.cs | 71 +++ .../ImageResizerUnitToStringConverter.cs | 55 +- .../SettingsXAML/Views/ImageResizerPage.xaml | 34 +- .../Views/ImageResizerPage.xaml.cs | 9 +- .../Settings.UI/Strings/en-us/Resources.resw | 23 +- .../ViewModels/ImageResizerViewModel.cs | 567 ++++++++---------- 9 files changed, 519 insertions(+), 619 deletions(-) create mode 100644 src/settings-ui/Settings.UI/Converters/ImageResizerSizeToAccessibleTextConverter.cs diff --git a/src/settings-ui/Settings.UI.Library/ImageSize.cs b/src/settings-ui/Settings.UI.Library/ImageSize.cs index 7bbad59726e6..017f66582014 100644 --- a/src/settings-ui/Settings.UI.Library/ImageSize.cs +++ b/src/settings-ui/Settings.UI.Library/ImageSize.cs @@ -3,241 +3,119 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Generic; using System.ComponentModel; using System.Runtime.CompilerServices; using System.Text.Json; using System.Text.Json.Serialization; +using Settings.UI.Library.Resources; -namespace Microsoft.PowerToys.Settings.UI.Library -{ - public class ImageSize : INotifyPropertyChanged - { - public ImageSize(int id) - { - Id = id; - Name = string.Empty; - Fit = ResizeFit.Fit; - Width = 0; - Height = 0; - Unit = ResizeUnit.Pixel; - } - - public ImageSize() - { - Id = 0; - Name = string.Empty; - Fit = ResizeFit.Fit; - Width = 0; - Height = 0; - Unit = ResizeUnit.Pixel; - } - - public ImageSize(int id, string name, ResizeFit fit, double width, double height, ResizeUnit unit) - { - Id = id; - Name = name; - Fit = fit; - Width = width; - Height = height; - Unit = unit; - } +namespace Microsoft.PowerToys.Settings.UI.Library; - private int _id; - private string _name; - private ResizeFit _fit; - private double _height; - private double _width; - private ResizeUnit _unit; - - public int Id - { - get - { - return _id; - } - - set - { - if (_id != value) - { - _id = value; - OnPropertyChanged(); - } - } - } - - public int ExtraBoxOpacity - { - get - { - if (Unit == ResizeUnit.Percent && Fit != ResizeFit.Stretch) - { - return 0; - } - else - { - return 100; - } - } - } +public partial class ImageSize : INotifyPropertyChanged +{ + public event PropertyChangedEventHandler PropertyChanged; - public bool EnableEtraBoxes + private bool SetProperty(ref T field, T value, [CallerMemberName] string propertyName = null) + { + bool changed = !EqualityComparer.Default.Equals(field, value); + if (changed) { - get - { - if (Unit == ResizeUnit.Percent && Fit != ResizeFit.Stretch) - { - return false; - } - else - { - return true; - } - } + field = value; + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName)); + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(AccessibleTextHelper))); } - [JsonPropertyName("name")] - public string Name - { - get - { - return _name; - } + return changed; + } - set - { - if (_name != value) - { - _name = value; - OnPropertyChanged(); - } - } - } + public ImageSize(int id = 0, string name = "", ResizeFit fit = ResizeFit.Fit, double width = 0, double height = 0, ResizeUnit unit = ResizeUnit.Pixel) + { + Id = id; + Name = name; + Fit = fit; + Width = width; + Height = height; + Unit = unit; + } - [JsonPropertyName("fit")] - public ResizeFit Fit - { - get - { - return _fit; - } + private int _id; + private string _name; + private ResizeFit _fit; + private double _height; + private double _width; + private ResizeUnit _unit; - set - { - if (_fit != value) - { - _fit = value; - OnPropertyChanged(); - OnPropertyChanged(nameof(ExtraBoxOpacity)); - OnPropertyChanged(nameof(EnableEtraBoxes)); - } - } - } + public int Id + { + get => _id; + set => SetProperty(ref _id, value); + } - [JsonPropertyName("width")] - public double Width - { - get - { - return _width; - } + /// + /// Gets a value indicating whether the property is used. When false, the + /// property is used to evenly scale the image in both X and Y dimensions. + /// + [JsonIgnore] + public bool IsHeightUsed + { + // Height is ignored when using percentage scaling where the aspect ratio is maintained + // (i.e. non-stretch fits). In all other cases, both Width and Height are needed. + get => !(Unit == ResizeUnit.Percent && Fit != ResizeFit.Stretch); + } - set - { - double newWidth = -1; - - if (value < 0 || double.IsNaN(value)) - { - newWidth = 0; - } - else - { - newWidth = value; - } - - if (_width != newWidth) - { - _width = newWidth; - OnPropertyChanged(); - } - } - } + [JsonPropertyName("name")] + public string Name + { + get => _name; + set => SetProperty(ref _name, value); + } - [JsonPropertyName("height")] - public double Height + [JsonPropertyName("fit")] + public ResizeFit Fit + { + get => _fit; + set { - get - { - return _height; - } - - set + if (SetProperty(ref _fit, value)) { - double newHeight = -1; - - if (value < 0 || double.IsNaN(value)) - { - newHeight = 0; - } - else - { - newHeight = value; - } - - if (_height != newHeight) - { - _height = newHeight; - OnPropertyChanged(); - } + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(IsHeightUsed))); } } + } - [JsonPropertyName("unit")] - public ResizeUnit Unit - { - get - { - return _unit; - } - - set - { - if (_unit != value) - { - _unit = value; - OnPropertyChanged(); - OnPropertyChanged(nameof(ExtraBoxOpacity)); - OnPropertyChanged(nameof(EnableEtraBoxes)); - } - } - } + [JsonPropertyName("width")] + public double Width + { + get => _width; + set => SetProperty(ref _width, value < 0 || double.IsNaN(value) ? 0 : value); + } - public event PropertyChangedEventHandler PropertyChanged; + [JsonPropertyName("height")] + public double Height + { + get => _height; + set => SetProperty(ref _height, value < 0 || double.IsNaN(value) ? 0 : value); + } - public void OnPropertyChanged([CallerMemberName] string propertyName = null) + [JsonPropertyName("unit")] + public ResizeUnit Unit + { + get => _unit; + set { - var handler = PropertyChanged; - if (handler != null) + if (SetProperty(ref _unit, value)) { - handler(this, new PropertyChangedEventArgs(propertyName)); + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(IsHeightUsed))); } } + } - public void Update(ImageSize modifiedSize) - { - ArgumentNullException.ThrowIfNull(modifiedSize); - - Id = modifiedSize.Id; - Name = modifiedSize.Name; - Fit = modifiedSize.Fit; - Width = modifiedSize.Width; - Height = modifiedSize.Height; - Unit = modifiedSize.Unit; - } + /// + /// Gets access to all properties for formatting accessibility descriptions. + /// + [JsonIgnore] + public ImageSize AccessibleTextHelper => this; - public string ToJsonString() - { - return JsonSerializer.Serialize(this); - } - } + 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 1d2c62daecf1..c5175bc180e4 100644 --- a/src/settings-ui/Settings.UI.UnitTests/ViewModelTests/ImageResizer.cs +++ b/src/settings-ui/Settings.UI.UnitTests/ViewModelTests/ImageResizer.cs @@ -205,7 +205,7 @@ public void EncoderShouldUpdateValueWhenSuccessful() } [TestMethod] - public void AddRowShouldAddNewImageSizeWhenSuccessful() + public void AddImageSizeShouldAddNewImageSizeWhenSuccessful() { // arrange var mockSettingsUtils = ISettingsUtilsMocks.GetStubSettingsUtils(); @@ -214,7 +214,7 @@ public void AddRowShouldAddNewImageSizeWhenSuccessful() int sizeOfOriginalArray = viewModel.Sizes.Count; // act - viewModel.AddRow("New size"); + viewModel.AddImageSize(); // Assert Assert.AreEqual(sizeOfOriginalArray + 1, viewModel.Sizes.Count); @@ -229,15 +229,15 @@ public void NewlyAddedImageSizeHasCorrectValues() ImageResizerViewModel viewModel = new ImageResizerViewModel(mockSettingsUtils.Object, SettingsRepository.GetInstance(_mockGeneralSettingsUtils.Object), sendMockIPCConfigMSG, (string name) => name); // act - viewModel.AddRow("New size"); + viewModel.AddImageSize("New size"); // Assert ImageSize newTestSize = viewModel.Sizes.First(x => x.Id == 0); - Assert.AreEqual(newTestSize.Name, "New size 1"); - Assert.AreEqual(newTestSize.Fit, ResizeFit.Fit); - Assert.AreEqual(newTestSize.Width, 854); - Assert.AreEqual(newTestSize.Height, 480); - Assert.AreEqual(newTestSize.Unit, ResizeUnit.Pixel); + Assert.AreEqual("New size 1", newTestSize.Name); + Assert.AreEqual(ResizeFit.Fit, newTestSize.Fit); + Assert.AreEqual(1024, newTestSize.Width); + Assert.AreEqual(640, newTestSize.Height); + Assert.AreEqual(ResizeUnit.Pixel, newTestSize.Unit); } [TestMethod] @@ -247,7 +247,7 @@ public void DeleteImageSizeShouldDeleteImageSizeWhenSuccessful() var mockSettingsUtils = ISettingsUtilsMocks.GetStubSettingsUtils(); Func sendMockIPCConfigMSG = msg => { return 0; }; ImageResizerViewModel viewModel = new ImageResizerViewModel(mockSettingsUtils.Object, SettingsRepository.GetInstance(_mockGeneralSettingsUtils.Object), sendMockIPCConfigMSG, (string name) => name); - viewModel.AddRow("New Size"); + viewModel.AddImageSize("New Size"); int sizeOfOriginalArray = viewModel.Sizes.Count; ImageSize deleteCandidate = viewModel.Sizes.First(x => x.Id == 0); @@ -268,16 +268,16 @@ public void NameOfNewImageSizesIsIncrementedCorrectly() ImageResizerViewModel viewModel = new ImageResizerViewModel(mockSettingsUtils.Object, SettingsRepository.GetInstance(_mockGeneralSettingsUtils.Object), sendMockIPCConfigMSG, (string name) => name); // act - viewModel.AddRow("New size"); // Add: "New size 1" - viewModel.AddRow("New size"); // Add: "New size 2" - viewModel.AddRow("New size"); // Add: "New size 3" + viewModel.AddImageSize("New size"); // Add: "New size 1" + viewModel.AddImageSize("New size"); // Add: "New size 2" + viewModel.AddImageSize("New size"); // Add: "New size 3" viewModel.DeleteImageSize(1); // Delete: "New size 2" - viewModel.AddRow("New size"); // Add: "New Size 4" + viewModel.AddImageSize("New size"); // Add: "New Size 4" // Assert - Assert.AreEqual(viewModel.Sizes[0].Name, "New size 1"); - Assert.AreEqual(viewModel.Sizes[1].Name, "New size 3"); - Assert.AreEqual(viewModel.Sizes[2].Name, "New size 4"); + Assert.AreEqual("New size 1", viewModel.Sizes[0].Name); + Assert.AreEqual("New size 3", viewModel.Sizes[1].Name); + Assert.AreEqual("New size 4", viewModel.Sizes[2].Name); } [TestMethod] diff --git a/src/settings-ui/Settings.UI/Converters/ImageResizerFitToStringConverter.cs b/src/settings-ui/Settings.UI/Converters/ImageResizerFitToStringConverter.cs index bfc991c07090..22ecd928436d 100644 --- a/src/settings-ui/Settings.UI/Converters/ImageResizerFitToStringConverter.cs +++ b/src/settings-ui/Settings.UI/Converters/ImageResizerFitToStringConverter.cs @@ -3,41 +3,38 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Generic; using System.Globalization; - +using System.Windows; +using Microsoft.PowerToys.Settings.UI.Library; using Microsoft.UI.Xaml.Data; -namespace Microsoft.PowerToys.Settings.UI.Converters +namespace Microsoft.PowerToys.Settings.UI.Converters; + +public sealed partial class ImageResizerFitToStringConverter : IValueConverter { - public sealed partial class ImageResizerFitToStringConverter : IValueConverter + // Maps each ResizeFit to its localized string. + private static readonly Dictionary FitToText = new() { - public object Convert(object value, Type targetType, object parameter, string language) - { - var toLower = false; - if ((string)parameter == "ToLower") - { - toLower = true; - } - - string targetValue = string.Empty; - switch (value) - { - case 0: targetValue = Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Fit_Fill_ThirdPersonSingular"); break; - case 1: targetValue = Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Fit_Fit_ThirdPersonSingular"); break; - case 2: targetValue = Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Fit_Stretch_ThirdPersonSingular"); break; - } - - if (toLower) - { - targetValue = targetValue.ToLower(CultureInfo.CurrentCulture); - } - - return targetValue; - } + { ResizeFit.Fill, Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Fit_Fill_ThirdPersonSingular") }, + { ResizeFit.Fit, Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Fit_Fit_ThirdPersonSingular") }, + { ResizeFit.Stretch, Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Fit_Stretch_ThirdPersonSingular") }, + }; - public object ConvertBack(object value, Type targetType, object parameter, string language) + public object Convert(object value, Type targetType, object parameter, string language) + { + if (value is ResizeFit fit && FitToText.TryGetValue(fit, out string fitText)) { - return value; + return parameter is string lowerParam && lowerParam == "ToLower" ? + fitText.ToLower(CultureInfo.CurrentCulture) : + fitText; } + + return DependencyProperty.UnsetValue; + } + + public object ConvertBack(object value, Type targetType, object parameter, string language) + { + return value; } } diff --git a/src/settings-ui/Settings.UI/Converters/ImageResizerSizeToAccessibleTextConverter.cs b/src/settings-ui/Settings.UI/Converters/ImageResizerSizeToAccessibleTextConverter.cs new file mode 100644 index 000000000000..3fd03b08d64d --- /dev/null +++ b/src/settings-ui/Settings.UI/Converters/ImageResizerSizeToAccessibleTextConverter.cs @@ -0,0 +1,71 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Windows; +using Microsoft.PowerToys.Settings.UI.Library; +using Microsoft.UI.Xaml.Data; + +namespace Microsoft.PowerToys.Settings.UI.Converters; + +/// +/// Creates accessibility text for controls related to properties. +/// +/// (Name) "Edit the Small preset" +/// (FullDescription) "Large - Fits within 1920 × 1080 pixels"" +public sealed partial class ImageResizerSizeToAccessibleTextConverter : IValueConverter +{ + private const char TimesGlyph = '\u00D7'; // Unicode "MULTIPLICATION SIGN" + + /// + /// Maps the supplied accessibility identifier to the format string of the localized accessible text. + /// + 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(); + private readonly ImageResizerUnitToStringConverter _unitConverter = new(); + + public object Convert(object value, Type targetType, object parameter, string language) + { + return (value, parameter) switch + { + (string presetName, string nameId) => FormatNameText(presetName, nameId), + (ImageSize preset, string _) => FormatDescriptionText(preset), + _ => DependencyProperty.UnsetValue, + }; + } + + private object FormatNameText(string presetName, string nameId) + { + return AccessibilityFormats.TryGetValue(nameId, out string format) ? + string.Format(CultureInfo.CurrentCulture, format, presetName) : + DependencyProperty.UnsetValue; + } + + private object FormatDescriptionText(ImageSize preset) + { + if (preset == null) + { + return DependencyProperty.UnsetValue; + } + + string fitText = _fitConverter.Convert(preset.Fit, typeof(string), null, null) as string; + string unitText = _unitConverter.Convert(preset.Unit, typeof(string), null, null) as string; + + return preset.IsHeightUsed ? + $"{preset.Name} - {fitText} {preset.Width} {TimesGlyph} {preset.Height} {unitText}" : + $"{preset.Name} - {fitText} {preset.Width} {unitText}"; + } + + public object ConvertBack(object value, Type targetType, object parameter, string language) + { + throw new NotImplementedException(); + } +} diff --git a/src/settings-ui/Settings.UI/Converters/ImageResizerUnitToStringConverter.cs b/src/settings-ui/Settings.UI/Converters/ImageResizerUnitToStringConverter.cs index ab4aec819550..fca73cfa0760 100644 --- a/src/settings-ui/Settings.UI/Converters/ImageResizerUnitToStringConverter.cs +++ b/src/settings-ui/Settings.UI/Converters/ImageResizerUnitToStringConverter.cs @@ -3,42 +3,39 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Generic; using System.Globalization; - +using System.Windows; +using Microsoft.PowerToys.Settings.UI.Library; using Microsoft.UI.Xaml.Data; -namespace Microsoft.PowerToys.Settings.UI.Converters +namespace Microsoft.PowerToys.Settings.UI.Converters; + +public sealed partial class ImageResizerUnitToStringConverter : IValueConverter { - public sealed partial class ImageResizerUnitToStringConverter : IValueConverter + // Maps each ResizeUnit value to its localized string. + private static readonly Dictionary UnitToText = new() { - public object Convert(object value, Type targetType, object parameter, string language) - { - var toLower = false; - if ((string)parameter == "ToLower") - { - toLower = true; - } - - string targetValue = string.Empty; - switch (value) - { - case 0: targetValue = Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Unit_Centimeter"); break; - case 1: targetValue = Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Unit_Inch"); break; - case 2: targetValue = Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Unit_Percent"); break; - case 3: targetValue = Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Unit_Pixel"); break; - } - - if (toLower) - { - targetValue = targetValue.ToLower(CultureInfo.CurrentCulture); - } - - return targetValue; - } + { ResizeUnit.Centimeter, Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Unit_Centimeter") }, + { ResizeUnit.Inch, Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Unit_Inch") }, + { ResizeUnit.Percent, Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Unit_Percent") }, + { ResizeUnit.Pixel, Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_Unit_Pixel") }, + }; - public object ConvertBack(object value, Type targetType, object parameter, string language) + public object Convert(object value, Type targetType, object parameter, string language) + { + if (value is ResizeUnit unit && UnitToText.TryGetValue(unit, out string unitText)) { - return value; + return parameter is string lowerParam && lowerParam == "ToLower" ? + unitText.ToLower(CultureInfo.CurrentCulture) : + unitText; } + + return DependencyProperty.UnsetValue; + } + + public object ConvertBack(object value, Type targetType, object parameter, string language) + { + return value; } } diff --git a/src/settings-ui/Settings.UI/SettingsXAML/Views/ImageResizerPage.xaml b/src/settings-ui/Settings.UI/SettingsXAML/Views/ImageResizerPage.xaml index 96d721ee2a0e..0b0e40000c58 100644 --- a/src/settings-ui/Settings.UI/SettingsXAML/Views/ImageResizerPage.xaml +++ b/src/settings-ui/Settings.UI/SettingsXAML/Views/ImageResizerPage.xaml @@ -18,6 +18,7 @@ + + Visibility="{x:Bind IsHeightUsed, Mode=OneWay, Converter={StaticResource BoolToVisibilityConverter}}" /> + Visibility="{x:Bind IsHeightUsed, Mode=OneWay, Converter={StaticResource BoolToVisibilityConverter}}" />