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 1fICm8-0008RP-RX for pgadmin-hackers@arkaria.postgresql.org; Mon, 14 May 2018 12:40:25 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fICm7-0003VW-Fy for pgadmin-hackers@arkaria.postgresql.org; Mon, 14 May 2018 12:40:23 +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 1fICm7-0003VI-36 for pgadmin-hackers@lists.postgresql.org; Mon, 14 May 2018 12:40:23 +0000 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fICm3-00034b-74 for pgadmin-hackers@postgresql.org; Mon, 14 May 2018 12:40:21 +0000 Received: by mail-wm0-x231.google.com with SMTP id t11-v6so13220781wmt.0 for ; Mon, 14 May 2018 05:40:18 -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=abyhO6JW27F0o1OH5l/G8H1UxAn4xzFr6k/ocSSbG4M=; b=jRtY3t2jLnyNco6WcnMN30TZC4H2O7DBHM8uMxR7QzGih/I+Uvs2OavCyqRHykdrl5 ACCcsBu0jTqaL//43YvZO44buns5i+zCNwqR55S+CxFnz6f6DXlZFhkA27sRRHfJ2YQ1 asDRZe2y3CgEuSRnl6NvcyF8CnAsHoZOQswG3Bsfvd3gi7uPRR5DqRl2BtRUqBtLr/GX gk/uwnUKm6jgHvvKUGJSJHERQCABUdL9gNsn0DMgQub01Ir3jzgl3pLnbv3VW5naxf41 duYgEKcPr458CQBf/1bA22OJYX7PQUjwReyF9zolmTiBtNHBa0t1VK0KQqtsztkJ7wRd C7kA== 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=abyhO6JW27F0o1OH5l/G8H1UxAn4xzFr6k/ocSSbG4M=; b=oHGP81j+1VOnC4GuHiq3eMWVWvdNyBuboq7GnYXKmDbG2IQUXA6EbTqL4uIo7KTJJb wTte/pfFujGy12OhMu25fMCTZwY0SmEhi5wbXsnkfCWs7GL19tSSXyouzlo/Dzui9RVw B4oSJI9wt1Qvg2xre047DYKXeBl+7T0lSVLYSyJqQAszrAajev0/kz2FvAnv0GmcqVq1 13z5HEBH6LSLasrB1G+RvbHHtBwyaHTiQELd9h5Xf6YzFnfIvBpjYNeQPT9zyVULXxE1 0TEwNWwrs9fvPM6wOJxZewFmpUudvJ1Y48HyT0rn5fT18AFkuHao87dpZ9sqxN5xaHNW UEWw== X-Gm-Message-State: ALKqPwcbfVtkMna4IdgzScURu+CNytvUhvvQRitwN8Gia1UemeD5jw95 Y3UffmCf0DiBoeMWeyUxv9gq2KEc5asdrcWe5vzc0A== X-Google-Smtp-Source: AB8JxZres8sJLeUGwJNDHP+6ryQc9izMhaviSYKmNb6zY/oPDVSKNFAMa8J8fJ0fazeqdjojtYN3V8cHXCeLStPOKgA= X-Received: by 2002:a1c:4489:: with SMTP id r131-v6mr5625969wma.140.1526301617424; Mon, 14 May 2018 05:40:17 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.85.194 with HTTP; Mon, 14 May 2018 05:40:16 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Mon, 14 May 2018 13:40:16 +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="000000000000ba2744056c29c9f1" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000ba2744056c29c9f1 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, May 14, 2018 at 1:38 PM, Ashesh Vashi wrote: > On Mon, May 14, 2018 at 2:59 PM, Dave Page wrote: > >> >> >> 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 o= f 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 no= t. >>>>> >>>>> I understand that. But - I've not seen any reference of domElement th= e >>>>> 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 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 up >>>>>> opportunities to improve 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 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 thi= s >>>> 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 :-) --=20 Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --000000000000ba2744056c29c9f1 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Mon, May 14, 2018 at 1:38 PM, Ashesh Vashi <ashesh.vash= i@enterprisedb.com> wrote:
=
On M= on, May 14, 2018 at 2:59 PM, Dave Page <dpage@pgadmin.org> w= rote:


On Sat, May 12, 201= 8 at 12:10 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com= > wrote:
On Sat, May 12, 201= 8, 02:58 Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:
=
Hello Ashesh,
1. In Tree= Node, we're keepging 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 d= omElement for most of its functions (setInode, unload, etc). Thus this is t= he easiest way to introduce our abstraction and keep the functionality as b= efore - at least until we decide that whether we want to switch out the lib= rary or not.
I understand that. But - I've not seen= any reference of domElement the code yet, hence - pointed that out.=C2=A0<= /blockquote>

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 tree is referenced thro= ughout the codebase, considering it to be a singleton seems like the most a= ppropriate pattern for this usecase. It is very much the same way how we cr= eate a single instance of the aciTree library and use that throughout the c= odebase. Moreover, it opens up opportunities to improve performance, for ex= ample 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 architecture the same in = the instance.
Yeah - I don't see any usage of tree o= bject from anywhere.
And, we're already creating new object in brows= er.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 th= is mean that the patch is going to get committed?
Yes - from me for 0002.

Can you do that today?=C2=A0
Done.

Great, thanks!

On t= o patch 0003 then :-)=C2=A0

--
Dave Page
Blog: http://pgsnake.blogsp= ot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
T= he Enterprise PostgreSQL Company
--000000000000ba2744056c29c9f1--