clarkyblade / blockly

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

injecting twice will add event listener to document and window twice #141

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. inject blockly to a div twice
2. copy and paste a block with ctrl+c and ctrl+v

What is the expected output? What do you see instead?

the block is copied as many times as inject is called, it should copy it only 
once

What browser are you using?

FF 20, but it's a generic problem

Please provide any additional information below.

the fix I implemented is below, but the problem is the fact that the library is 
kind of a singleton (for example I can't inject two blockly instances on the 
same page)

I would like to start working on making blockly stateless, do you have any 
feedback on how would you approach it so I don't work on a way that won't be 
accepted?

Index: core/inject.js
===================================================================
--- core/inject.js      (revision 775)
+++ core/inject.js      (working copy)
@@ -277,7 +277,6 @@
  * @private
  */
 Blockly.init_ = function() {
-  Blockly.bindEvent_(window, 'resize', document, Blockly.svgResize);
   // Bind events for scrolling the workspace.
   // Most of these events should be bound to the SVG's surface.
   // However, 'mouseup' has to be on the whole document so that a block dragged
@@ -285,11 +284,17 @@
   // Also, 'keydown' has to be on the whole document since the browser doesn't
   // understand a concept of focus on the SVG image.
   Blockly.bindEvent_(Blockly.svg, 'mousedown', null, Blockly.onMouseDown_);
-  Blockly.bindEvent_(document, 'mouseup', null, Blockly.onMouseUp_);
   Blockly.bindEvent_(Blockly.svg, 'mousemove', null, Blockly.onMouseMove_);
   Blockly.bindEvent_(Blockly.svg, 'contextmenu', null, Blockly.onContextMenu_);
-  Blockly.bindEvent_(document, 'keydown', null, Blockly.onKeyDown_);

+  if (!Blockly.documentEventsBound) {
+      Blockly.bindEvent_(window, 'resize', document, Blockly.svgResize);
+      Blockly.bindEvent_(document, 'mouseup', null, Blockly.onMouseUp_);
+      Blockly.bindEvent_(document, 'keydown', null, Blockly.onKeyDown_);
+
+      Blockly.documentEventsBound = true;
+  }
+

Original issue reported on code.google.com by mari...@marianoguerra.org on 30 Apr 2013 at 4:14

GoogleCodeExporter commented 9 years ago
Good call.  Good patch.  It's in review and will land shortly.

Making Blockly not be a singleton would be great.  Imagine:
var blockly1 = new Blockly(document.getElementById('blocklyDiv1'), 
{path:'../'});
var blockly2 = new Blockly(document.getElementById('blocklyDiv2'), 
{path:'../'});
Or something like that.

It has been a slow process, but we are getting close.  One by one the 
singletons inside Blockly have been converted to classes that can be 
instantiated.  Blockly.ContextMenu, Blockly.Toolbox, and Blockly.Tooltip are 
examples of singletons that still need converting.  I think the only things 
that should remain singletons are Blockly.Language, Blockly.Procedures, 
Blockly.Variables, and Blockly.Xml.  Only then can we convert Blockly itself.

I'll grant you commit access to Blockly.  Just be sure to send any code to 
codereview.appspot.com for an LGTM from me before committing.
http://code.google.com/p/blockly/wiki/ContributingCode

Original comment by neil.fra...@gmail.com on 4 May 2013 at 3:49

GoogleCodeExporter commented 9 years ago
I submitted to code reviews, just stating it here in case I did something wrong 
and you didn't get the notification.

Original comment by luismarianoguerra@gmail.com on 2 Aug 2013 at 5:33

GoogleCodeExporter commented 9 years ago
Luis, you need to also click on "Publish+Mail Comments" and fill in Neil's name 
at codereview.appspot.com before he'll see it.  It's a confusing UI.

Original comment by sper...@google.com on 2 Aug 2013 at 5:41