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 1fICnt-00005u-Pu for pgadmin-hackers@arkaria.postgresql.org; Mon, 14 May 2018 12:42:14 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fICns-0003xZ-FX for pgadmin-hackers@arkaria.postgresql.org; Mon, 14 May 2018 12:42:12 +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 1fICns-0003xP-9A for pgadmin-hackers@lists.postgresql.org; Mon, 14 May 2018 12:42:12 +0000 Received: from mail-oi0-x22f.google.com ([2607:f8b0:4003:c06::22f]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fICno-0002PS-Dh for pgadmin-hackers@postgresql.org; Mon, 14 May 2018 12:42:11 +0000 Received: by mail-oi0-x22f.google.com with SMTP id n65-v6so10503900oig.6 for ; Mon, 14 May 2018 05:42:07 -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=oZBFg/LYGbBOpp3Tsx3E8syO49aLO+s65pasgkY3pm8=; b=xcGxOfLPl5aVn5gs0DmMi+N+XRjwRVseVoUivIQcYmUWDavhf3QPACkOyMhv4bpw1U f7JFSbg5nwCaBbCBM8AWO0Gyi4IaZUxioo7GqsExjprKYnE1T2xweJ1AFKy5B9m+LLp4 T8n5yqpAnB2/nik9216ijtYBnJbrki+Q1XHGnKEACmjYDQcEDq3+ISdgCbU2cVwqPQk1 Jd5bAlW2GPJB68g6CeMI3BuvdQNIU5cshmp44n41nJiTfzHbj3poAtl+ksL0eaxNlCtE fLmua19z65JQi9YZySy6JKsROAisRZzneR5sVg8ZJjmzX8KyYuuet0rufzJgRe0LM2vW rYmQ== 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=oZBFg/LYGbBOpp3Tsx3E8syO49aLO+s65pasgkY3pm8=; b=MLwSPg2t74MAel4XmjMgEmdeB2Elk7DXKxhxa56CA8LJWPCsKkDQaJeswyOsua7NRb htBNFbfU8y99kVt8/hI0HnAWXeSD5eF2nong+D1bKMYHWNLW9sZQEH50r+q8IN05yZtM NCm5TSeqnLKzoW/X24mxhskFhNFOhwWW5O8odeHDymTaPgNzX0GPOqT3RBeKVp+5PTb2 0JEDpj+VWe4gczCMoUjFZHJZNCMDatKoiWiaYdkkM3zWgP6TE0Dw0VraFigmrbj+c4Op SJ+dn8Ex/I41fb8TF4u+OZH7plSODCwh3QhNQOXfBoebArFjLh7a1scD897usyzWDSEd SYeQ== X-Gm-Message-State: ALKqPwdQN7cWWouffxiBTIPfIQx2s10puyRvH6Stv4BBTroqYsFwBXcc Ko9mdFhdgpw1p9t01KNIa4KAlDAgAZysIlQU4LS4Kw== X-Google-Smtp-Source: AB8JxZqXjbFpoCcVU9gmNfjbma2jDNuOfw2QKsNHKIDb46NqizcBHh8tF3PUUjvbF7p2Y6wIMbqrrJYDXHB+yD8V3Ds= X-Received: by 2002:aca:bac2:: with SMTP id k185-v6mr6759334oif.269.1526301725424; Mon, 14 May 2018 05:42:05 -0700 (PDT) MIME-Version: 1.0 Received: by 10.138.5.130 with HTTP; Mon, 14 May 2018 05:41:44 -0700 (PDT) In-Reply-To: References: From: Ashesh Vashi Date: Mon, 14 May 2018 18:11:44 +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="0000000000002a192a056c29d066" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000002a192a056c29d066 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, May 14, 2018 at 6:10 PM, Dave Page wrote: > > > On Mon, May 14, 2018 at 1:38 PM, Ashesh Vashi < > ashesh.vashi@enterprisedb.com> 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 = of 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 n= ot. >>>>>> >>>>>> 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 i= t >>>>>>> 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 o= f 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 >>>>> this 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 :-) > Yes - already working on it! :-) -- Thanks, Ashesh > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > --0000000000002a192a056c29d066 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 6:10 PM, Dave Page <dpa= ge@pgadmin.org> wrote:


= On Mon, May 14, 2018 at 1:38 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
On Mon, May 14, 2018 at 2:59 PM, Dave Page <dpage@= pgadmin.org> wrote:
<= div class=3D"gmail_quote">


On= Sat, May 12, 2018 at 12:10 AM, Ashesh Vashi <ashesh.vashi@en= terprisedb.com> wrote:

=
2.=C2=A0

Since this tree is referenced throughout the c= odebase, considering it to be a singleton seems like the most appropriate p= attern for this usecase. It is very much the same way how we create a singl= e instance of the aciTree library and use that throughout the codebase. Mor= eover, it opens up opportunities to improve performance, for example cachin= g 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 a= nywhere.
And, we're already creating new object in browser.js (and, = not utitlizing that instance anywhere.)

You are right, we do not need to ex= port 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?



On to patch 0003 then :-)=C2=A0Yes - already working on it! :-)

-- Thanks, Ashes= h=C2=A0

--
Dave PageBlog: http://pg= snake.blogspot.com
Twitter: @pgsnake

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

--0000000000002a192a056c29d066--