Remove usage of potentialVariableMap #5656

JerryHue · 2021-11-01T11:52:56Z

The basics

I branched from develop
My pull request is against develop
My code follows the style guide

The details

Resolves

Proposed Changes

To avoid triggering a refresh, we will not create a variable like main workspaces do. Instead, we create a variable by accessing the variable map directly.

Behavior Before Change

Before, when creating a new variable, the flyout workspace would use potentialVariableMap to keep track of such variables that are not being used.

Behavior After Change

Now, flyout workspace is using its own variableMap, removing the unnecessary references to potentialVariableMap.

Reason for Changes

Simplify the definition of what a workspace is (potentialVariableMap was a property used in the special case of a flyout workspace).

Test Coverage

I tested on the following browsers:

Desktop Firefox

The way I tested is by creating and deleting variables and making sure that it behaved the same way as before the changes.

Documentation

I believe some sparse documentation needs to be updated, since the potentialVariableMap was heavily involved on the usage of flyout workspaces. I tried to reword some comments that I could find.

Additional Information

No additional information has to be added.

To avoid triggering a refresh, we will not create a variable like main workspaces do. Instead, we create a variable by accessing the variable map directly.

google-cla · 2021-11-01T11:52:59Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

JerryHue · 2021-11-01T11:54:54Z

@googlebot I signed it!

maribethb

Hello, thank you for the PR! I have some comments that hopefully help clarify what would be needed here, but after looking at how the logic changes, I'm not sure that we should continue forward with this refactor. I'm going to post in the bug about it in case there is further discussion. Thank you, and sorry both that it took us a while to respond to your PR and that this refactor may not be needed after all.

maribethb · 2021-11-24T20:05:33Z

core/variables.js

  let variable = null;
  // Try to just get the variable, by ID if possible.
  if (id) {
-    // Look in the real variable map before checking the potential variable map.
+    // Look in the variable map


The main idea behind this refactor is that for any workspace, we have two properties: variableMap and potentialVariableMap. "Real" workspaces (not flyouts) use the variableMap but not the potentialVariableMap and flyout workspaces use the potentialVariableMap but not the variableMap (mostly, see below). We could simplify this logic by just using the variableMap for both use cases. We still need the concept of "potential" variables that exist in the flyout but not the main workspace, but we can store those in the same property to simplify things.

The flyout was using the variableMap property to store the variableMap for the target workspace. However, you can just access that via workspace.targetWorkspace.getVariableMap() which frees you up to use the variableMap property for the "potential" variables.

For this part, you still need to do what the comment you deleted says: look in the real variable map before checking for potential variables. In our new code, that means if we're in a flyout, we need to check the target workspace's variable map first (real variables) and then check the flyout's variable map (potential variables) if needed. If we're not in a flyout, just check the variable map (real variables).

So you can do something like

if (workspace.isFlyout) { // Check for real variables in the target workspace first. variable = workspace.targetWorkspace.getVariableById(id); } if (!variable) { // Either this is a flyout and the main workspace didn't contain the variable, so check for potential variables, or this wasn't a flyout workspace so just check for real ones. variable = workspace.getVariableById(id); }

So looking at this logic, I'm not sure that we should follow through with this refactor. It simplifies things in one respect, because we are eliminating the potentialVariableMap property which is unused for main workspaces. However, now we are using one property to mean two different things depending on whether it's a flyout workspace or not, which is a different kind of complexity that comes through on the comment for that second if statement.

I can confirm that @maribethb is correct that, when a workspace is a Flyout's workspace, one needs to look at the real VariableMap stored on the targetWorkspace first.

Since:

both the Flyout and it's .workspace_ have a .targetWorkspace property, and

on the workspace, that is only set if .isFlyout is true, and

the flyout's own .workspace_ will always be a WorkspaceSvg rather than a base Workspace,

I propose the following approach:

On WorkspaceSvg:

override getVariableById so that if .isFlyout is set it will try this.targetWorkspace.getVariableById(id) first. If not (or if that does not return a Variable), then do a super call as usual.

N.B.: our convention is to do that via <DerivedClass>.superClass_.<method>.call(this, <arguments...>))

Be sure to document the reason for this. I suggest you start with the deleted comment and edit as appropriate to explain what is now going on.

modify the override of createVariable so that it only calls refreshToolboxSelection if this.isFlyout is false (otherwise you will discover that opening certain toolbox sections causes an infinite loop). (Or feel free to propose a better solution if you'd like.)

I agree with @maribethb that having a bit of magic in the lookups (now in the WorkspaceSvg overrides) is undesirable (all else being equal), but I think the basic idea proposed by Beka in #5656 is still a good one: specifically, because the "potential" variables are actually variables that really appear in blocks on the flyout, it makes perfect sense that they should appear in the flyout's workspace's variable map.

(@maribethb: if the above is still disagreeable, then my alternative propsoal would be to modify VariableMap so that instances can have a "parent" map—sort of like how JS objects have a [[Prototype]], only with lookups being done on the parent first, before looking locally.)

maribethb · 2021-11-24T20:22:02Z

core/variables.js

@@ -493,9 +489,6 @@ const getVariable = function(workspace, id, opt_name, opt_type) {
    }
    // Otherwise look up by name and type.
    variable = workspace.getVariable(opt_name, opt_type);


We would need to do the same kind of logic here to check the target workspace first.

(Unless we move the code to WorkspaceSvg.prototype.getVariableById as I propose above.)

maribethb · 2021-11-24T20:24:59Z

core/variables.js

  // Variables without names get uniquely named for this workspace.
  if (!opt_name) {
    const ws = workspace.isFlyout ? workspace.targetWorkspace : workspace;
    opt_name = exports.generateUniqueName(ws);
  }

-  // Create a potential variable if in the flyout.
+  // Create a variable directly in the variable map if in the flyout.


This section is where the refactor would simplify things. workspace.getVariableMap().createVariable is equivalent to workspace.createVariable. No matter if we are in a flyout or a main workspace, after this refactor, we would just create it in the variable map so we could eliminate the if statements here and just do

variable = workspace.createVariable(...)

However again, looking at this simplification vs the additional complexity introduced when looking for a variable, I'm not sure it's worth doing.

I concur about the change required: the entire second section of the function can be replace by:

return workspace.createVariable(opt_name, opt_type, id);

(which is in part why I think this change is worth doing).

JerryHue · 2021-11-25T00:44:25Z

Hello, @maribethb! I understand the points that you have made, and I agree with them.

As a side note, I chose this specific issue to work on since it seems less complex and it helped me dive into the codebase and understand things a little bit better. I appreciate your response. Thank you.

cpcallen

I've looked at this PR in detail and, with some considerable assistance from @NeilFraser, I now believe I properly understand the original bug, this PR, and the relevant parts of the underlying codebase. Big props to you, @JerryHue for picking up this issue—even if it was maybe a little more thorny than you expected!

I think we should proceed with the proposed changes, though there are changes required as noted by @maribethb. See inline for my detailed response to her comment.

cpcallen · 2022-02-04T14:54:24Z

core/variables.js

  let variable = null;
  // Try to just get the variable, by ID if possible.
  if (id) {
-    // Look in the real variable map before checking the potential variable map.
+    // Look in the variable map


I can confirm that @maribethb is correct that, when a workspace is a Flyout's workspace, one needs to look at the real VariableMap stored on the targetWorkspace first.

Since:

both the Flyout and it's .workspace_ have a .targetWorkspace property, and

on the workspace, that is only set if .isFlyout is true, and

the flyout's own .workspace_ will always be a WorkspaceSvg rather than a base Workspace,

I propose the following approach:

On WorkspaceSvg:

override getVariableById so that if .isFlyout is set it will try this.targetWorkspace.getVariableById(id) first. If not (or if that does not return a Variable), then do a super call as usual.

N.B.: our convention is to do that via <DerivedClass>.superClass_.<method>.call(this, <arguments...>))

Be sure to document the reason for this. I suggest you start with the deleted comment and edit as appropriate to explain what is now going on.

modify the override of createVariable so that it only calls refreshToolboxSelection if this.isFlyout is false (otherwise you will discover that opening certain toolbox sections causes an infinite loop). (Or feel free to propose a better solution if you'd like.)

I agree with @maribethb that having a bit of magic in the lookups (now in the WorkspaceSvg overrides) is undesirable (all else being equal), but I think the basic idea proposed by Beka in #5656 is still a good one: specifically, because the "potential" variables are actually variables that really appear in blocks on the flyout, it makes perfect sense that they should appear in the flyout's workspace's variable map.

(@maribethb: if the above is still disagreeable, then my alternative propsoal would be to modify VariableMap so that instances can have a "parent" map—sort of like how JS objects have a [[Prototype]], only with lookups being done on the parent first, before looking locally.)

cpcallen · 2022-02-04T15:04:15Z

core/variables.js

  let variable = null;
  // Try to just get the variable, by ID if possible.
  if (id) {
-    // Look in the real variable map before checking the potential variable map.
+    // Look in the variable map


I think this vestigial comment can be deleted.

cpcallen · 2022-02-04T15:07:37Z

core/variable_map.js

@@ -358,9 +358,6 @@ VariableMap.prototype.getVariablesOfType = function(type) {
 VariableMap.prototype.getVariableTypes = function(ws) {
  const variableMap = {};
  object.mixin(variableMap, this.variableMap_);


While you're modifying this function, please change it to use Object.assign instead of object.mixin.

cpcallen · 2022-02-04T15:07:47Z

core/variable_map.js

-  if (ws && ws.getPotentialVariableMap()) {
-    object.mixin(variableMap, ws.getPotentialVariableMap().variableMap_);
-  }


This eslint warning is a sign that in fact this code does need to be retained, but modified appropriately:

if (ws) { Object.assign(variableMap, ws.getVariableMap().variableMap_); }

cpcallen · 2022-02-04T15:09:04Z

core/variables.js

@@ -493,9 +489,6 @@ const getVariable = function(workspace, id, opt_name, opt_type) {
    }
    // Otherwise look up by name and type.
    variable = workspace.getVariable(opt_name, opt_type);


(Unless we move the code to WorkspaceSvg.prototype.getVariableById as I propose above.)

cpcallen · 2022-02-04T15:10:58Z

core/variables.js

  // Variables without names get uniquely named for this workspace.
  if (!opt_name) {
    const ws = workspace.isFlyout ? workspace.targetWorkspace : workspace;
    opt_name = exports.generateUniqueName(ws);
  }

-  // Create a potential variable if in the flyout.
+  // Create a variable directly in the variable map if in the flyout.


I concur about the change required: the entire second section of the function can be replace by:

return workspace.createVariable(opt_name, opt_type, id);

(which is in part why I think this change is worth doing).

BeksOmega · 2022-03-28T22:51:39Z

I'm going to go ahead and close this as stale. Feel free to reopen if you're still interested in working on this! Or always feel free to reach out about other places you can contribute if you are interested =)

Remove usage of potentialVariableMap

15479db

To avoid triggering a refresh, we will not create a variable like main workspaces do. Instead, we create a variable by accessing the variable map directly.

JerryHue requested a review from a team as a code owner November 1, 2021 11:52

JerryHue requested a review from cpcallen November 1, 2021 11:52

google-cla bot added the cla: no Used by Google's CLA checker. label Nov 1, 2021

google-cla bot added cla: yes Used by Google's CLA checker. and removed cla: no Used by Google's CLA checker. labels Nov 1, 2021

alschmiedt assigned cpcallen Nov 1, 2021

cpcallen mentioned this pull request Nov 1, 2021

Unable to merge PRs from external contributors due to check failures #5659

Closed

maribethb reviewed Nov 24, 2021

View reviewed changes

maribethb mentioned this pull request Nov 24, 2021

Cleanup handling of potential variables. #5273

Open

cpcallen requested changes Feb 4, 2022

View reviewed changes

BeksOmega closed this Mar 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove usage of potentialVariableMap #5656

Remove usage of potentialVariableMap #5656

JerryHue commented Nov 1, 2021

google-cla bot commented Nov 1, 2021

JerryHue commented Nov 1, 2021

maribethb left a comment

maribethb Nov 24, 2021

cpcallen Feb 4, 2022 •

edited

Loading

maribethb Nov 24, 2021

cpcallen Feb 4, 2022

maribethb Nov 24, 2021

cpcallen Feb 4, 2022

JerryHue commented Nov 25, 2021

cpcallen left a comment

cpcallen Feb 4, 2022 •

edited

Loading

cpcallen Feb 4, 2022

cpcallen Feb 4, 2022

cpcallen Feb 4, 2022

cpcallen Feb 4, 2022

cpcallen Feb 4, 2022

BeksOmega commented Mar 28, 2022

Remove usage of potentialVariableMap #5656

Remove usage of potentialVariableMap #5656

Conversation

JerryHue commented Nov 1, 2021

The basics

The details

Resolves

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Documentation

Additional Information

google-cla bot commented Nov 1, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

JerryHue commented Nov 1, 2021

maribethb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcallen Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JerryHue commented Nov 25, 2021

cpcallen left a comment

Choose a reason for hiding this comment

cpcallen Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeksOmega commented Mar 28, 2022

cpcallen Feb 4, 2022 •

edited

Loading

cpcallen Feb 4, 2022 •

edited

Loading