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 1fICl5-0008IN-P6 for pgadmin-hackers@arkaria.postgresql.org; Mon, 14 May 2018 12:39:20 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fICl4-0008Ui-GU for pgadmin-hackers@arkaria.postgresql.org; Mon, 14 May 2018 12:39:18 +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 1fICl4-0008UX-5s for pgadmin-hackers@lists.postgresql.org; Mon, 14 May 2018 12:39:18 +0000 Received: from mail-ot0-x235.google.com ([2607:f8b0:4003:c0f::235]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fICkv-0002JS-Gs for pgadmin-hackers@postgresql.org; Mon, 14 May 2018 12:39:17 +0000 Received: by mail-ot0-x235.google.com with SMTP id t1-v6so14047148oth.8 for ; Mon, 14 May 2018 05:39:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=8g//izKpx88ZoKRup+dMIRD8ySFQ+WisaZXEYBpSb8k=; b=R0FDScSDNDLHT3VDiQYfXhBGkeMwiu8HBOkpR1RGx6l7BLlMOQsyjJqBtV/kJrCLgB lI21wllXWyPwGRqTsMyZ6bJn8xBQJ2P/sGs8Oit0CV0ZCcGuwe+q7ich3Koh7KohsJf9 WwcWuj3gy9yfJRDNeTXav77jQSDFglQME0ZmT/qPa/47/Ui6bynBZ9ZEJL9wcXuE62CK HOfu2TX6H0f6PD2l4EkmlZrfHFyx9MFjviahe85dlxmMIBiNbpjE+GAbTzd19FADjEun Y+OGnpmBYN/JhP2dW/wldF7K0Jm9PQeB0QOOAnODp5hlXwEy7+tB97IG0XHu8Hx8++YD 8oMA== 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=8g//izKpx88ZoKRup+dMIRD8ySFQ+WisaZXEYBpSb8k=; b=RE+z8/zo8r4QxhTL4MedwsfdeeHO0qMOGHHWlzAsrVzsuIWLLG37c7K43FCQ7FyJP5 Gj0adk4JwmwrtRpE2FwTZtC/6ewoqHiwDq6h/vpPkEUdi9OXc5vLwy6N45hQPXnS+oz2 q4Gi5KDkX+V7J+fYuAU8T6fJj7xkzLjBkt/YXH+JUkNiryywLQP2JEoMDm0p3Af5Feha Dd9VvI28MihN6xMOKWV7xwiFl1CCMilBfoCoUBXXpyej6TV3wrnrXBo9uZlY/gQLTiH1 +2p438mxW3Lw6WTiC4UjDk1kLvNy6xECKQIQjGm9GBfI5LLXkfS2/UpZpmjpEkpKyNIo +aOA== X-Gm-Message-State: ALKqPwfgMtzCYBLljT+9QgQhWirDQ9+O+Husb7wnl1ZcJHFDEvJWvPPz SR4N46IPf75gQ7HBR31N8Fg9AX+7q3SGGYRpl7Yvvg== X-Google-Smtp-Source: AB8JxZqWnnlU2KfGVEuZRRZoxdekbcXVqpM7cOu9MiZmQYcud8CDwmVf7m7DO8BfLW9PNNswLPbxNe+0qP81nLvshBc= X-Received: by 2002:a9d:1b4a:: with SMTP id l68-v6mr7517098otl.224.1526301547079; Mon, 14 May 2018 05:39:07 -0700 (PDT) MIME-Version: 1.0 Received: by 10.138.5.130 with HTTP; Mon, 14 May 2018 05:38:46 -0700 (PDT) In-Reply-To: References: From: Ashesh Vashi Date: Mon, 14 May 2018 18:08:46 +0530 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Dave Page Cc: Joao Pedro De Almeida Pereira , Anthony Emengo , "Akshay Joshi (EDB)" , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="00000000000088be2e056c29c57e" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000088be2e056c29c57e Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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 >>>>> functions (setInode, unload, etc). Thus this is the easiest way to >>>>> introduce our abstraction and keep the functionality as before - at l= east >>>>> 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 >>> =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 us= ecase. >>>>> It is very much the same way how we create a single instance of the a= ciTree >>>>> 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. Th= e >>> 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? > Done. -- Thanks, Ashesh > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > --00000000000088be2e056c29c57e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
<= div style=3D"margin:0pt 0pt 8px">On Mon, May 14, 2018 at 2:59 PM, Dave Page= <dpage@pgadmin.org> wrote:


On Sat, May 12, 2018 at 12:10 AM, Ashesh Vashi <= ashesh.v= ashi@enterprisedb.com> wrote:
On Sat, May 12, 2018, 02:58 Joao De Almeida Pereira <jdealmeidapereira@pivo= tal.io> wrote:
Hello Ashesh,

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

As of right now, our=C2=A0Tr= ee=C2=A0abstraction acts as an adapter to the aciTree l= ibrary. The aciTree library needs the domElement for most of its functions = (setInode, unload, etc). Thus this is the easiest way to introduce our abst= raction and keep the functionality as before - at least until we decide tha= t whether we want to switch out the library or not.
I un= derstand that. But - I've not seen any reference of domElement the code= yet, hence - pointed that out.=C2=A0

If you look at the functi= on: reload, unload you will= see that domNode is used to communicate with the A= CITree

=E2=80=8B

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

<= /p>

Since this tree is ref= erenced throughout the codebase, considering it to be a singleton seems lik= e the most appropriate pattern for this usecase. It is very much the same w= ay how we create a single instance of the aciTree library and use that thro= ughout the codebase. Moreover, it opens up opportunities to improve perform= ance, for example caching lockups of nodes. I=E2=80=99m not a fan of single= tons myself, but I feel like we=E2=80=99re simply keeping the architecture = the same in the instance.
Yeah - I don't see any usa= ge of tree object from anywhere.
And, we're already creating new obj= ect in browser.js (and, not utitlizing that instance anywhere.)

You are right, we do not need to export tree as a sin= gleton 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.

-- Than= ks, Ashesh=C2=A0

--
D= ave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB U= K: http://www.ent= erprisedb.com
The Enterprise PostgreSQL Company

--00000000000088be2e056c29c57e--