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

Collapsed blocks do not update when modified #3784

Closed
BeksOmega opened this issue Apr 1, 2020 · 9 comments
Closed

Collapsed blocks do not update when modified #3784

BeksOmega opened this issue Apr 1, 2020 · 9 comments
Labels
component: rendering issue: bug Describes why the code or behaviour is wrong

Comments

@BeksOmega
Copy link
Collaborator

Describe the bug

Collapsed blocks do not update when they are modified. This includes:

  • Inputs added
  • Fields added
  • Field values changed
  • etc

This affects all collapsed blocks, not just the examples below.

To Reproduce

Steps to reproduce the behavior:

  1. Drag out a procedure def and a caller.
  2. Collapse the caller.
  3. Rename the procedure.
  4. Observe how the collapsed caller does not update.

Expected behavior

Collapsed blocks should update.

Screenshots

CollapsedNotUpdating2
CollapsedNotUpdating

Additional context

I fixed this in AppInventor's repo in mit-cml/appinventor-sources#2074

Here is the description of that pull request:

  1. Adds an extra rendering step to the rendering pipeline in the case that the block is collapsed. This step makes sure that the block is in a proper collapsed state (e.g. all inputs are hidden) before re-rendering the block.
  2. Fixes some bugs from core like Hidden statement inputs seem broken #1967
  3. Optimizes toString to make it super zippy. (Probably overkill, but it was fun)

I think the same strategy could work in Core as well. I would be happy to put up a pull request if you are interested.

@BeksOmega BeksOmega added issue: triage Issues awaiting triage by a Blockly team member issue: bug Describes why the code or behaviour is wrong labels Apr 1, 2020
@rachel-fenichel
Copy link
Collaborator

Yes, we're interested in a fix.

@moniika who has looked into a variety of related rendering issues to discuss and review the associated PR.

@moniika
Copy link
Contributor

moniika commented Apr 2, 2020

+1 would love fix.
Here's a related bug #3619 (probably partial dupe).

@BeksOmega
Copy link
Collaborator Author

@moniika Does the solution in the other PR look good to you? I wasn't sure how you guys would feel about the if (isRendering) return; solution to the stack overflow problem.

I just wanna check before I start trying to update it. Totally cool with whatever you guys decide.

@moniika moniika added this to the 2020_q2_release milestone Apr 3, 2020
@moniika moniika removed the issue: triage Issues awaiting triage by a Blockly team member label Apr 3, 2020
@moniika
Copy link
Contributor

moniika commented Apr 3, 2020

@moniika Does the solution in the other PR look good to you? I wasn't sure how you guys would feel about the if (isRendering) return; solution to the stack overflow problem.

I just wanna check before I start trying to update it. Totally cool with whatever you guys decide.

I'll take a closer look at that and give you a reply by end of day.

@moniika
Copy link
Contributor

moniika commented Apr 4, 2020

@moniika Does the solution in the other PR look good to you? I wasn't sure how you guys would feel about the if (isRendering) return; solution to the stack overflow problem.
I just wanna check before I start trying to update it. Totally cool with whatever you guys decide.

I'll take a closer look at that and give you a reply by end of day.

I'm not really a fan of the isRendering flag. I think we could avoid the recursive calls by updating the collapsed block text without appending field. I tried frankenstein-ing something based on your appinventor changes in #3799 and it seems to fix variables and procedure issue, but I haven't done very extensive edge case testing. Maybe we could do something similar?
@BeksOmega

@BeksOmega
Copy link
Collaborator Author

@moniika From my reading it does indeed look like that solves the variable problem. But I think there might be some other areas that need addressing.

For example, adding inputs to a collapsed block can look pretty derpy:
Collapse
(the above is an example from my L-System Playground)

Do you think your solution could be expanded to handle those cases?

My comment here also brings up some alternative solutions.

@rachel-fenichel
Copy link
Collaborator

After taking a look at your comment over on the other PR, I see why you're using isRendering. I agree that it's error-prone, but that the other solution would be a pretty big change.

So I'm okay with having a flag. I prefer calling it isRenderInProgress to make its intent more explicit.

@moniika and I discussed offline as well.

@BeksOmega
Copy link
Collaborator Author

Sounds good @rachel-fenichel @moniika I'll work on getting a draft up and running. Know that I'm cool throwing it out if we come up with something better.

@samelhusseini
Copy link
Contributor

Looks like this has been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: rendering issue: bug Describes why the code or behaviour is wrong
Projects
None yet
Development

No branches or pull requests

4 participants