[BUG?] Paths not being tracked properly #487

Activity

1. trusktr commented on Aug 27, 2015

   

   trusktr

   on Aug 27, 2015

   ContributorAuthor

   More actions

   I tried reproducing with this simple example:

   var scene = FamousEngine.createScene()
   var logo = scene.addChild()
   var node2 = scene.addChild()
   scene.removeChild(logo)
   node2.addChild(logo)
   new Position(logo)
   node2.removeChild(logo)
   scene.addChild(logo)

   but that works. In my case, I'm removing and adding hundreds of nodes at a time.
2. trusktr commented on Aug 27, 2015

   

   trusktr

   on Aug 27, 2015

   ContributorAuthor

   More actions

   Alright, so I did an experiment to double check that I'm not accidentally adding nodes to the same parent node (in fact, they shouldn't be on any node at all because I'm calling removeChild elsewhere first). I've got code like this:

                       if (viewNode === cardFamousNode.getParent()) {
                           console.log('viewNode is already parent...')
                           debugger
                       }
                       viewNode.addChild(cardFamousNode)

   The console.log and debugger statements never fire, and the error happens on the line after the conditional check.
3. michaelobriena commented on Aug 27, 2015

   

   michaelobriena

   on Aug 27, 2015

   Member

   More actions

   Sorry, this is really hard to debug without the specific code. Not much we can do here.
4. artichox commented on Sep 1, 2015

   

   artichox

   on Sep 1, 2015

   More actions

   Here is the situation in which this bug occurs for me:

   class MyNode extends Node
    constructor: ()->
           super
           @.nextBtn = new NextBtnNode() #this class also extends Node
           @.addChild( @.nextBtn )
           @.moduleNodes = []
       #remove all of the current module nodes and insert the new modules  
   setModules: (modules)->
       @.modules = modules
       for node in @.moduleNodes
          @.removeChild node
       for module in @.modules
          newNode = new ModuleNode(module)
          @.moduleNodes.push newNode
          @.addChild newNode
   
   myNode = new MyNode()
   myNode.setModules( [1, 2, 3] )
   myNode.setModules( [4, 5, 6] ) #ERROR item already exists at path

   On the last line, the node's TransformSystem tries to register a new node at the path of the current NextBtn node, which was never removed. If I remove all the nodes including the NextBtn and then add all of the nodes, I don't receive an error. Thus, the path to the NextBtn node needs to update itself to reflect that it has moved from being at index 1 among all the children (for example) to index 0.
5. trusktr commented on Sep 1, 2015

   

   trusktr

   on Sep 1, 2015

   ContributorAuthor

   More actions

   @artichox Is that code who's repo you don't mind sharing?
6. trusktr commented on Sep 1, 2015

   

   trusktr

   on Sep 1, 2015

   ContributorAuthor

   More actions

   I have this problem in another project now. My case is simple. I've got some code like this:

   import Node from 'react-famous/Node'
   import {SplashScreen, SignUpScreen, LoginScreen} from './screens'
   
   // This must contain React components that extend from react-famous/Node
   let loginlayouts = {
       SplashScreen, SignUpScreen, LoginScreen
   }
   
   
   class Login extends Node {
       render() {
           return (
               <Node _famousParent  = {this} id="rootNode">
   
                   {React.createElement(this.data.loginLayout)}
   
               </Node>
           )
       }
   
       getMeteorData() {
           console.log('login root data changed.')
           return {
               loginLayout: loginlayouts[Session.get('loginlayout')]
           }
       }
   }
   reactMixin(Login.prototype, ReactMeteorData)
   
   export default Login

   When the value of this.data.loginLayout changes, the Login component is re-rendered (in React's sense of rendering). The Node instance in the return function creates a Famous Node behind the scenes, and that instance remains instantiated across React renders. What changes is the value of {React.createElement(this.data.loginLayout)}. The value of that is a component that looks like this:

   import Node from 'react-famous/Node'
   import Node from 'react-famous/dom-rederables/DOMElement'
   
   export default
   class SplashScreen extends Node {
       render() {
           return (
               <Node _famousParent={this} id="SplashRoot">
                   <DOMElement>
                       <h1>...</h1>
                   </DOMElement>
                   <Node>
                       ...
                   </Node>
               </Node>
           )
       }
   }

   When the value of {React.createElement(this.data.loginLayout)} in the Login component changes, that causes the (React) component to be unmounted, which behind the scene unmounts the Famous Node that it represents. Then, the new value of {React.createElement(this.data.loginLayout)} is another React component that, when mounted (in React's sense of mounting) goes a head and creates a new Famous Node and adds that with addChild to the Login component's Famous Node.

   So, in my case, all that is happening (when the value of this.data.loginLayout changes) is a single node is removed (and all it's sub-nodes along with it) and a single node is added (and all it's sub-nodes along with it), and that's when I get the error.

   Maybe the index in one of the loops in the path logic is off by one or something?
7. trusktr commented on Sep 1, 2015

   

   trusktr

   on Sep 1, 2015

   ContributorAuthor

   More actions

   @artichox I'm gonna look into fixing this today.
8. artichox commented on Sep 2, 2015

   

   artichox

   on Sep 2, 2015

   More actions

   Great thank you @trusktr
   Here is the repo
   client/modules/base/ModulesView.coffee is the node class. setModules is called in client/Scene.coffee

   Also, I was mistaken: the bug still persists if I remove all of the child nodes including the NextBtn
9. trusktr commented on Sep 2, 2015

   

   trusktr

   on Sep 2, 2015

   ContributorAuthor

   More actions

   @artichox I didn't get to solve this today. I found a temporary workaround to the problem in my case: I'm swapping views by positioning them off screen or on screen instead of adding/removing nodes. It's lame, but that'll do for now.
10. talves commented on Sep 2, 2015

    

    talves

    on Sep 2, 2015

    More actions

    @trusktr this should be one of the highest priorities to fix
11. trusktr commented on Sep 2, 2015

    

    trusktr

    on Sep 2, 2015

    ContributorAuthor

    More actions

    @talves @michaelobriena @alexanderGugel As mentioned on Slack in #engine, the current workaround is to manually remove child nodes before removing parent nodes, in post-order. I'm gonna test that out.
12. artichox commented on Sep 3, 2015

    

    artichox

    on Sep 3, 2015

    More actions

    Manually removing all of the child nodes in the tree does prevent path collisions, but unfortunately the recycling of DOM elements will sometimes result in some of the node's content being retained. So when I make a new node, it occasionally will have the content of one of my previous nodes in its background. Pretty funky looking. Preemptively setting content to "" in the node constructor doesn't fix the issue. Anyone found a workaround for this?
13. trusktr commented on Sep 8, 2015

    

    trusktr

    on Sep 8, 2015

    ContributorAuthor

    More actions

    @artichox Maybe we can override a method in the dom renderer to not recycle anything, for now?
14. CompSciFutures

    added a commit that references this issue on Feb 1, 2016

    fix issue with cached elements having lingering style data when they …

    ...

    12909a2