Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree - Mailing list pgadmin-hackers
| From | Joao De Almeida Pereira |
|---|---|
| Subject | Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree |
| Date | |
| Msg-id | CAE+jjakrT2AhM2gEGt+QKEANMypn08O4vweOgwhyqQdV6mZEEQ@mail.gmail.com Whole thread Raw |
| In response to | Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree (Ashesh Vashi <ashesh.vashi@enterprisedb.com>) |
| Responses |
Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree
|
| List | pgadmin-hackers |
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- If it is not Null or Undefined it really means that the data is Valid? Shouldn’t it be
isDataDefined? - 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.
- If it is not Null or Undefined it really means that the data is Valid? Shouldn’t it be
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.enabledwhat it does it check if a Node Menu should be enabled or not. The name of it maybe should benodeMenu.enabled?
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 <dpage@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: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, ourTreeabstraction 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,unloadyou will see thatdomNodeis used to communicate with the ACITree2. Are you expecting the tree class to be a singleton classSince 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
treecan 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
pgadmin-hackers by date: