From 18640d7ddc3f67d839f3f6901a401206d74acae4 Mon Sep 17 00:00:00 2001 From: Connor Hudson Date: Wed, 6 Jun 2018 15:54:12 -0400 Subject: [PATCH 01/11] use mergeWith in requestAddMonitor, requestUpdateMonitor So undefined values don't overwrite defined ones --- src/engine/runtime.js | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/engine/runtime.js b/src/engine/runtime.js index ab38955e4..5562ed9db 100644 --- a/src/engine/runtime.js +++ b/src/engine/runtime.js @@ -1522,7 +1522,17 @@ class Runtime extends EventEmitter { * @param {!MonitorRecord} monitor Monitor to add. */ requestAddMonitor (monitor) { - this._monitorState = this._monitorState.set(monitor.get('id'), monitor); + const id = monitor.get('id'); + if (this._monitorState.has(id)) { + this._monitorState = this._monitorState.set(id, this._monitorState.get(id).mergeWith((prev, next) => { + if (!next) { + return prev; + } + return next; + }, monitor)); + } else { + this._monitorState = this._monitorState.set(id, monitor); + } } /** @@ -1536,7 +1546,12 @@ class Runtime extends EventEmitter { const id = monitor.get('id'); if (this._monitorState.has(id)) { this._monitorState = - this._monitorState.set(id, this._monitorState.get(id).merge(monitor)); + this._monitorState.set(id, this._monitorState.get(id).mergeWith((prev, next) => { + if (!next) { + return prev; + } + return next; + }, monitor)); } } From 0d0543810f6ceeb774209027541696c7fbcb8793 Mon Sep 17 00:00:00 2001 From: Connor Hudson Date: Wed, 6 Jun 2018 16:00:59 -0400 Subject: [PATCH 02/11] Properly capture all variable monitor values on add --- src/engine/blocks.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/engine/blocks.js b/src/engine/blocks.js index 85ef3b204..aaf96c47d 100644 --- a/src/engine/blocks.js +++ b/src/engine/blocks.js @@ -539,7 +539,13 @@ class Blocks { params: this._getBlockParams(block), // @todo(vm#565) for numerical values with decimals, some countries use comma value: '', - mode: block.opcode === 'data_listcontents' ? 'list' : 'default' + mode: block.opcode === 'data_listcontents' ? 'list' : block.mode, + x: block.x, + y: block.y, + sliderMin: block.sliderMin, + sliderMax: block.sliderMax, + width: block.width, + height: block.height })); } break; From 77dda04228f4361aea076989ac31ed80f7aacac9 Mon Sep 17 00:00:00 2001 From: Connor Hudson Date: Wed, 6 Jun 2018 17:14:09 -0400 Subject: [PATCH 03/11] Set visible: false instead of deleting state in requestRemoveMonitor This potentially has performance implications, to be investigated later. --- src/engine/runtime.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/engine/runtime.js b/src/engine/runtime.js index 5562ed9db..cdfa8be5e 100644 --- a/src/engine/runtime.js +++ b/src/engine/runtime.js @@ -1561,7 +1561,13 @@ class Runtime extends EventEmitter { * @param {!string} monitorId ID of the monitor to remove. */ requestRemoveMonitor (monitorId) { - this._monitorState = this._monitorState.delete(monitorId); + // this._monitorState = this._monitorState.delete(monitorId); + // TODO is this performant? + if (this._monitorState.has(monitorId)) { + this._monitorState = this._monitorState.set( + monitorId, this._monitorState.get(monitorId).merge({visible: false}) + ); + } } /** From 13bf932fbbf8277558fb08f6fa9b7acde906de42 Mon Sep 17 00:00:00 2001 From: Connor Hudson Date: Thu, 7 Jun 2018 10:11:15 -0400 Subject: [PATCH 04/11] Remove unnecessary arguments to requestAddMonitor --- src/engine/blocks.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/engine/blocks.js b/src/engine/blocks.js index aaf96c47d..2ac03698d 100644 --- a/src/engine/blocks.js +++ b/src/engine/blocks.js @@ -540,12 +540,8 @@ class Blocks { // @todo(vm#565) for numerical values with decimals, some countries use comma value: '', mode: block.opcode === 'data_listcontents' ? 'list' : block.mode, - x: block.x, - y: block.y, sliderMin: block.sliderMin, - sliderMax: block.sliderMax, - width: block.width, - height: block.height + sliderMax: block.sliderMax })); } break; From 833c27ef21905d4939f695da0a15c1fdd6551f48 Mon Sep 17 00:00:00 2001 From: Connor Hudson Date: Thu, 7 Jun 2018 11:24:55 -0400 Subject: [PATCH 05/11] Remove debug comments, tweak logic for monitor data merging --- src/engine/runtime.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/engine/runtime.js b/src/engine/runtime.js index cdfa8be5e..74f2623d7 100644 --- a/src/engine/runtime.js +++ b/src/engine/runtime.js @@ -1524,8 +1524,9 @@ class Runtime extends EventEmitter { requestAddMonitor (monitor) { const id = monitor.get('id'); if (this._monitorState.has(id)) { + // Use mergeWith here to prevent undefined values from overwriting existing ones this._monitorState = this._monitorState.set(id, this._monitorState.get(id).mergeWith((prev, next) => { - if (!next) { + if (typeof next === 'undefined' || next === null) { return prev; } return next; @@ -1546,8 +1547,9 @@ class Runtime extends EventEmitter { const id = monitor.get('id'); if (this._monitorState.has(id)) { this._monitorState = + // Use mergeWith here to prevent undefined values from overwriting existing ones this._monitorState.set(id, this._monitorState.get(id).mergeWith((prev, next) => { - if (!next) { + if (typeof next === 'undefined' || next === null) { return prev; } return next; @@ -1561,8 +1563,6 @@ class Runtime extends EventEmitter { * @param {!string} monitorId ID of the monitor to remove. */ requestRemoveMonitor (monitorId) { - // this._monitorState = this._monitorState.delete(monitorId); - // TODO is this performant? if (this._monitorState.has(monitorId)) { this._monitorState = this._monitorState.set( monitorId, this._monitorState.get(monitorId).merge({visible: false}) From 99dc46ba803a61db113cdc8f153b10718e0c39f8 Mon Sep 17 00:00:00 2001 From: Connor Hudson Date: Thu, 7 Jun 2018 11:27:10 -0400 Subject: [PATCH 06/11] Add x, y to changeBlock function again after change to merge logic --- src/engine/blocks.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/engine/blocks.js b/src/engine/blocks.js index 2ac03698d..074139ac5 100644 --- a/src/engine/blocks.js +++ b/src/engine/blocks.js @@ -539,6 +539,8 @@ class Blocks { params: this._getBlockParams(block), // @todo(vm#565) for numerical values with decimals, some countries use comma value: '', + x: block.x, + y: block.y, mode: block.opcode === 'data_listcontents' ? 'list' : block.mode, sliderMin: block.sliderMin, sliderMax: block.sliderMax From 2cb276c4767e4c79a987a01e240f60329055005b Mon Sep 17 00:00:00 2001 From: Connor Hudson Date: Thu, 7 Jun 2018 15:18:45 -0400 Subject: [PATCH 07/11] Consolidate some logic in requestAddMonitor and requestUpdateMonitor --- src/engine/runtime.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/engine/runtime.js b/src/engine/runtime.js index 74f2623d7..103c7660c 100644 --- a/src/engine/runtime.js +++ b/src/engine/runtime.js @@ -1523,25 +1523,19 @@ class Runtime extends EventEmitter { */ requestAddMonitor (monitor) { const id = monitor.get('id'); - if (this._monitorState.has(id)) { - // Use mergeWith here to prevent undefined values from overwriting existing ones - this._monitorState = this._monitorState.set(id, this._monitorState.get(id).mergeWith((prev, next) => { - if (typeof next === 'undefined' || next === null) { - return prev; - } - return next; - }, monitor)); - } else { + if (!this.requestUpdateMonitor(monitor)) { // update monitor if it exists in the state + // if the monitor did not exist in the state, add it this._monitorState = this._monitorState.set(id, monitor); } } /** - * Update a monitor in the state. Does nothing if the monitor does not already + * Update a monitor in the state. Does nothing and returns false if the monitor does not already * exist in the state. * @param {!Map} monitor Monitor values to update. Values on the monitor with overwrite * values on the old monitor with the same ID. If a value isn't defined on the new monitor, * the old monitor will keep its old value. + * @returns {boolean} true if monitor exists in the state and was updated, false if it did not exist. */ requestUpdateMonitor (monitor) { const id = monitor.get('id'); @@ -1554,7 +1548,9 @@ class Runtime extends EventEmitter { } return next; }, monitor)); + return true; } + return false; } /** From 0e394fc87328f35ec10fa655f5735f3f52234607 Mon Sep 17 00:00:00 2001 From: Connor Hudson Date: Fri, 8 Jun 2018 15:59:35 -0400 Subject: [PATCH 08/11] Add monitor show/hide functions, changed remove monitor to delete state --- src/engine/runtime.js | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/src/engine/runtime.js b/src/engine/runtime.js index 103c7660c..17422c56c 100644 --- a/src/engine/runtime.js +++ b/src/engine/runtime.js @@ -1535,7 +1535,7 @@ class Runtime extends EventEmitter { * @param {!Map} monitor Monitor values to update. Values on the monitor with overwrite * values on the old monitor with the same ID. If a value isn't defined on the new monitor, * the old monitor will keep its old value. - * @returns {boolean} true if monitor exists in the state and was updated, false if it did not exist. + * @return {boolean} true if monitor exists in the state and was updated, false if it did not exist. */ requestUpdateMonitor (monitor) { const id = monitor.get('id'); @@ -1552,18 +1552,40 @@ class Runtime extends EventEmitter { } return false; } - + /** * Removes a monitor from the state. Does nothing if the monitor already does * not exist in the state. * @param {!string} monitorId ID of the monitor to remove. */ requestRemoveMonitor (monitorId) { - if (this._monitorState.has(monitorId)) { - this._monitorState = this._monitorState.set( - monitorId, this._monitorState.get(monitorId).merge({visible: false}) - ); - } + this._monitorState = this._monitorState.delete(monitorId); + } + + /** + * Hides a monitor. Does nothing if the monitor already does + * not exist in the state. + * @param {!string} monitorId ID of the monitor to remove. + * @return {boolean} true if monitor exists and was updated, false otherwise + */ + requestHideMonitor (monitorId) { + return this.requestUpdateMonitor(new Map([ + ['id', monitorId], + ['visible', false] + ])); + } + + /** + * Shows a monitor. Does nothing if the monitor already does + * not exist in the state. + * @param {!string} monitorId ID of the monitor to remove. + * @return {boolean} true if monitor exists and was updated, false otherwise + */ + requestShowMonitor (monitorId) { + return this.requestUpdateMonitor(new Map([ + ['id', monitorId], + ['visible', true] + ])); } /** From a6629628cd280608dd61042952f512aa63ceffd5 Mon Sep 17 00:00:00 2001 From: Connor Hudson Date: Fri, 8 Jun 2018 16:00:12 -0400 Subject: [PATCH 09/11] Use monitor show/hide monitor functions, remove incorrect block property references --- src/engine/blocks.js | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/engine/blocks.js b/src/engine/blocks.js index 074139ac5..58ca4a934 100644 --- a/src/engine/blocks.js +++ b/src/engine/blocks.js @@ -529,22 +529,21 @@ class Blocks { block.targetId = isSpriteSpecific ? optRuntime.getEditingTarget().id : null; if (wasMonitored && !block.isMonitored) { - optRuntime.requestRemoveMonitor(block.id); + optRuntime.requestHideMonitor(block.id); } else if (!wasMonitored && block.isMonitored) { - optRuntime.requestAddMonitor(MonitorRecord({ - id: block.id, - targetId: block.targetId, - spriteName: block.targetId ? optRuntime.getTargetById(block.targetId).getName() : null, - opcode: block.opcode, - params: this._getBlockParams(block), - // @todo(vm#565) for numerical values with decimals, some countries use comma - value: '', - x: block.x, - y: block.y, - mode: block.opcode === 'data_listcontents' ? 'list' : block.mode, - sliderMin: block.sliderMin, - sliderMax: block.sliderMax - })); + // Tries to show the monitor for specified block. If it doesn't exist, add the monitor. + if (!optRuntime.requestShowMonitor(block.id)) { + optRuntime.requestAddMonitor(MonitorRecord({ + id: block.id, + targetId: block.targetId, + spriteName: block.targetId ? optRuntime.getTargetById(block.targetId).getName() : null, + opcode: block.opcode, + params: this._getBlockParams(block), + // @todo(vm#565) for numerical values with decimals, some countries use comma + value: '', + mode: block.opcode === 'data_listcontents' ? 'list' : block.mode + })); + } } break; } From 78da15c09cc29d28cf07d5ab985c4623f97bea64 Mon Sep 17 00:00:00 2001 From: Connor Hudson Date: Fri, 8 Jun 2018 17:04:52 -0400 Subject: [PATCH 10/11] Remove blocks.mode reference --- src/engine/blocks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/blocks.js b/src/engine/blocks.js index 58ca4a934..6f795e312 100644 --- a/src/engine/blocks.js +++ b/src/engine/blocks.js @@ -541,7 +541,7 @@ class Blocks { params: this._getBlockParams(block), // @todo(vm#565) for numerical values with decimals, some countries use comma value: '', - mode: block.opcode === 'data_listcontents' ? 'list' : block.mode + mode: block.opcode === 'data_listcontents' ? 'list' : 'default' })); } } From 38756e10223c99aaa3467597309eac521bea6537 Mon Sep 17 00:00:00 2001 From: Connor Hudson Date: Mon, 11 Jun 2018 08:37:25 -0400 Subject: [PATCH 11/11] Update code comments to address PR feedback --- src/engine/runtime.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/engine/runtime.js b/src/engine/runtime.js index 17422c56c..cf6d90a67 100644 --- a/src/engine/runtime.js +++ b/src/engine/runtime.js @@ -1518,7 +1518,7 @@ class Runtime extends EventEmitter { /** * Add a monitor to the state. If the monitor already exists in the state, - * overwrites it. + * updates those properties that are defined in the given monitor record. * @param {!MonitorRecord} monitor Monitor to add. */ requestAddMonitor (monitor) { @@ -1530,8 +1530,7 @@ class Runtime extends EventEmitter { } /** - * Update a monitor in the state. Does nothing and returns false if the monitor does not already - * exist in the state. + * Update a monitor in the state and report success/failure of update. * @param {!Map} monitor Monitor values to update. Values on the monitor with overwrite * values on the old monitor with the same ID. If a value isn't defined on the new monitor, * the old monitor will keep its old value. @@ -1552,7 +1551,7 @@ class Runtime extends EventEmitter { } return false; } - + /** * Removes a monitor from the state. Does nothing if the monitor already does * not exist in the state. @@ -1563,9 +1562,8 @@ class Runtime extends EventEmitter { } /** - * Hides a monitor. Does nothing if the monitor already does - * not exist in the state. - * @param {!string} monitorId ID of the monitor to remove. + * Hides a monitor and returns success/failure of action. + * @param {!string} monitorId ID of the monitor to hide. * @return {boolean} true if monitor exists and was updated, false otherwise */ requestHideMonitor (monitorId) { @@ -1576,9 +1574,9 @@ class Runtime extends EventEmitter { } /** - * Shows a monitor. Does nothing if the monitor already does + * Shows a monitor and returns success/failure of action. * not exist in the state. - * @param {!string} monitorId ID of the monitor to remove. + * @param {!string} monitorId ID of the monitor to show. * @return {boolean} true if monitor exists and was updated, false otherwise */ requestShowMonitor (monitorId) {