scratchfoundation / scratch-storage

Load and store project and asset files for Scratch 3.0
BSD 3-Clause "New" or "Revised" License
57 stars 127 forks source link

Inconsistent handling of missing costumes (web site vs. desktop app) #367

Open cwillisf opened 2 years ago

cwillisf commented 2 years ago

Expected Behavior

Loading into the desktop app a project which refers to a missing costume should cause that costume to appear as a placeholder image, just like it does on the Scratch website.

Actual Behavior

Loading into the desktop app a project which refers to a missing costume causes an error message and prevents the project from loading at all.

Note that this means a single missing costume will prevent using the desktop app to recover any other data from a project -- even intact sprites, costumes, or sounds.

Steps to Reproduce

  1. Open the standalone Scratch app on a Mac or Windows computer
  2. Load a project which refers to a missing costume

Example project: Missing Blue Guy (broken).sb3.zip

Screenshots

image

When this happens, the developer console looks like this: image

cwillisf commented 2 years ago

This appears to be due to the storage system making an assumption about the web helper being populated, which isn't true in the desktop app. The desktop app should not be using WebHelper at all, so it certainly shouldn't be causing an error in the console.

apple502j commented 2 years ago

@cwillisf There exists a legitimate reason to use a WebHelper (music extension assets), however for costumes that's not the case. We could remove these lines: https://github.com/LLK/scratch-gui/blob/scratch-desktop/src/lib/storage.js#L15

However this does not fix the root cause, which can be fixed by adding the following line after https://github.com/LLK/scratch-storage/blob/develop/src/WebHelper.js#L102

if (!store) return Promise.resolve(null);

I also found another issue while debugging this: if the asset server could not be fetched at all (due to CORS, network disconnection, blocking the URL using developer tools etc) the code ends up in an infinite loop, spamming hundreds of logs per second and never loading the project.

cwillisf commented 2 years ago

Ooh, thanks for the reminder about the music assets! I stand by my assertion that the desktop app should avoid using WebHelper, since we intend it to be usable in completely offline environments, but considering the music extension means we're a little further from that ideal than I had been thinking when I wrote that comment. In particular, it's not just a technical issue like I had been thinking, but instead involves design decisions and user experience tradeoffs. I expect we won't decide to change it any time soon, but like you said, we can fix the costume case without breaking the music extension.

Thanks for bringing up that infinite loop issue, too! That's... not great 😅