Project dirty state fixes #1962

kchadha · 2019-01-29T23:28:09Z

Proposed Changes

Depends on #1941 (and currently the diff includes changes from that PR).

@chrisgarrity and I worked on all of these changes together.

Fix erroneous project changed state reporting.

This PR starts to decouple emitting a targets update from emitting a project changed.
Prior to this PR, we were erroneously emitting a project changed state in the following cases:

Loading a project
Running a project (scratch 2.0 did not consider running a project to change the dirty state of a project, this also includes clicking a block in the toolbox or on a sprite's workspace).
Switching from project page to editor mode
Switching sprites

The above scenarios were getting triggered by emitting project changes in the following scenarios:

during vm.clear() (which happens during load project),
during renaming a sprite whether or not such a change occurred (also happens during project load), 3) when receiving any scratch blocks event in a blocks container that can affect the project (e.g. blocks containers for actual sprites but not blocks containers for the flyout or monitors).
during runtime._step() which is always running (before/during/after project load)

After investigating and comparing the cases in which Scratch 2.0 and Scratch 3.0 track dirty state, we decided to resolve these issues by:

No longer emitting project state changes from runtime._step()
No longer emitting project state changes from vm.clear()
Being more specific about when blocks events emit project changes (e.g. we check if an actual change took place in the VM rather than just upon receiving an event). This doesn't resolve all of our issues since we have some scenarios in which switching sprites still causes a project change because we're effectively deleting a block and recreating that block in the VM. This is a known issue we hope to address in a future PR.
As a side effect of removing emitting project changes from runtime._step, we now specifically track project changes that were previously being emitted as a side effect (e.g. adding a sprite, adding a costume, etc. -- all the changes in src/virtual-machine.js)
changing renameSprite() to only emit a project change/targets update if an actual rename occurred.

Test Coverage

Added lots of unit tests and an integration test. Also did lots of manual testing!
We have documented our investigation and manual testing in the following doc. We have also documented some known issues in the doc.

https://docs.google.com/document/d/1DKM4rf-ajioOrAeipsgtuIluJNMIo71L3MfgB84y3JE/edit

Changes can be tested at https://llk.github.io/scratch-gui/project-state-changes

… scratch-blocks events are fired.

…changes in scenarios that were previously taking advantage of runtime._step()

rschamp

Overall this looks great, thanks for all the tests! I don't think this necessarily has to change before merging, there were just more than a few questions/suggestions.

src/engine/blocks.js

rschamp · 2019-01-30T20:22:23Z

src/engine/blocks.js

@@ -564,7 +582,8 @@ class Blocks {
                // Changing the value in a dropdown
                block.fields[args.name].value = args.value;

-                if (!optRuntime){
+                if (!this.runtime){
+                    log.warn('Runtime is not optional, it should get passed in when the block container is created.');


I think this check should be farther up (and fatal) or can just be removed altogether. We assume it exists above... and all over the place elsewhere.

@rschamp, I think you're right. We had this check from before updating tests to resolve #1942 and this can probably be updated now.

rschamp · 2019-01-30T20:24:39Z

src/engine/blocks.js

@@ -592,7 +611,8 @@ class Blocks {
            block.mutation = mutationAdapter(args.value);
            break;
        case 'checkbox': {
-            if (!optRuntime) {
+            if (!this.runtime) {
+                log.warn('Runtime is not optional, it should get passed in when the block container is created.');


rschamp · 2019-01-30T20:26:59Z

src/engine/blocks.js

@@ -663,6 +683,10 @@ class Blocks {
        }
        }

+        // TODO maybe track actual changes,


Is this super hard to figure out in this pass?

Ah I had actually put this in here as a note to myself to see whether we needed to track actual changes or if this was good enough. I think in general it is usually rare that we would receive a change event from scratch-blocks where a change did not occur. This is unlike the other cases (create, move, delete) where we get the respective events from scratch-blocks but effectively no change occurs in the VM.

I don't think it's possible to get a change event for a block without some user interaction with the block. @picklesrus, I would be interested to know, can you think of scenarios where this would happen?

I think I'll probably remove this TODO comment.

rschamp · 2019-01-30T20:31:46Z

src/virtual-machine.js

                }
                if (projectVersion === 3) {
-                    return this._addSprite3(validatedInput[0], validatedInput[1]);
+                    return this._addSprite3(validatedInput[0], validatedInput[1])
+                        .then(() => this.runtime.emitProjectChanged());


Since we reject in case of neither of these, can't we put this in the next then()? I.e. chained to .then(validatedInput =>...).then(/* here? */)?

Yep, that makes sense! Good catch!

rschamp · 2019-01-30T20:33:03Z

src/virtual-machine.js

-        this.runtime.on(Runtime.TARGETS_UPDATE, () => {
-            this.emitTargetsUpdate();
+        this.runtime.on(Runtime.TARGETS_UPDATE, emitProjectChanged => {
+            this.emitTargetsUpdate(emitProjectChanged);


Is this ever true?

Right now it isn't, but I don't know whether it's better to just assume always false here...

In a future pass we plan to completely separate out targets update and project changed, so we can fix this there then too. I'll file an actual issue for it.

Filed in #1964

kchadha · 2019-01-30T20:37:36Z

Towards resolving scratchfoundation/scratch-gui#4275

…`.then` (at the higher level)

kchadha assigned paulkaplan and rschamp Jan 30, 2019

kchadha added the pr - needs review label Jan 30, 2019

kchadha added this to the February 2019 milestone Jan 30, 2019

kchadha added 5 commits January 30, 2019 10:59

Add tests for project changed state.

df823a0

Fix tests for reordering costumes and sounds.

ee936e3

Add tests for project state changes emitted by editing blocks.

b2f63cd

Emit project changes when actually editing blocks instead of whenever…

e276bcc

… scratch-blocks events are fired.

Stop emitting project changes within 'runtime._step()'. Emit project …

dfcd09f

…changes in scenarios that were previously taking advantage of runtime._step()

kchadha added the needs-qa label Jan 30, 2019

kchadha mentioned this pull request Jan 30, 2019

Update project changes test scratchfoundation/scratch-gui#4457

Merged

rschamp suggested changes Jan 30, 2019

View reviewed changes

rschamp assigned kchadha and unassigned rschamp Jan 30, 2019

kchadha mentioned this pull request Jan 30, 2019

Separate targets update and project changed #1964

Open

Remove superfluous runtime not optional warnings.

94cc7e6

kchadha force-pushed the project-dirty-state-fixes branch from 5a4a11b to b270fae Compare January 30, 2019 21:44

Code cleanup -- move out duplicate .then statements after the next …

13e69ba

…`.then` (at the higher level)

kchadha force-pushed the project-dirty-state-fixes branch from b270fae to 13e69ba Compare January 30, 2019 21:46

rschamp approved these changes Feb 5, 2019

View reviewed changes

rschamp added pr - good to merge and removed pr - needs review labels Feb 5, 2019

rschamp unassigned paulkaplan Feb 5, 2019

kchadha added QA-OK and removed needs-qa labels Feb 5, 2019

kchadha merged commit c796a8b into develop Feb 5, 2019

kchadha deleted the project-dirty-state-fixes branch February 5, 2019 21:38

Project dirty state fixes #1962

Project dirty state fixes #1962

Uh oh!

Conversation

kchadha commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Test Coverage

Uh oh!

rschamp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kchadha commented Jan 30, 2019

Uh oh!

Uh oh!

kchadha commented Jan 29, 2019 •

edited

Loading