Received: from malur.postgresql.org ([2a02:16a8:dc51::56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fI9nx-0007qe-1F for pgadmin-hackers@arkaria.postgresql.org; Mon, 14 May 2018 09:30:05 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fI9nv-0005Dp-8g for pgadmin-hackers@arkaria.postgresql.org; Mon, 14 May 2018 09:30:03 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fI9nu-0005DT-N7 for pgadmin-hackers@lists.postgresql.org; Mon, 14 May 2018 09:30:03 +0000 Received: from mail-wm0-x233.google.com ([2a00:1450:400c:c09::233]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fI9nq-00075b-VT for pgadmin-hackers@postgresql.org; Mon, 14 May 2018 09:30:01 +0000 Received: by mail-wm0-x233.google.com with SMTP id j5-v6so13795429wme.5 for ; Mon, 14 May 2018 02:29:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Mp7nbPgvWMYRXDhd4r+fv2seqPU39BrMJFdo2Z1eypQ=; b=omuc/sktkffe6bs1HYWwfWp2f4hiHNiCRMbqIC5eiNim1Oa7beYIcl9gpKvUXDPgye FDjJois/wrqzXyUA1FlxgWqNkNx40O3JjL6e7cI3DgpkeU007dqluKG9nHtZHYYaUhm0 IAkGQJWOb/4tQ9MfNLxYsPA2IYp+B7jn7gK/hfhQnLn9knEm/daefoTOAywyvNvfDiR3 HTxN7n1bnL4adwDajPDa+GoU/K/IbAz+DurRL+DWHv88tuw1JcisoY+JrS3rjIHPUa1q 0z+bY4vFXPo603CIXNEiCVXWNN1okmkvm5tmZRjS/xuUFEAVEGVebtguW1FUC8WPKC92 Ftfg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Mp7nbPgvWMYRXDhd4r+fv2seqPU39BrMJFdo2Z1eypQ=; b=rU2a7E05kaGweQ1acqZJf8vfTYq8EaofZH63jj8EdtSoHnFRnR+Lie1i+VDVp2nnTD xuYS7FyXXTJCAFNMOjhoOmbwC7DnoPIFXp4SCWDyX3rhojHiRVHowV0Z3jpnToeiSAGh 33uBKCU/fPeI56vnK9TVTYiAGLtK/JHbCTAu23yejwyyvkSSSMJ7YvCQi1RDph3v/6UK b2X13KkaKEyz/BKR4sPMOQ8zb0q2LFa0m8SjaCSRTGA9SJ/qiOys4L/rr30fUIQRQEYY CKKyPd+0fGtyjndqOtJo35tCfN5kyncoTtDG2Vn7735HgYRxdnPv4cIm6SKGucoHn+8x gwxg== X-Gm-Message-State: ALKqPwe26IuP5/Y2WIGeWULcMyGfScT3zaU4cPSCT2DIGDdtWkk8UHU/ N4Okc5gTjG+rXKmxpC7r4vTLfi6xle2Ajw9WdJY1+A== X-Google-Smtp-Source: AB8JxZo/nmRC+gti7i5cKrVXR8RCQZ1mOyZzNSSvdwiye9fIJtLFJE9+mLo4FlqO9DjaJDI5wbGNYuMea5mcTjQqOhE= X-Received: by 2002:a1c:4489:: with SMTP id r131-v6mr5118334wma.140.1526290197190; Mon, 14 May 2018 02:29:57 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.85.194 with HTTP; Mon, 14 May 2018 02:29:56 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Mon, 14 May 2018 10:29:56 +0100 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Ashesh Vashi Cc: Joao Pedro De Almeida Pereira , Anthony Emengo , "Akshay Joshi (EDB)" , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="0000000000000730e7056c27217b" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000000730e7056c27217b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sat, May 12, 2018 at 12:10 AM, Ashesh Vashi < ashesh.vashi@enterprisedb.com> wrote: > On Sat, May 12, 2018, 02:58 Joao De Almeida Pereira < > jdealmeidapereira@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 func= tions >>>> (setInode, unload, etc). Thus this is the easiest way to introduce our >>>> abstraction and keep the functionality as before - at least until we d= ecide >>>> 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 >> =E2=80=8B >> >>> 2. Are you expecting the tree class to be a singleton class >>> >>> Since this tree is referenced throughout the codebase, considering it t= o >>>> be a singleton seems like the most appropriate pattern for this usecas= e. It >>>> is very much the same way how we create a single instance of the aciTr= ee >>>> library and use that throughout the codebase. Moreover, it opens up >>>> opportunities to improve performance, for example caching lockups of n= odes. >>>> I=E2=80=99m not a fan of singletons myself, but I feel like we=E2=80= =99re 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. >> =E2=80=8B >> >> 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? --=20 Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --0000000000000730e7056c27217b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Sat, May 12, 2018 at 12:10 AM, Ashesh Vashi <ashesh.vas= hi@enterprisedb.com> wrote:
On Sat, May 12, 2018, 02:58 Joao De Almeida Pereira <jdealmeidapereir= a@pivotal.io> wrote:
Hello Ashesh,

1. In TreeNode, we're keepgin= g the reference of DOMElement, do we really need it?

=

As of right now, our=C2=A0Tree=C2=A0abstraction acts as an adapter to = the aciTree library. The aciTree library needs the domElement for most of i= ts functions (setInode, unload, etc). Thus this is the easiest way to intro= duce 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 domEle= ment the code yet, hence - pointed that out.=C2=A0

If you look at the function: reload, unload you will see that= domNode is used to communicate with the ACITree

=E2=80=8B

2.=C2=A0Are you expecting the tree class to be a= singleton class

Since this tr= ee is referenced throughout the codebase, considering it to be a singleton = seems like the most appropriate pattern for this usecase. It is very much t= he same way how we create a single instance of the aciTree library and use = that throughout the codebase. Moreover, it opens up opportunities to improv= e performance, for example caching lockups of nodes. I=E2=80=99m not a fan = of singletons myself, but I feel like we=E2=80=99re simply keeping the arch= itecture the same in the instance.
Yeah - I don't se= e any usage of tree object from anywhere.
And, we're already creatin= g 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 no= w. The line that exports the variable tree can be r= emove when applying the patch number 2.

=E2=80=8B

I think we addressed all the concern raised about this patch. Does th= is mean that the patch is going to get committed?
Yes - from me for 0002.

Can you do that today?=C2=A0
=

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

= EnterpriseDB UK: = http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--0000000000000730e7056c27217b--