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’t 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’t 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?
Thanks
Victoria & Joao
On Tue, May 15, 2018 at 6:37 AM Ashesh Vashi <ashesh.va...@enterprisedb.com>
wrote:
> Hi Joao,
> On Mon, May 14, 2018 at 6:11 PM, Ashesh Vashi <
> ashesh.va...@enterprisedb.com> wrote:
>
>>
>> On Mon, May 14, 2018 at 6:10 PM, Dave Page <dp...@pgadmin.org> wrote:
>>
>>>
>>>
>>> On Mon, May 14, 2018 at 1:38 PM, Ashesh Vashi <
>>> ashesh.va...@enterprisedb.com> wrote:
>>>
>>>> On Mon, May 14, 2018 at 2:59 PM, Dave Page <dp...@pgadmin.org> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Sat, May 12, 2018 at 12:10 AM, Ashesh Vashi <
>>>>> ashesh.va...@enterprisedb.com> wrote:
>>>>>
>>>>>> On Sat, May 12, 2018, 02:58 Joao De Almeida Pereira <
>>>>>> jdealmeidapere...@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 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
>>>>>>>
>>>>>>>
>>>>>>>> 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 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’m not a fan of singletons myself, but I feel like we’re
>>>>>>>>> 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.
>>>>>>>
>>>>>>>
>>>>>>> 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
>>>
>>
>>
>
