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 1fHFaH-00025Y-4u for pgadmin-hackers@arkaria.postgresql.org; Fri, 11 May 2018 21:28:13 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fHFaF-000410-Nf for pgadmin-hackers@arkaria.postgresql.org; Fri, 11 May 2018 21:28:11 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fHFaF-00040q-HE for pgadmin-hackers@lists.postgresql.org; Fri, 11 May 2018 21:28:11 +0000 Received: from mx0a-00296801.pphosted.com ([148.163.150.38]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fHFaB-0004Tn-3g for pgadmin-hackers@postgresql.org; Fri, 11 May 2018 21:28:10 +0000 Received: from pps.filterd (m0114582.ppops.net [127.0.0.1]) by mx0a-00296801.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w4BLPnJX001030 for ; Fri, 11 May 2018 21:28:02 GMT Received: from mail-it0-f71.google.com (mail-it0-f71.google.com [209.85.214.71]) by mx0a-00296801.pphosted.com with ESMTP id 2hvxhkh1eh-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Fri, 11 May 2018 21:28:02 +0000 Received: by mail-it0-f71.google.com with SMTP id n190-v6so2915327itg.4 for ; Fri, 11 May 2018 14:28:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=dzRHSbL1MIlB0kzMW0bU4539XWs/XZo71hwVc2f/LJg=; b=t4CoqlNdlv0aEwsladxS00nmNe2n/UWHCBGEVRlYHrV/Rr2q9nvN3KsMKtB7gUmF8l s5UIipqR5JSPYQ7QvcejF18Dl5X5COur6/z6+jTX3Clv4RjTns2EnnVSgBpQsbxEefcs hpV3uLT776SmgpizJI9AXRlu1Re7nUqS3zuLqnu2NPA0Eeccw/YxO9Cq1N52jvj43+tf mEAq6PFUlb8jmQVnaJ6FzwWIPKwE5FMXNcQELDy5zJEBJJH4JUrom22nskJYEBMSIU8Q RKZLMiVsId2xAD1yDRDjtwwzcUk40UnAQPeWQUcNdEFQU3WgAaOwWZ+7a4JfiCr5cKFg s/0g== X-Gm-Message-State: ALKqPwcjOljK9ItAT5wVA7qOkTRNhc0wyTcClc91zR7fhrXXymk1H95p 8KNfazU0SkIuPJ2+BEwer9qXrwuUl9TIP6ykuYpdgU/I3G1LSxYZWZtvrsVrSD7OR1ZjtjMKdWn 7W8mO/XcA7w0MCndXlh0AKxiXAwFBP//YmLEwDG8VWgvB1OBF6OE5ZxcB9bF37n15fUGe X-Received: by 2002:a24:a648:: with SMTP id r8-v6mr5100299iti.147.1526074081547; Fri, 11 May 2018 14:28:01 -0700 (PDT) X-Google-Smtp-Source: AB8JxZr3kHS9COaivzDz8Hn3ti4Uyp1DUyfU58fSEVhtQCrBXrSqpZXJMTqFZK3DKwz9spIqWOjZnfIdqqzORFrgOE4= X-Received: by 2002:a24:a648:: with SMTP id r8-v6mr5100276iti.147.1526074081274; Fri, 11 May 2018 14:28:01 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Joao De Almeida Pereira Date: Fri, 11 May 2018 17:27:25 -0400 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Ashesh Vashi Cc: Anthony Emengo , Akshay Joshi , Dave Page , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="00000000000084139c056bf4cf37" X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-05-11_10:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=1 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1805110195 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000084139c056bf4cf37 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 functi= ons >> (setInode, unload, etc). Thus this is the easiest way to introduce our >> abstraction and keep the functionality as before - at least until we dec= ide >> 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 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 nod= es. >> I=E2=80=99m not a fan of singletons myself, but I feel like we=E2=80=99r= e 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? 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 wrote: > On Thu, May 10, 2018 at 8:08 PM, Anthony Emengo > 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 functi= ons >> (setInode, unload, etc). Thus this is the easiest way to introduce our >> abstraction and keep the functionality as before - at least until we dec= ide >> 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 nod= es. >> I=E2=80=99m not a fan of singletons myself, but I feel like we=E2=80=99r= e 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 > >> =E2=80=8B >> >> Sincerely, >> >> Anthony and Victoria >> > > --00000000000084139c056bf4cf37 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello Ashesh,

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

=

As of right no= w, our=C2=A0Tree=C2=A0abs= traction acts as an adapter to the aciTree library. The aciTree library nee= ds the domElement for most of its functions (setInode, unload, etc). Thus t= his is the easiest way to introduce our abstraction and keep the functional= ity as before - at least until we decide that whether we want to switch out= the library or not.
I understand that. But - I've n= ot seen any reference of domElement 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 expecti= ng the tree class to be a singleton class

<= p style=3D"margin:0px 0px 1.2em">

Since this tree is referenced throughout the codebase, consider= ing it to be a singleton seems like the most appropriate pattern for this u= secase. 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 u= p opportunities to improve performance, for example caching lockups of node= s. I=E2=80=99m not a fan of singletons myself, but I feel like we=E2=80=99r= e 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 th= at 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 th= is mean that the patch is going to get committed?=C2=A0

We are looking forward to continue the development on this track of w= ork, but as you know, we are blocked until it gets committed into master. W= e hope that this gets unblocked soon because this is a very big track of wo= rk and the longer it is blocked, the longer it will take to get finalized.<= /div>

Thanks
Joao


On Fri, May 11, 2018 at 2:32 AM As= hesh Vashi <ashesh.vash= i@enterprisedb.com> wrote:
<= div dir=3D"ltr">
On Thu, May 10, 2018 a= t 8:08 PM, Anthony Emengo <aemengo@pivotal.io> wrote:

1. In TreeNode, we're keepging the refere= nce of DOMElement, do we really need it?

As of right now, our Tree abstraction acts as an adapter to the aciTree lib= rary. The aciTree library needs the domElement for most of its functions (s= etInode, unload, etc). Thus this is the easiest way to introduce our abstra= ction 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 domElem= ent the code yet, hence - pointed that out.=C2=A0

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

=

Since this tree is referenced t= hroughout the codebase, considering it to be a singleton seems like the mos= t 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 th= e codebase. Moreover, it opens up opportunities to improve performance, for= example caching lockups of nodes. I=E2=80=99m not a fan of singletons myse= lf, but I feel like we=E2=80=99re simply keeping the architecture the same = in the instance.

Yeah - I don't see an= y usage of tree object from anywhere.
And, we're already crea= ting new object in browser.js (and, not utitlizing that instance anywhere.)=

-- Thanks, Ashesh
=E2=80=8B

Sincerely,

Anthony and Victoria

--00000000000084139c056bf4cf37--