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 :-) -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
