On Mon, May 14, 2018 at 6:10 PM, Dave Page <dp...@pgadmin.org> wrote:
> > > On Mon, May 14, 2018 at 1:38 PM, Ashesh Vashi < > ashesh.va...@enterprisedb.com> wrote: > >> On Mon, May 14, 2018 at 2:59 PM, Dave Page <dp...@pgadmin.org> wrote: >> >>> >>> >>> On Sat, May 12, 2018 at 12:10 AM, Ashesh Vashi < >>> ashesh.va...@enterprisedb.com> wrote: >>> >>>> 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. >>>> >>> >>> Can you do that today? >>> >> Done. >> > > Great, thanks! > > On to patch 0003 then :-) > Yes - already working on it! :-) -- Thanks, Ashesh > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
