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 1fIqjv-0003Ed-1G for pgadmin-hackers@arkaria.postgresql.org; Wed, 16 May 2018 07:20:47 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fIqjt-0005s4-HI for pgadmin-hackers@arkaria.postgresql.org; Wed, 16 May 2018 07:20:45 +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 1fIqjt-0005ru-5c for pgadmin-hackers@lists.postgresql.org; Wed, 16 May 2018 07:20:45 +0000 Received: from mail-ot0-x231.google.com ([2607:f8b0:4003:c0f::231]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fIqjh-0002n0-V3 for pgadmin-hackers@postgresql.org; Wed, 16 May 2018 07:20:44 +0000 Received: by mail-ot0-x231.google.com with SMTP id y10-v6so3234986otg.10 for ; Wed, 16 May 2018 00:20:33 -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=K9VviW2DpS/Dh/RkW8J56aGx/3peqxxgprE0GVYgajU=; b=KYmXQlY2yx7C9RUZwbXB7gxg0u1iBj5HaTtxNtDceKbRPH/5INHASM9hBpSGYGz2zg yMxOJOKhdx9YUork+1z0GpS+hKO4gCL+0IxsKstHHpIZwGiLZV4gdC3OazcKZSjX5x5i O+wVELSgRRY/JPC1pUxVH/zRGKSZnUqRg6PamD+8ShuSZHIiWxNvw7cfMr1MF4+dQGy5 4AlptscvYBTreLej8ZAoTtKsqZC7UF/CyWDAEvpByi3Ra35VBR+wNZWwQhlGuDVVQWi4 UAkTR3g82CxrdAGgsLzUBYZhaqIxiLkTGUcQWSGL6S/fnN38BQtL2fF7ixenO65FMJZ/ c0Mg== 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=K9VviW2DpS/Dh/RkW8J56aGx/3peqxxgprE0GVYgajU=; b=dCXNuD7kybF6r2LP9TuXJVZnqshcT30WXOj2K4aNm2PxpR75gCVn0oieUe4A2Z9YZx R7eQzSKhcZee41Juog4xquI2mN1bDN2YZedf5JAPWt8/7UhzBl1E1rBgKGiKwwEqj/Ra 1rFYPEqHgUpDRnve882KmchBw1Nf6RfpEZc0O4aD5Mzv63Z7R4LEp4hhTR3DLGTomq2m BVv2GEh9jvZpLk/3LFO0y4X8ruGacYvM5tC0Dl/V9YvAXEKfKaM6oWjowgjW+9naKw58 P04BHDBr22oshenfWMyHpeO7AHCtLlkMU115rVqkcZYJyHAKXMHDoU3PFltn/Vm7AdBz kcvA== X-Gm-Message-State: ALKqPwdcdKILxeC+LQok7vPMdutrag/MNRkFqE1WCNBFBvTg0r1ZrXmA pWseQJucb4sEezjj/TESaSxpoz0irArQci1hF6OwKw== X-Google-Smtp-Source: AB8JxZpmEMqDKAXY3broMc8ZSTnZqgUEV18EBC5jLBiiLWXJoNG6PD1c3Cutq/v4zcGUzCNVYJvsC3gr6ZWaqWaG1Tw= X-Received: by 2002:a9d:3c3b:: with SMTP id q56-v6mr13837695otc.226.1526455231363; Wed, 16 May 2018 00:20:31 -0700 (PDT) MIME-Version: 1.0 Received: by 10.138.5.130 with HTTP; Wed, 16 May 2018 00:20:10 -0700 (PDT) In-Reply-To: References: From: Ashesh Vashi Date: Wed, 16 May 2018 12:50:10 +0530 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Joao De Almeida Pereira Cc: Anthony Emengo , Dave Page , Akshay Joshi , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="000000000000d4d747056c4d8d36" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000d4d747056c4d8d36 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Joao, On Tue, May 15, 2018 at 8:33 PM, Joao De Almeida Pereira < jdealmeidapereira@pivotal.io> wrote: > Hello Ashesh, > > These are our comments to the patch: > > > - > > Can you explain the reasoning behind the change you did on the > canCreate function? > > I do agree as Akshay mentioned ealier, canCreate.canCreate does not look right to me too, and neither directory structure, nor name of the script does not make sense to me. As those functions are always going to apply on the tree nodes only, moved them under the 'sources/tree' directory, and renamed it as 'node_menu'. > > - > > Bind creates a new instance of a function, do we really need that? > > If you look at the original proposed patch, the implementation looks like this. *canCreate: function(itemData, item, data) {* * return canCreate.canCreate(pgBrowser, , item, data);* *}* Here - we're already creating an anonymous function, which is an object under the hood. And - for every tree node, they will be a separate anonymous function anyway. Why not use better object oriented approach? > > - > > 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 locat= ion > doesn=E2=80=99t look correct. > > I renamed it as isValidTreeNodeData, because - we were using it in for testing the tree data. I do agree - it is a generic function that could be used with objects of any type. But - when I moved the code, the function name was 'isProvidedDataValid(...)', which was present in 'sources/menu/menu_enabled' file, which was definitely not right place. :-) Please suggest me the right place, and name. > > - 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. > > I think - you're missing the point. Here - we're dealing with two types of menus: 1. Contextual menu (which will always depend on the current selected tree node) 2. Normal menus > > - > > 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. > > In my opinion - keeping them in them in different directories/files does not make them decoupled to be honest. The original patch was separating them in different places, but - still uses some of the functionalities directly from the tree, which was happening because we have contextual menu. To give a better solution, I can think of putting the menus related code understand 'sources/tree/menu' directory. That will give clear distiguation between actual tree, and dependent code. What do you say? > > - > > 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? > > nodeMenu.enabled(...) looks to general to me. How about nodeMenu.isSupportedNode(...)? If you look at the implementation of the function, it checks for the current tree node is one of the supported nodes, or not. -- Thanks, Ashesh > =E2=80=8B > > > Thanks > Victoria & Joao > > On Tue, May 15, 2018 at 6:37 AM Ashesh Vashi < > ashesh.vashi@enterprisedb.com> 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 mo= st 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 o= r not. >>>>>>>>> >>>>>>>>> I understand that. But - I've not seen any reference of domElemen= t >>>>>>>>> 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, considerin= g >>>>>>>>>> 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 ins= tance of >>>>>>>>>> the aciTree library and use that throughout the codebase. Moreov= er, it >>>>>>>>>> opens up opportunities to improve performance, for example cachi= ng 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 >>>> >>> >>> >> --000000000000d4d747056c4d8d36 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Joao,

On Tue, May 15, 2018 at 8:33 PM, Joao De Alm= eida Pereira <jdealmeidapereira@pivotal.io> wrote= :

T= hese are our comments to the patch:

  • Can you explain the reasoning behind the change = you did on the canCreate function?

I do agree as Akshay mentioned ealier, canCreate.canCreate does not l= ook right to me too, and neither directory structure, nor name of the scrip= t does not make sense to me.
As those functions are always going to app= ly on the tree nodes only, moved them under the 'sources/tree' dire= ctory, and renamed it as 'node_menu'.=C2=A0

  • Bind cre= ates a new instance of a function, do we really need that?

If you look at the original proposed patch, the implem= entation looks like this.

canCreate: function(itemData, ite= m, data) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0return canCreate.= canCreate(pgBrowser, <collection_node_type>, item, data);
}

Here - we're already creating an = anonymous function, which is an object under the hood.
And - for every = tree node, they will be a separate anonymous function anyway.
Why not use better object oriented approach?

  • 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.
I renamed it as isValidTreeNodeData, because - we were using = it in for testing the tree data.
I do agree - it is a generic function = that could be used with objects of any type.

But - when I move= d the code, the function name was 'isProvidedDataValid(...)', which= was present in 'sources/menu/menu_enabled' file, which was definit= ely not right place. :-)
Please suggest me the right place, and name.
  • The tree folder is just a Tree= that we use to store information. The menu uses a Tree but the 2 things sh= ould be separated.
I think - you= 're missing the point.

Here - we're dealin= g with two types of menus:
1. Contextual menu (which will always = depend on the current selected tree node)
2. Normal menus

    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 s= eparate block was to ensure that we do not have the Menu and Tree coupling.=

In my opinion - keeping them in= them in different directories/files does not make them decoupled to be hon= est.

The original patch was separating them in dif= ferent places, but - still uses some of the functionalities directly from t= he tree, which was happening because we have contextual menu.
To = give a better solution, I can think of putting the menus related code under= stand 'sources/tree/menu' directory.
That will give clear= distiguation between actual tree, and dependent code.

=
What do you say?
=

  • 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?

nodeMenu.enabled(...) looks to general to m= e.=C2=A0
How about nodeMenu.isSupportedNode(...)?

<= div>If you look at the implementation of the function, it checks for the cu= rrent tree node is one of the supported nodes, or not.

=
-- Thanks, Ashesh
=E2=80=8B
<= div>

Thanks
Victoria & Joao

On Tue, May 15, 2018 at 6:37 AM Ashesh Vashi <ashesh.vashi@e= nterprisedb.com> wrote:
On Mon, = May 14, 2018 at 6:11 PM, Ashesh Vashi <ashesh.vashi@enterprise= db.com> wrote:

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 <<= a href=3D"mailto:dpage@pgadmin.org" target=3D"_blank">dpage@pgadmin.org= > wrote:


On Sat, May 12, 2= 018 at 12:10 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
Hello Ashesh,
<= br>
1. In TreeNode, we're keepging the reference of D= OMElement, do we really need it?

As of right now, our=C2=A0Tree=C2=A0abstraction acts as an adapter to the aciTree librar= y. The aciTree library needs the domElement for most of its functions (setI= node, unload, etc). Thus this is the easiest way to introduce our abstracti= on and keep the functionality as before - at least until we decide that whe= ther we want to switch out the library or not.
I underst= and 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, unload you will see that domNode is us= ed to communicate with the ACITree

=E2=80=8B

2.=C2=A0Are you expecting the tree clas= s to be a singleton class

Sinc= e this tree is referenced throughout the codebase, considering it to be a s= ingleton seems like the most appropriate pattern for this usecase. It is ve= ry 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 n= ot 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 alread= y creating new object in browser.js (and, not utitlizing that instance anyw= here.)

You = are right, we do not need to export tree as a singleton for now. The line t= hat exports the variable tree can be remove when ap= plying 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 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 Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



--000000000000d4d747056c4d8d36--