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

bug: disabling/enabling blocks not consitent #7113

Closed
1 task done
mrkprdo opened this issue May 23, 2023 · 4 comments · Fixed by #7172 or #7650
Closed
1 task done

bug: disabling/enabling blocks not consitent #7113

mrkprdo opened this issue May 23, 2023 · 4 comments · Fixed by #7172 or #7650
Assignees
Labels
issue: bug Describes why the code or behaviour is wrong
Milestone

Comments

@mrkprdo
Copy link

mrkprdo commented May 23, 2023

Check for duplicates

  • I have searched for similar issues before opening a new one.

Description

if one of the inner block is disabled, and then you disabled few blocks on the top.
saving the json/xml and loading it again, if you enable the top level block it does not get enabled, but the context menu has already changed to "disable block", you have to disable and enable it again to work with the block.

Reproduction steps

  1. Load this json in the playground, this is a simple if-condition block
{
 "blocks": {
   "languageVersion": 0,
   "blocks": [
     {
       "type": "controls_if",
       "id": "wGMY60,6:n?|G}f|zdt{",
       "x": 16,
       "y": 8,
       "enabled": false,
       "inputs": {
         "IF0": {
           "block": {
             "type": "logic_compare",
             "id": "d,5g$byf^3xi`z9`gv6*",
             "fields": {
               "OP": "EQ"
             },
             "inputs": {
               "A": {
                 "block": {
                   "type": "variables_get",
                   "id": "M4KwCD]HsT.Y76^8h8*0",
                   "fields": {
                     "VAR": {
                       "id": "guRlyqX$KE+(,eMeqbmp"
                     }
                   }
                 }
               },
               "B": {
                 "block": {
                   "type": "text",
                   "id": "e^gV0l)OCAFsN9}~)ok_",
                   "fields": {
                     "TEXT": "Hello World"
                   }
                 }
               }
             }
           }
         },
         "DO0": {
           "block": {
             "type": "text_print",
             "id": "U^hRVv/ZefKP:%UmLwAr",
             "inputs": {
               "TEXT": {
                 "shadow": {
                   "type": "text",
                   "id": "ce%/Ftbh0o!0Yj-Qu;]l",
                   "fields": {
                     "TEXT": "abc"
                   }
                 },
                 "block": {
                   "type": "variables_get",
                   "id": "a~/}=W]C``GDHr(Y=_J1",
                   "enabled": false,
                   "fields": {
                     "VAR": {
                       "id": "guRlyqX$KE+(,eMeqbmp"
                     }
                   }
                 }
               }
             }
           }
         }
       }
     }
   ]
 },
 "variables": [
   {
     "name": "asd",
     "id": "guRlyqX$KE+(,eMeqbmp"
   }
 ]
}
  1. Enable the top level block which is the if-block.
  2. It does not get enabled, but context menu has changed.
  3. The only thing that has been done here, is the getter block for asd is disabled before disabling the entire blocks.
  4. Regardless of inner block is changed, as long as you re-load the json definition and re-enable the block, it is still reproducible.

Stack trace

No response

Screenshots

image

Browsers

No response

@mrkprdo mrkprdo added issue: bug Describes why the code or behaviour is wrong issue: triage Issues awaiting triage by a Blockly team member labels May 23, 2023
@maribethb maribethb added this to the Upcoming milestone May 24, 2023
@maribethb maribethb removed the issue: triage Issues awaiting triage by a Blockly team member label May 24, 2023
@mrkprdo
Copy link
Author

mrkprdo commented Oct 26, 2023

Hi @maribethb,
After upgrading to v10.2.2, we still have this issue on our app, however trying to reproduce this on the playground again:
both here:
https://blockly-demo.appspot.com/static/tests/playgrounds/advanced_playground.html#debugEnabled=false
and
https://blockly-demo.appspot.com/static/tests/playground.html
They seem to be working... is the playground using the latest version too (v10.2.2 as of this writing)?

@BeksOmega BeksOmega reopened this Nov 13, 2023
@BeksOmega BeksOmega added the issue: triage Issues awaiting triage by a Blockly team member label Nov 13, 2023
@maribethb
Copy link
Contributor

I can only reproduce this in XML, not in JSON. The advanced playground uses XML for its save capabilities so that's why you can reproduce it there but not by loading JSON in the regular simple playground. I wrote a test case for xml equivalent to the json test case I wrote in #7172 and can confirm it fails. Guess I should have written that at the time too.

I confirmed this bug was also present in XML in Blockly v9.3.1 so I'm not sure why I thought it wasn't when I fixed this for XML. I did notice it wasn't present in XML in 9.1.1 however.

@mrkprdo are you serializing in json or xml in your app? You provided json for the reproduction steps above, but if you're still experiencing this in your app, is it with xml? I can't repro with JSON at all right now.

@mrkprdo
Copy link
Author

mrkprdo commented Nov 13, 2023

@maribethb that's right, i am serializing in xml.

@maribethb
Copy link
Contributor

maribethb commented Nov 14, 2023

When you call domToWorkspace (xml)

  1. calls domToBlockInternal
  2. The top block and its children are created
  3. Those blocks get initSvg called on them <- disabled rendering is done here during normal rendering. importantly visuallyDisabled is not set. only the value of block.isEnabled() is checked.
  4. updateDisabled is called on the top block only
  5. if the top block is not disabled, we still call updateDisable on the next blocks but not on the child blocks
  6. Return early, so children blocks never get their visually disabled status set if their parent is not disabled

other note: the fix in #7172 was effectively removed in #7308 so investigating that. adding it back to the renderEfficiently method makes the test pass

in json, it works because updateDisabled is called in initBlock which is called on every single block, not just the top blocks. however, initBlock also has a TODO suggesting that it's not the correct place to do that kind of state management.

so I think our options are
a) add back the fix in #7172 that was removed in #7308 - in other words, call updateDisabled when the block is rendered
- not sure why it was removed so can't say whether this should be done or not
b) call updateDisabled for all the blocks during xml serialization, not just top blocks
- would be more similar to what we do in json, however the json implementation implies it's not optimal
- isn't ideal calling it on every block, because the method itself also calls itself for next blocks, and sometimes for child blocks, so redundant work is done
c) change updateDisabled to always call itself on the child blocks (regardless of if the parent's disabled status matches)
d) add a parameter to updateDisabled so that when we call it during serialization we can have it always work on the children too, but when just calling it normally (e.g. from context menu click) it doesn't

@maribethb maribethb removed the issue: triage Issues awaiting triage by a Blockly team member label Nov 16, 2023
@maribethb maribethb self-assigned this Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug Describes why the code or behaviour is wrong
Projects
None yet
3 participants