search

leopard-js / sb-edit

Javascript library for manipulating Scratch project files
MIT License
55 stars 15 forks source link

Reference/resolve variables by ID instead of name #89

Closed towerofnix closed 1 year ago

towerofnix commented 1 year ago

This is a major change to the way sb-edit reads and serializes variables.

This PR changes BlockInput.Variable and BlockInput.List so their value is a pair of values (in an object), { id: string, name: string }, instead of one value (just the name). The fromSb3 deserializing code pulls from the IDs in the actual sb3 file, and all serializing code is updated to respect the new format — most notably, this resolves issues in toLeopard surrounding multiple variables existing of different IDs but the same name.

The last commit, af5280ba271a184aa0a218ab7b306a7f2f31377c, adds a final pass to fromSb3 deserialization so that variables which are never referenced in blocks nor shown as monitors are totally wiped from the code — this is a bit of a presumptuous decision and means sb3 -> sb3 conversion is explicitly not a one-to-one conversion, but I think that's an okay precedent to set, and it seemed to make more sense to run this pass on input data in fromSb3 rather than preprocessing in toLeopard. (In the future, I'm considering separating this chunk of code out so it's one of a modular set of "processing" prefabs including other automatic code optimizations, but that would be for a separate PR.)

@adroitwhiz I added you as a PR reviewer, but feel free to pass if you aren't interested or don't currently have the time!

towerofnix commented 1 year ago

Will need to regen snapshots for this PR and the follow-up one, rn I am crashing for the night! 🐌

PullJosh commented 1 year ago

Hey! I have been looking at this code (and your other PRs) in my moments of free time... It will take me a bit longer to review everything, but I just wanted to send you a message to say a huge thanks for the contribution. 😄

I really appreciate you continuing to contribute fixes! ❤️

towerofnix commented 1 year ago

No problem at all! I've been enjoying popping back into this codebase. Leopard is really one of my favorite projects surrounding Scratch, so I'm glad to have the opportunity to poke around the code and improve it! ✨

towerofnix commented 1 year ago

Thanks, both! I've gone over your code reviews. The most notable change is in the implementation for Target.blocks — a property which I updated for this purpose but forgot to push to this branch! — matching your suggestion of recursively pushing to an "all blocks" collector array instead of spreading/flat-mapping intermediates, @adroitwhiz.

I've also made similar changes and optimizations throughout, see my recent commits. Most notable: caching a set of stage variables instead of iterating over target.variables every time, as well as entirely short-circuiting the set check if the target is the stage.

towerofnix commented 1 year ago

@adroitwhiz Yep, it wasn't visible when you reviewed because I pushed it to compat (which is based off this branch) and forgot to push it here too. 📦

Thanks both for the reviews!

@PullJosh I've merged this pull request. It's not dependent on any other PRs, as my other pull requests are either independent or support for #90. They should all be reviewed, but here's a summary of their relationships to help guide you:

Since this pull request fixes user-visible bugs (failing to import some projects such as seen in #85 and leopard-js/leopard#162) and does not depend on any other pull requests, it's ready and useful to end users to release a version of sb-edit before other pull requests are reviewed.