TheJoeFin / Simple-Icon-File-Maker

Create .ico files quickly and at different scales.
https://apps.microsoft.com/store/detail/simple-icon-file-maker/9NS1BM1FB99Z
157 stars 14 forks source link

Unsupported JPEG types cause the app to crash #6

Closed skycommand closed 1 year ago

skycommand commented 1 year ago

It appears Simple Icon File Maker does not support every JPEG image. That's fine. However, upon encountering those unsupported JPEG images, Simple Icon File Maker crashes. I'm appending a sample.

(Commons.Wikimedia.org) Lamprotornis hildebrandti wallpaper.zip

TheJoeFin commented 1 year ago

Interesting scenario! Thanks for bringing it to my attention, I'll take a look.

cerealfordinner commented 1 year ago

It does look like the file is corrupt in some way. I'm a junior who does almost entirely backend, but I wanted to look at this project and thought this issue seemed like a good way to get some hands on experience with WinUI.

What I was thinking was wrap the SetSourceAsync method in a try-catch and display a dialog box to the user at least for the time being to prevent the unhandled exception.

Something like this:

        try
        {
            await bitmapImage.SetSourceAsync(fileStream);
        }
        catch (Exception ex)
        {
            ContentDialog dialog = new ContentDialog()
            {
                Title = "Error",
                Content = "An error occurred: " + ex.Message,
                PrimaryButtonText = "Ok"
            };
            dialog.XamlRoot = this.Content.XamlRoot;
            await dialog.ShowAsync();
            return;
        }

Like I said I'm still relatively new to this so if there's a better approach I'm interested in hearing it.

If this is something that works I can always clean it up a bit and put in a PR.

TheJoeFin commented 1 year ago

Hello @cerealfordinner! I used this app to learn WinAppSDK and WinUI 3 so hopefully you can learn with it as well!

As for your suggestion, that would totally work, but here are my thoughts about when to pop an error and what kind of UX to show.

When to show the warning

I have not debugged this issue yet, but I'm assuming (like you mentioned in your snippet) the app crashes when setting the source of the bitmapImage. But I did not do a good job of keeping this code D.R.Y., so that line does get called a couple of different places. Step 1: consolidate the code for reading a bitmap and setting the MainImage.Source into a single method. Step 2: implement a try catch like you suggested.

What Kind of UX to use

First off, your ContentDialog suggestion would totally work. Personally though, I prefer UX where errors don't require me to click "OK" to try something again. What about the InfoBar control with the state of error for Severity and the message $"Failed to read image. Error: {ex.Message}"

Thank you for taking the initiative on this issue and I would welcome a PR! Also feel free to reach out if you have any more questions or issues getting the code running.