Skip to content
This repository has been archived by the owner on Sep 29, 2023. It is now read-only.

Hierarchy improvements #28

Closed
wants to merge 17 commits into from
Closed

Conversation

phorward
Copy link
Member

This branch introduces the following improvements into the Hierarchy prototype for a better usability.

  • member variable rootKindName that can be used to specify a custom entity kind for root nodes
  • haschildren property to the HierarchySkel to save the information if an item has children - this is very important for later improvements of the Hierarchy-Widget in the Vi
  • default stub for the getAvailableRootNodes() function that runs in all cases and should serve a reference implementation
  • removed and replaced unused variables

Every feature was placed in a separate commit.

This should allow the use of another kindName without an appended "_rootNode" as hierarchy root kind. The former behavior is preserved to stay backward compatible.
…as children or not

This feature is required for later improvements in the Vi widget for displaying the hierarchy, where the availability of children in nodes can directly be visualized before any data is fetched.
…rototype

This stub should serve as a reference implementation to provide a suitable default behavior.
Copy link
Contributor

@tsteinruecken tsteinruecken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there a lot of issues here...

  1. What's the benefit of removing the defacto-standard of kindName+"_rootNode"? There's probably a lot of code depending on that
  2. the default stub of "getAvailableRootNodes() breaks the default of keeping your data private unless explicitly exposed by the developer
  3. Replacing Variable "x" with "_" shadows our global translate function _()
  4. Keeping track of wherever an entity has children or not is probably a good idea, but it's skel.toDB() is the wrong place (f.e. if the last child of a node is deleted, it's parents haschildren flag has to toggle)

@phorward
Copy link
Member Author

Ok your issues are acknowledged. I refer to them seperately below:

  1. The benefit is, that the developer is more flexible in the ways how to define the data model. A real world example: There is an existing data kind already available with a lot of data. A project extension now should be able to connect hierarchys to these existing data entites, so they are used as hierarchy repositories. In the case the rootKindName cannot flexibly be changed, copying the already available data into a new data kind ending with "_rootNode" is necessary. Why? This improvement still keeps the "defacto-standard" you call it and gives the developer the ability to use his data more flexible.

  2. I think, that ViUR should be easy-to-use. Pre-defined functions help to rapidly prototype new data models and later on customize them to meet the requirements of the project - including which data is exposed. What would be a better alternative for this stub? Is it really necessary that I copy and paste any old code from old projects just to get things to run, because this function is called several times but nowhere defined? This yields in an error 500 in case you create a new Hierarchy module without any functionality, and this is awkward.

  3. You are fully right and I already changed this back to "x" again.

  4. What would be, in your opinion, a better place to store this information, than toDB()?
    You are right, that the nodes must keep track on deleted children. Where could this be put, my first idea would be overriding HierarchySkel.delete() - but is this good? The postDeletedHandler() is also the wrong place because information about the parententry is lost.

@tsteinruecken
Copy link
Contributor

  1. Currently, the recommended path is to leave these existing data alone and create a RootNode in the desired module for each of them, referencing it. This also works if there have to be two or more RootNodes for each entry (like files and pages). By clever utilizing the key structure (db.Key.from_path("xxx_rootNode", name=str(entry.key())) it's possible to do so with with zero database call overhead.

  2. Fine, but you can't sacrifice security/safety for it. In my view, if the developer makes a mistake (which it is not to define that function), an server error is better than to leak data. BTW: normally, the rootNodes returned are created by that function. So unless your'e doing the trick from 1) there won't be any. Which would also be a safe default; we could add that function returning always an empty list by default.

  3. Okay :)

  4. I think, the prototype has to keep track of these and update the affected trees on add/delete/reparent calls

@phorward
Copy link
Member Author

phorward commented May 2, 2018

Tobi, I recently did the following changes and request another merge of this pull request, so that not all work I spent into it is lost.

The last commit 37f5aa8 of branch feature/hierarchyImprovements now contains the following changes I request to merge:

  • Dynamic rootKindName variable as described above
  • Default stub for getAvailableRootNodes() now just returning an empty list and providing an example in the documentation
  • Some code refactoring so that unused variables are not longer assigned
  • Entirely removed previously provided haschildren property

Can you please add this now, to close this case? Thanks.

@tsteinruecken
Copy link
Contributor

Replaced by
#113
#114
#115

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants