JakAttack909 / blockly

Automatically exported from code.google.com/p/blockly
0 stars 0 forks source link

FieldTextInput calls dispose_ on showEditor_ #179

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
Inside FieldTextInput.showEditor_:

this.dispose_() is being called Instead of passing it as an argument to 
WidgetDiv.show.

This:

    Blockly.WidgetDiv.show(this, this.dispose_());

should be:

    Blockly.WidgetDiv.show(this, this.dispose_);

Original issue reported on code.google.com by mich...@enuan.com on 5 Dec 2013 at 3:47

GoogleCodeExporter commented 8 years ago
Thanks a lot.

Original comment by sper...@google.com on 12 Dec 2013 at 11:18

GoogleCodeExporter commented 8 years ago
Hold on.  I just spent the last half hour tracking down a bug that turned out 
to be caused by this fix (r1557, which I appear to have approved).

I believe that the original code was completely correct and that the current 
code is broken.  FieldTextInput.dispose_ is a closure which returns a function. 
 So executing it produces the function that will then be called by the 
WidgetDiv.

Michele, please tell me if you agree, or if you were experiencing a different 
error.
Thanks!

Original comment by neil.fra...@gmail.com on 23 Dec 2013 at 9:49

GoogleCodeExporter commented 8 years ago
Neil,

I think you're completely right.

I just checked the code that was giving me problems, here I'm using a subclass 
of Blockly.FieldTextInput to track the editing state of the field and while 
upgrading our (quite old) version of blockly that was still using 
Blockly.FieldTextInput.closeEditor I didn't notice that now 
Blockly.FieldTextInput._dispose is returning a closure, I overlooked it while 
checking the diff between our revisions.

My bad, I'm really sorry for bringing up the wrong issue and for the waste of 
your time.

Michele

Original comment by mich...@enuan.com on 23 Dec 2013 at 10:11

GoogleCodeExporter commented 8 years ago
No problem.  Closing issue.

Original comment by neil.fra...@gmail.com on 23 Dec 2013 at 7:38