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 1fHQGi-0001rL-T9 for pgadmin-hackers@arkaria.postgresql.org; Sat, 12 May 2018 08:52:45 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fHQGd-0002HS-PF for pgadmin-hackers@arkaria.postgresql.org; Sat, 12 May 2018 08:52:39 +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 1fHQGd-0002HH-8b for pgadmin-hackers@lists.postgresql.org; Sat, 12 May 2018 08:52:39 +0000 Received: from mail-oi0-x229.google.com ([2607:f8b0:4003:c06::229]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fHQGV-0007qj-G3 for pgadmin-hackers@postgresql.org; Sat, 12 May 2018 08:52:37 +0000 Received: by mail-oi0-x229.google.com with SMTP id k17-v6so6733983oih.5 for ; Sat, 12 May 2018 01:52:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=RJy4+RUUkIHb07v0GNktMEHb/FFeN3fwya6AaTNnNmo=; b=MgFUCx6g4dEPChxONqNAwBvxvxppRW25zTZId5ypAVfchHw3UE8ENpVu5V93mwc+LB BNyZZUjdfV79bcN02nE9ViKtpkWOJqULLBt6oBAMgSTvHCsJhn5BrJlmWS1PtCD0om1s FJb/Dx9AVHImI/K1FUKEFTD9DRod0J3WCZHjp52KckYyv1m24/8uZJiaHf0ixDMu7flN xeSJpLtGh3i99EjnWWXGddpVcuwTUN5g3pvaYJj5IeTb5RRP/rl7OaUK5OegdiGcImLg r0l9ljseaNCUxl+Ai8fGINbd/D0VokkeacnoFBZtrEB42/+0eUicDwxjsCDzsRRDrDEY /wsw== 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=RJy4+RUUkIHb07v0GNktMEHb/FFeN3fwya6AaTNnNmo=; b=Ga+CtTMBkEjZa7fC39Y87/Y60EitG9quX1WZzhrVjLilyPhjtGtlp1YvS5eHthb7I+ tVsXDidXVNl+iedGlVctZScjO0hBgqYhU8A5eJLMMp6fy3C49jZ7809DCwk5NLhuJeBz 3S7XUrEXjsrMauJx2b8OmVHn1PAsDASMY10/kf4vRTTOJr/1IQHfPrLb6j9suNf0r26m gE2yIGlPuOvYY/omufFbEsIzFbVEhInPO9xgTXiAahGgRmDG2OPI1LWytA4mVEyADT92 xBuEDN14ifxc7jG0uWFyhU/N6fur/0QuIIDiyDVxDcImf7KaZM87sexKP0Ul/WRgfx4H KC/w== X-Gm-Message-State: ALKqPwdzyF0zQI9Vb9g9EgooqHzTXxupIK+2n44GdRAFHzzyXL0CAkQd MVew39wsFJD/OLfXFT3z0ok4JezwfsMqmfIABcPnxw== X-Google-Smtp-Source: AB8JxZq/9DWyuEZyWbQp/pOv22nyOIeXe/iQbsL2Mw1pYuZzxd20w67i2YrWJ2YXO1GisQrQ4GDdZalWkUBDKtlP6T0= X-Received: by 2002:aca:c411:: with SMTP id u17-v6mr1515408oif.244.1526115150500; Sat, 12 May 2018 01:52:30 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Ashesh Vashi Date: Sat, 12 May 2018 04:40:49 +0530 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Joao Pedro De Almeida Pereira Cc: Anthony Emengo , "Akshay Joshi (EDB)" , Dave Page , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="0000000000006eae0b056bfe5f53" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000006eae0b056bfe5f53 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 funct= ions >>> (setInode, unload, etc). Thus this is the easiest way to introduce our >>> abstraction and keep the functionality as before - at least until we de= cide >>> 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 aciTre= e >>> library and use that throughout the codebase. Moreover, it opens up >>> opportunities to improve performance, for example caching lockups of no= des. >>> I=E2=80=99m not a fan of singletons myself, but I feel like we=E2=80=99= 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 utitlizin= g >> 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. -- 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 wo= rk > 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.vashi@enterprisedb.com> 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 reall= y >>>> 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 funct= ions >>> (setInode, unload, etc). Thus this is the easiest way to introduce our >>> abstraction and keep the functionality as before - at least until we de= cide >>> 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 t= o >>> 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 aciTre= e >>> library and use that throughout the codebase. Moreover, it opens up >>> opportunities to improve performance, for example caching lockups of no= des. >>> I=E2=80=99m not a fan of singletons myself, but I feel like we=E2=80=99= 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 utitlizin= g >> that instance anywhere.) >> >> -- Thanks, Ashesh >> >>> =E2=80=8B >>> >>> Sincerely, >>> >>> Anthony and Victoria >>> >> >> --0000000000006eae0b056bfe5f53 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Sat, = May 12, 2018, 02:58 Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">
Hello Ashesh,

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

As of right now, our=C2=A0Tree=C2=A0abstraction acts as an adapter to the aciTree library. The a= ciTree library needs the domElement for most of its functions (setInode, un= load, etc). Thus this is the easiest way to introduce our abstraction and k= eep 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.=C2=A0

If you= look at the function: reload, unl= oad you will see that domNode is used to com= municate with the ACITree

=E2=80=8B

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

S= ince 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 libr= ary and use that throughout the codebase. Moreover, it opens up opportuniti= es to improve performance, for example caching lockups of nodes. I=E2=80=99= m not a fan of singletons myself, but I feel like we=E2=80=99re simply keep= ing the architecture the same in the instance.
Yeah - I = don't see any usage of tree object from anywhere.
And, we're alr= eady creating new object in browser.js (and, not utitlizing that instance a= nywhere.)

= You are right, we do not need to export tree as a singleton for now. The li= ne that exports the variable tree can be remove whe= n 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.

-- 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 w= ork 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 A= shesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
=
On Thu, May 10, 2018 at 8:08 PM, Antho= ny Emengo <aemengo@pivotal.io> wrote:

<= p>

= 1. In TreeNode, we're keepging the reference of DOMElement, do we reall= y 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 expecting 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

--0000000000006eae0b056bfe5f53--