Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Settings] ImageResizer settings accessibility updates, fixes and refactor #36903

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

daverayment
Copy link
Contributor

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Accessibility Updates

  • Added accessible name for the image size preset settings card group, giving additional context to the Add button when it is narrated.
  • Replaced the Add button's "Add new size" accessible name with "Add new preset", as suggested.
  • Added dynamic Name and FullDescription accessible fields for Edit and Remove buttons. Instead of just being "Edit" and "Remove", the Name properties now also refer to the preset name, e.g. "Edit the Small preset". New description fields give a complete summary of all the preset properties, e.g. "Removes the Large preset, which fits to 1920 by 1080 pixels".

Preset Descriptions fix

  • Fixed the fit and unit text being absent in the card description field.
  • Refactored ImageResizerFitToStringConverter and ImageResizerUnitToStringConverter to create lookups instead of querying for the resource on every call.

Localization

  • Renamed "EditButton" to "ImageResizer_EditButton" in resources and XAML, to align with other names and to reduce the risk of future conflicts with control IDs on other settings pages.
  • ImageResizerPage should not have to know the localized default name for adding a new image size preset. This has been made a default in the view model instead.

ViewModel Fixes and Refactor

  • Backing fields were directly set to the settings values. The public property names are now used consistently, following best practice.
  • SavesImageSizes() saved the settings to the "sizes.json" file before updating the settings value itself. This may have resulted in issues where adding a new preset, not changing any of its properties, then closing/navigating away would not save the new item in "sizes.json", even though it would be correctly saved in "settings.json".
  • Hard-coded values for new preset properties removed. These are now taken from Settings.Properties.ImageresizerCustomSize instead.
  • EncoderGuid lookup added, replacing repeated if-else code in GetEncoderGuid() and GetEncoderIndex().
  • Various assignments between the observable Sizes property and its backing field or temporary variables have been removed and replaced with a centralised CollectionChanged handler. For example, SizePropertyChanged was:
    public void SizePropertyChanged(object sender, PropertyChangedEventArgs e)
    {
        ImageSize modifiedSize = (ImageSize)sender;
        ObservableCollection<ImageSize> imageSizes = Sizes;
        imageSizes.First(x => x.Id == modifiedSize.Id).Update(modifiedSize);
        _advancedSizes = imageSizes;
        SavesImageSizes(imageSizes);
    }

and can now be reduced to:

    public void SizePropertyChanged(object sender, PropertyChangedEventArgs e)
    {
        SaveImageSizes();
    }
  • Renamed the Sizes observable collection backing field to _sizes from _advancedSizes.
  • Refactored repeated property setting code into a new SetProperty() function.
  • Removed saving to "sizes.json" in the Encoder setter. The encoder guid is not saved in that file. It is still saved in "settings.json" in SetProperty().
  • Renamed AddRow() to more descriptive AddImageSize(). This also matches the existing DeleteImageSize().
  • Fixed the PropertyChanged event handler not being cleaned up when deleting a size preset, in DeleteImageSize(). Subscribing and unsubscribing is now handled in new methods SubscribeToItemPropertyChanged() and UnsubscribeFromPropertyChanged(), keeping this related code together.
  • Slightly simplified GenerateNameForNewSize().

ImageSize Fixes and Refactor

  • Update() is not required and has been removed. Dependency properties are correctly defined for the bound fields already.
  • Removed parameterless constructor and added constructor parameter defaults instead.
  • Refactored repeated property setting code into a SetProperty() function.
  • Added [JsonIgnore] to fields which were not meant to be serialized.
  • Removed unused ExtraBoxOpacity field.
  • Renamed EnableEtraBoxes (sic) to IsHeightUsed, better reflecting its usage.

Validation Steps Performed

AIW Tests

Confirmed new accessibility values were present by using Accessibility Insights for Windows:

  • New group Name and updated add button Name:

Screenshot 2025-01-15 023536

  • Edit and Remove buttons include the size preset name in their Name properties:

Screenshot 2025-01-14 000626

  • Edit and Remove buttons include FullDescription accessibility fields with complete summary information on the preset properties:

Screenshot 2025-01-15 023037

Other Tests

Tested that:

  • The fit and unit descriptions were now correctly displayed with the width and height information.
  • All values were updated correctly when the image preset properties were updated, including Name and FullDescription accessibility properties.
  • Properties were correctly persisted to both "sizes.json" and "settings.json" files.
  • The old JSON files were correctly read, i.e. the now-ignored properties in ImageSize do not cause any issues.
  • Different formats are correctly used for whether the height property is being used in the preset.
  • Deleting size presets at the beginning, middle and end of the list do not cause issues and the list is reloaded in the same order when navigating away from the page and back again.
  • Adding a new size preset shows the custom width and height, not the same values as the Small preset.
  • Adding and editing new sizes correctly persists the properties to "sizes.json" and "settings.json" files and these are loaded back in the same order when navigating away from and back to the settings page.

…es and refactor view model and ImageSize. New resources for accessibility text formats.
… unit tests having incorrect expected/actual orderings.
…nverter, instead of via format strings; simplified encoder GUID collection declaration and retrieval.
@daverayment
Copy link
Contributor Author

@drawbyperpetual Thanks again for the review. I've made the suggested changes and re-run the tests. Please let me know if there's anything further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants