Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup handling of potential variables. #5273

Open
BeksOmega opened this issue Aug 2, 2021 · 6 comments
Open

Cleanup handling of potential variables. #5273

BeksOmega opened this issue Aug 2, 2021 · 6 comments

Comments

@BeksOmega
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

Potential variables are variables that we don't want to exist on the main workspace. For example, if you want blocks in your flyout to reference variables, but you don't want those variables to be accessible in the main workspace until the block is dragged into the main workspace.

The problem is, we shouldn't need a separate potentialVariableMap_ which is only available on workspaces that are flyouts. We should be able to use the normal variableMap_ of the workspace that is a flyout.

Describe the solution you'd like

Use the normal variableMap_ of the workspace that is the flyout, instead of creating a separate property that is only used by flyouts. This would require modifying the flyout, workspace, and variables namespace.

Additional context

The potential variable map is package, so this shouldn't be a breaking change.

@BeksOmega BeksOmega added type: cleanup issue: triage Issues awaiting triage by a Blockly team member labels Aug 2, 2021
@NeilFraser NeilFraser added component: variables and removed issue: triage Issues awaiting triage by a Blockly team member labels Aug 4, 2021
@NeilFraser NeilFraser added this to the Bug Bash Backlog milestone Aug 4, 2021
@BeksOmega BeksOmega added the help wanted External contributions actively solicited label Aug 7, 2021
@JerryHue
Copy link

Hello! I would like to give this issue a try. May I get assigned this issue to me?

@cpcallen
Copy link
Contributor

Hello! I would like to give this issue a try. May I get assigned this issue to me?

Yes!

@maribethb
Copy link
Contributor

Hey Beka (and team),

after looking into a PR that tries to refactor this, I'm not sure it actually simplifies things. More details here: #5656 (comment)

with the tldr: "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." (the complexity being you still need to check the target workspace's variablemap first)

So I'm leaning towards not changing anything here after all, what do you think?

@BeksOmega
Copy link
Collaborator Author

@maribethb yeah I feel you. Having complexity hidden in comments is not a great look. However, having properties of a superclass that is only used by a (sort-of) subclass doesn't smell good either. Imo the property feels like an acretion/band-aid rather than a principled abstraction. Like, the workspace has knowledge of this specific use case the flyout has, which it shouldn't know about, which makes the design feel inelegant.

So I still don't think that the potentialVariableMap is a good solution, and I think I still personally lean toward unifying the properties. But if you think two properties is less confusing than one I totally hear you (explicit complexity is better than implicit!), and that sounds good to me.

@maribethb
Copy link
Contributor

Yeah, I hear you, it does feel weird having the workspace know about the potential variable map. After looking at it again, it doesn't seem like it needs to. The flyout could have its own potentialVariableMap stored on the flyout class instead of the workspace class, right? Would that be better? I don't think that would significantly change any of the logic when we're checking for variables, but it would move the property from workspace to flyout.

@BeksOmega
Copy link
Collaborator Author

Yesss I love that idea! But sadly I don't think it'll work b/c typing :/ I checked and apparently Flyout's aren't actually Workspace subtypes (they just seem to act like it). They are actually composed with a workspace. And sadly the variable functions expect workspaces.

I guess the variable functions could be changed to use a type-union of either a Flyout or a Workspace (which would probably be a better representation of the function's itnerface) but I imagine that'll require a lot of changes other places in the code base as well.

So yeah :/ again totally up to you. There doesn't seem to be any really good solution here lol. So whatever you think is best is cool w/ me.

@cpcallen cpcallen self-assigned this Apr 12, 2022
@BeksOmega BeksOmega removed the help wanted External contributions actively solicited label Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants