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 1fIbVO-0005OE-0R for pgadmin-hackers@arkaria.postgresql.org; Tue, 15 May 2018 15:04:46 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fIbVM-0004DS-SM for pgadmin-hackers@arkaria.postgresql.org; Tue, 15 May 2018 15:04:44 +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 1fIbVM-0004DI-Lo for pgadmin-hackers@lists.postgresql.org; Tue, 15 May 2018 15:04:44 +0000 Received: from mx0b-00296801.pphosted.com ([148.163.153.148]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fIbVG-0004yY-Ah for pgadmin-hackers@postgresql.org; Tue, 15 May 2018 15:04:43 +0000 Received: from pps.filterd (m0114585.ppops.net [127.0.0.1]) by mx0b-00296801.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w4FF0tIF028527 for ; Tue, 15 May 2018 15:04:35 GMT Received: from mail-it0-f71.google.com (mail-it0-f71.google.com [209.85.214.71]) by mx0b-00296801.pphosted.com with ESMTP id 2hwtdrt8dt-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Tue, 15 May 2018 15:04:34 +0000 Received: by mail-it0-f71.google.com with SMTP id n83-v6so1879633itg.2 for ; Tue, 15 May 2018 08:04:33 -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=LEKi4bwgVa9EBLoqmWF8wivipFxy8Yc+Bt44rl1llJI=; b=ODOiR2KWCK7NaifxuOAHUsHs5/swTQ5DyimWivZYw8LlqM53ILhwfDoDkaWm+Snd+Z 1FlzFO1Q09qYdU4BBNu7NE3LS1i++zOHLed0pNmOEbv7zoStEFOgoOep8sCaJZuJY22R ZGO7cgmKJW6AJgWPR1/jEcwbFtc5mbU8662+yVEq736pePs2IIB6JcdD6ZRf4bhDTEF3 CquzXCgRXxLDSxcYxJrqCVi+j7ybcVvMbWCWcdagcj9tU0T+6nRDS4Q/sE0DPcmfwCqn qIbzyft2oNmZZJY3Hhp9vFCT+T8YofdnSOnFk7dRZu1VlYyAIbZZl6c98fCOX0KHrA9E l7lw== X-Gm-Message-State: ALKqPwc6r6/QjNHBH/PnHi4wRFaCs2bPPEHQme0Ki5ARkdXqMd80rUb4 4nAiS2HITHkWbuJtubD4gBhf9mr2Uf8MsOnCujnNHBlevfXEKwSukZAzsngxH//YicW4zWFPebm Tj0Exim9Ai0dgJa2vm0dpb9lQ1zGbXG8B77OOqqY3XssEt88en3s+rrwXpjOc+E5Aqm9B X-Received: by 2002:a6b:ab82:: with SMTP id u124-v6mr14983676ioe.234.1526396673181; Tue, 15 May 2018 08:04:33 -0700 (PDT) X-Google-Smtp-Source: AB8JxZoolHBu/74o4hzDyc5qdf8thIC49FGl7TJMyXWpm7bdq9g2qgT2E6y0/mXfhKnKzE/U/tLIOAPg7kd+sdQ2Dag= X-Received: by 2002:a6b:ab82:: with SMTP id u124-v6mr14983650ioe.234.1526396672846; Tue, 15 May 2018 08:04:32 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Joao De Almeida Pereira Date: Tue, 15 May 2018 11:03:56 -0400 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Ashesh Vashi Cc: Anthony Emengo , Dave Page , Akshay Joshi , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="00000000000078f4dd056c3feb5d" X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-05-15_04:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=2 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 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-1805150153 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000078f4dd056c3feb5d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hello Ashesh, These are our comments to the patch: - Can you explain the reasoning behind the change you did on the canCreate function? Bind creates a new instance of a function, do we really need that? - isValidTreeNodeData 1. If it is not Null or Undefined it really means that the data is Valid? Shouldn=E2=80=99t it be isDataDefined? 2. This looks like a generic function that could be used with objects of any type not specific to TreeNodeData. So the file location doesn=E2=80=99t look correct. - The tree folder is just a Tree that we use to store information. The menu uses a Tree but the 2 things should be separated. In our point of view the current entanglement of the ACITree into our code came from missing concepts into a single place (Menu + Storage of information). The idea behind having the Tree as a separate block was to ensure that we do not have the Menu and Tree coupling. - supportedNodesMenu.enabled what it does it check if a Node Menu should be enabled or not. The name of it maybe should be nodeMenu.enabled? =E2=80=8B Thanks Victoria & Joao On Tue, May 15, 2018 at 6:37 AM Ashesh Vashi wrote: > Hi Joao, > On Mon, May 14, 2018 at 6:11 PM, Ashesh Vashi < > ashesh.vashi@enterprisedb.com> wrote: > >> >> 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 mos= t of its >>>>>>>>> functions (setInode, unload, etc). Thus this is the easiest way t= o >>>>>>>>> introduce our abstraction 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 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 inst= ance of >>>>>>>>> the aciTree library and use that throughout the codebase. Moreove= r, 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 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! :-) >> > Majority part of the 0003 patch looks good to me. > Except choice of the path of some of the file, and name of the functions. > > Please find the updated patch. > I've moved files under the 'pgadmin/static/js/menu' directory under the > 'pgadmin/static/js/tree', as they're using tree functionalities directly. > > Please review it, and let me know your concern. > > -- Thanks, Ashesh > >> >> -- Thanks, Ashesh >> >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >> > --00000000000078f4dd056c3feb5d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello Ashesh,

These are our comments to the patch:<= /div>

  • Can you explain the reasoning behind the change = you did on the canCreate function?
    Bind creates a new instance of a fun= ction, do we really need that?

  • isValidTreeNodeData

    1. If it is not Null or Undefined it really mea= ns that the data is Valid? Shouldn=E2=80=99t it be isDataD= efined?
    2. This looks like a generic function that coul= d be used with objects of any type not specific to TreeNodeData. So the fil= e location doesn=E2=80=99t look correct.

  • The tree folder is just a Tree that we use to st= ore information. The menu uses a Tree but the 2 things should be separated.=
    In our point of view the current entanglement of the ACITree into our c= ode came from missing concepts into a single place (Menu + Storage of infor= mation).
    The idea behind having the Tree as a separate block was to ensu= re that we do not have the Menu and Tree coupling.

  • supportedNodesMenu.enabled what it does it check if a Node Menu should be enabled or not. The name = of it maybe should be nodeMenu.enabled?

=E2=80=8B
<= div>

Thanks
Victoria & Joao

On Tue, May 15, 2018= at 6:37 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
Hi J= oao,
On Mon, May 14= , 2018 at 6:11 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com= > wrote:
<= div class=3D"gmail_extra">

On Mon, May 14, 2018 at 6:10 PM, Dave Page= <dpage@pgadmin.org> wrote:


On Mon, May 14, 2018 at 1:38 PM, Ashesh Vashi <ash= esh.vashi@enterprisedb.com> wrote:
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.vashi@enterprisedb.com> wrote:
=
On Sat, May 12, 2018, 02:58 Joao De Almeida Pe= reira <jdealmeidapereira@pivotal.io> wrote:
Hello Ashesh,

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

As of r= ight now, our=C2=A0Tree=C2=A0abstraction acts as an adapter to the aciTree library. The aciTree libr= ary needs the domElement for most of its functions (setInode, unload, etc).= Thus this is the easiest way to introduce our abstraction and keep the fun= ctionality as before - at least until we decide that whether we want to swi= tch out the library or not.
I understand that. But - I&#= 39;ve not seen any reference of domElement the code yet, hence - pointed th= at 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
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
=

2.=C2=A0Are you expecting t= he 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 useca= se. It is very much the same way how we create a single instance of the aci= Tree library and use that throughout the codebase. Moreover, it opens up op= portunities 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 si= mply keeping the architecture the same in the instance.
<= /div>
= Yeah - I don't see any usage of tree object from anywhere.
And, we&#= 39;re already creating new object in browser.js (and, not utitlizing that i= nstance anywhere.)

You are right= , we do not need to export tree as a singleton for now. The line that expor= ts the variable tree can be remove when applying th= e 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 to patch 0003 then :-)=C2=A0
Yes - already working on it! :-)
Majority part of the 0003 patch looks good to me.
Except c= hoice of the path of some of the file, and name of the functions.

Please find the updated patch.
I've moved fil= es under the 'pgadmin/static/js/menu' directory under the 'pgad= min/static/js/tree', as they're=C2=A0using tree functionalities dir= ectly.

Please review it, and let me know your conc= ern.

-- Thanks, Ashesh

-- Thanks, Ashesh=C2=A0

=
--
Dave Pa= ge
Blog: http:= //pgsnake.blogspot.com
Twitter: @pgsnake

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


--00000000000078f4dd056c3feb5d--