On Sat, May 12, 2018, 02:58 Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote:
> Hello Ashesh, > > 1. In TreeNode, we're keepging the reference of DOMElement, do we really >>> need it? >> >> As of right now, our Tree abstraction acts as an adapter to the aciTree >>> library. The aciTree library needs the domElement for most of its functions >>> (setInode, unload, etc). Thus this is the easiest way to introduce our >>> abstraction and keep the functionality as before - at least until we decide >>> that whether we want to switch out the library or not. >> >> I understand that. But - I've not seen any reference of domElement the >> code yet, hence - pointed that out. > > If you look at the function: reload, unload you will see that domNode is > used to communicate with the ACITree > > >> 2. Are you expecting the tree class to be a singleton class >> >> Since this tree is referenced throughout the codebase, considering it to >>> be a singleton seems like the most appropriate pattern for this usecase. It >>> is very much the same way how we create a single instance of the aciTree >>> library and use that throughout the codebase. Moreover, it opens up >>> opportunities to improve performance, for example caching lockups of nodes. >>> I’m not a fan of singletons myself, but I feel like we’re simply keeping >>> the architecture the same in the instance. >> >> Yeah - I don't see any usage of tree object from anywhere. >> And, we're already creating new object in browser.js (and, not utitlizing >> that instance anywhere.) > > > You are right, we do not need to export tree as a singleton for now. The > line that exports the variable tree can be remove when applying the patch > number 2. > > > I think we addressed all the concern raised about this patch. Does this > mean that the patch is going to get committed? > Yes - from me for 0002. -- Thanks, Ashesh > We are looking forward to continue the development on this track of work, > but as you know, we are blocked until it gets committed into master. We > hope that this gets unblocked soon because this is a very big track of work > and the longer it is blocked, the longer it will take to get finalized. > > Thanks > Joao > > > On Fri, May 11, 2018 at 2:32 AM Ashesh Vashi < > ashesh.va...@enterprisedb.com> wrote: > >> On Thu, May 10, 2018 at 8:08 PM, Anthony Emengo <aeme...@pivotal.io> >> wrote: >> >>> 1. In TreeNode, we're keepging the reference of DOMElement, do we really >>>> need it? >>> >>> As of right now, our Tree abstraction acts as an adapter to the aciTree >>> library. The aciTree library needs the domElement for most of its functions >>> (setInode, unload, etc). Thus this is the easiest way to introduce our >>> abstraction and keep the functionality as before - at least until we decide >>> that whether we want to switch out the library or not. >>> >> I understand that. But - I've not seen any reference of domElement the >> code yet, hence - pointed that out. >> >>> 2. Are you expecting the tree class to be a singleton class >>> >>> Since this tree is referenced throughout the codebase, considering it to >>> be a singleton seems like the most appropriate pattern for this usecase. It >>> is very much the same way how we create a single instance of the aciTree >>> library and use that throughout the codebase. Moreover, it opens up >>> opportunities to improve performance, for example caching lockups of nodes. >>> I’m not a fan of singletons myself, but I feel like we’re simply keeping >>> the architecture the same in the instance. >>> >> Yeah - I don't see any usage of tree object from anywhere. >> And, we're already creating new object in browser.js (and, not utitlizing >> that instance anywhere.) >> >> -- Thanks, Ashesh >> >>> >>> >>> Sincerely, >>> >>> Anthony and Victoria >>> >> >>
