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

[ImageResizer] Fix issues with blank Width and Height controls #37373

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

Conversation

daverayment
Copy link
Contributor

Summary of the Pull Request

This fixes some related issues with the Width and Height NumberBox controls for the Custom preset. The issues were:

  1. Users could not clear existing numbers from the field and move away from the field without a databinding error being displayed. (Red border.)
  2. Settings were not saved if Width or Height were blank, leading to the previous value for that dimension being used in the resize operation. This would result in incorrect sizes for the resulting image, and the UI not showing the actual value used.
  3. Empty/auto values were not saved to the settings file, meaning the previous value of Width or Height was loaded in again when Image Resizer was re-launched.

PR Checklist

Detailed Description of the Pull Request / Additional comments

The issue centred around the fact that emptying a NumberBox bound to a non-nullable double value will result in a data binding error because the empty control now has a null Value.

Inputting 0 into the fields instead of clearing them completely is fine and would be reasonable as a workaround, but this does not reflect the description of the feature on the Image Resizer help page, which specifically mentions that one or the other dimension field may be left blank to take advantage of the auto-scaling functionality.

The solution is to translate empty strings to 0, but not to display 0 in the UI. This is achieved through combining a NumberFormatter and a ValueConverter:

  • The NumberFormatter ensures that if the control has a value of 0, it will display an empty string.
  • The ValueConverter ensures that when the user clears the NumberBox or inputs an expression that evaluates to an invalid input, that 0 is always set as the result.

Without the ValueConverter, the data binding step would fail because null is the value of a cleared NumberBox and we would be attempting to bind to ResizeSize.Width or ResizeSize.Height, which are non-nullable.

There are likely some alternative methods to get around this, perhaps by using double.NaN instead of 0, or changing Width and Height to nullable. However, relying on the ValueConverter and NumberFormatter means that we do not need to change the underlying types, and this was seen as lower risk.

I made a few changes and added comments to AutoDoubleConverter.cs. This was because I initially thought I could use it on the custom preset's Width and Height fields, before deciding to create a new Converter made specifically to resolve the issue at hand. Also, I took the opportunity to make it more defensive in terms of type handling by using pattern matching. The functionality is identical.

Validation Steps Performed

The application was manually tested both via the command line and by running directly from Visual Studio (which opens up the file picker because there is no file pre-selected).

Tested that:

  • Width and Height NumberBoxes may now be empty, and no error is reported when they are in this state.
  • When Width or Height is set to (auto) by blanking it out, the other dimension is used correctly to resize the image. The previous value of the now-blank field is not used.
  • If "0" is input manually into either field, it is updated to be an empty string when focus moves from the control, keeping the UI consistent.
  • The empty value is persisted to the settings file and is reloaded correctly when the application is re-launched. (An empty value is saved as "0".)
  • A variety of file types are supported (sanity check - I did not test all supported types).
  • (auto) text appears in the list of presets where one dimension has not been provided.
  • The specific steps detailed in the original Issues no longer appear.
  • All unit tests still pass.
  • The Settings page for Image Resizer is still correct.

Screenshots

Dimensions may be blank without validation errors:
image

Empty dimensions are correctly marked as auto in the presets drop-down:
image

@crutkas
Copy link
Member

crutkas commented Feb 10, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@crutkas
Copy link
Member

crutkas commented Feb 11, 2025

Failed XAML formatting

@daverayment
Copy link
Contributor Author

Failed XAML formatting

Thanks, @crutkas , and sorry for missing that. I've corrected the namespace ordering, and everything passes locally now.

@crutkas
Copy link
Member

crutkas commented Feb 11, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@crutkas crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Feb 14, 2025
@lei9444
Copy link
Contributor

lei9444 commented Feb 17, 2025

Hi @daverayment The Image Resizer UI appears as expected, and the code looks good to me. However, the settings page also needs to be updated. Could you update this part as well?
image

@daverayment
Copy link
Contributor Author

Thank you for the review, @lei9444. I agree that it makes sense to fix the settings formatting, just so everything is consistent.

There is an AutoDoubleConverter in the ImageResizer code which already does the (auto) formatting, so hopefully it won't take too long.

Do you think the capitalisation of the scaling mode needs altering, too? The line starts with a capital letter in the settings, but not in the ImageResizer UI itself.

Thanks again.

@lei9444
Copy link
Contributor

lei9444 commented Feb 18, 2025

Thanks @daverayment Good catch on this one. I think we should keep them consistent. Maybe we could create another PR for the format change to keep this PR focused on the issue fix. @crutkas do you have any suggestion on this?

@crutkas
Copy link
Member

crutkas commented Feb 18, 2025

I’m fine with that

@daverayment
Copy link
Contributor Author

Thanks, both.

@lei9444 I've raised the inconsistency in casing as #37520. It seems it would be a good first issue for a new contributor to tackle.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • src/modules/imageresizer/ui/App.xaml: Language not supported
  • src/modules/imageresizer/ui/Views/InputPage.xaml: Language not supported
Comments suppressed due to low confidence (1)

src/modules/imageresizer/ui/Views/NumberBoxValueConverter.cs:18

  • The method should return an empty string for double.NaN to be consistent with ZeroToEmptyStringNumberFormatter.
value is double d && double.IsNaN(d) ? 0 : value;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In for .89 Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants