Changes between Initial Version and Version 1 of Ticket #14058, comment 5


Ignore:
Timestamp:
Nov 2, 2011, 11:59:19 PM (8 years ago)
Author:
bill
Comment:

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #14058, comment 5

    initial v1  
    11Thanks, I checked over the patch.  It looks generally good, although it's a bit messy how it saves the cookie in one file but restores it in another file.  Maybe restoring the selection should occur from _dndSelector.js too?
    22
    3 I'm not too worried about the catch you mentioned above (where selected nodes are hidden).  But it doesn't make me wonder about race conditions.  You are calling set("paths", ...) at the same time the the tree itself is loading (and the data in this._openedNodes is getting applied).   Maybe the set("paths") should be delayed until after the tree finishes loading?   Or rather, wait until the tree finishes loading and then call _dndSelector.setSelected()?
     3I'm not too worried about the catch you mentioned above (where selected nodes are hidden).  But it does make me wonder about race conditions.  You are calling set("paths", ...) at the same time the the tree itself is loading (and the data in this._openedNodes is getting applied).   Maybe the set("paths") should be delayed until after the tree finishes loading?   Or rather, wait until the tree finishes loading and then call _dndSelector.setSelected()?
    44
    55Also, a minor thing, when you find a push() inside of a forEach() that probably means you should be using map() instead, for example I think that