Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 4 of cosmoz-treenode-navigator #1

Merged
merged 28 commits into from
Aug 17, 2017
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
92c303e
Refactor search(), demo added
JaySunSyn Jul 21, 2017
9dca38a
Cleanup & Demo via iron-component-page
JaySunSyn Jul 21, 2017
b19a36f
Refactor to fit cosmoz-treenode-navigator
JaySunSyn Jul 22, 2017
b9fb2ff
added cosmoz-tree-behavior
JaySunSyn Jul 23, 2017
f7d2fdb
added hook for tree class; added notify tree changes
JaySunSyn Jul 23, 2017
dea0b9a
add properties to afterFound Hook
JaySunSyn Jul 23, 2017
760b426
refaktor to make it renderLevel sensitive
JaySunSyn Jul 23, 2017
1c88582
Defaults to ES5
JaySunSyn Jul 23, 2017
cd64e21
fixes of PR comments
JaySunSyn Jul 23, 2017
5a2064e
PR comments (without rename)
JaySunSyn Jul 26, 2017
f8b75df
PR comments (back to old naming)
JaySunSyn Jul 26, 2017
5af285e
Removed & Exposed Methods, fixed issues
JaySunSyn Jul 29, 2017
d8e9ffd
Rename getPath() => getPathNodes()
JaySunSyn Jul 29, 2017
e878ec1
re-added getNodeByProperty() getPathStringByProperty()
JaySunSyn Jul 31, 2017
00bb3de
getChildren() refactor
JaySunSyn Jul 31, 2017
44ca085
Added comments and some (default) properties
JaySunSyn Jul 31, 2017
13f837a
PR comments: minor fixes, added childProperty as class attribute
JaySunSyn Aug 1, 2017
4174ae1
PR comments: search() refactor; findNode() added
JaySunSyn Aug 1, 2017
47dfbf0
Fix getPathNodes(); add hasChildren(); utilise _searchNodes() in getN…
JaySunSyn Aug 1, 2017
16dcf27
added Init options of DefaultTree
JaySunSyn Aug 2, 2017
90122a3
fixed consistent return of search()
JaySunSyn Aug 2, 2017
fa33008
Refactor getPathNodes()
JaySunSyn Aug 3, 2017
e465ec6
'basic' Test added; & adjustments
JaySunSyn Aug 3, 2017
9e5078d
'multiRoot' Test fix
JaySunSyn Aug 3, 2017
3c5318f
'missingAncestor' Test getPathNodes() in Progress
JaySunSyn Aug 3, 2017
23ab530
adjusted getPathNodes()
JaySunSyn Aug 4, 2017
5308aae
adjusted getPathNodes() Tests
JaySunSyn Aug 4, 2017
20a3aad
getPathNodes() => map(); added some tests
JaySunSyn Aug 7, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
bower_components/
node_modules/
debug.log
.DS_Store
315 changes: 217 additions & 98 deletions cosmoz-default-tree.html
Original file line number Diff line number Diff line change
@@ -1,10 +1,23 @@
<link rel="import" href="../polymer/polymer.html">
<link rel="import" href="cosmoz-tree.html">

<!--
Navigator through object with treelike datastructure and default settings.

@demo demo/index.html
-->
<script>
'use strict';

// Needed for iron-component-page.
Polymer({ is: 'cosmoz-default-tree' });
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we actually need to make this a Polymer element.
Did you see a reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason was for the iron-component-page to not crash. (demo & docs)
Maybe there is another way but I didn't explore it yet..

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok well then leave it but maybe add a comment about it, too.


window.Cosmoz = window.Cosmoz || {};

function _objectValues(obj) {
if (!obj) {
return;
}
return Object.keys(obj).map(function (key) {
return obj[key];
});
Expand All @@ -14,143 +27,249 @@
* Cosmoz.DefaultTree
*
* @constructor
*/
Cosmoz.DefaultTree = function (treeData) {
*
* @param {object} treeData (The tree object.)
* @param {object} options (Tree options.)
* @param {string} options.childProperty ["children"] (The name of the property a search should be based on. e.g. "name")
* @param {string} options.propertyName ["name"] (The name of the property a search should be based on. e.g. "name")
* @param {string} options.pathStringSeparator ["/"] (The string the path should get separated with.)
* @param {string} options.pathLocatorSeparator ["."] (The string which separates the path segments of a path locator.)
*/
Cosmoz.DefaultTree = function (treeData, options) {
Cosmoz.Tree.apply(this, arguments);
this._treeData = treeData;
this._roots = _objectValues(treeData);

options = options || {};
this.pathLocatorSeparator = options.pathLocatorSeparator || '.';
this.pathStringSeparator = options.pathStringSeparator || '/';
this.childProperty = options.childProperty || 'children';
this.searchProperty = options.searchProperty || 'name';
};

Cosmoz.DefaultTree.prototype = Object.create(Cosmoz.Tree.prototype);

Cosmoz.DefaultTree.prototype.getNodeByProperty = function (propertyName, propertyValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As in comment, needs to stay.

if (propertyName === undefined || propertyValue === undefined) {
/**
* Searches a (multi root) node and matches nodes based on a property and a value.
* @return {object} - The first found node.
* @param {string} propertyName (The name of the property the match should be based on. e.g. "name")
* @param {string} propertyValue (The value of the property the match should be based on. e.g. "Peter")
* @param {array} nodes [this._roots] (The objects the search should be based on.)
*/
Cosmoz.DefaultTree.prototype.getNodeByProperty = function (propertyValue, propertyName, nodes) {
if (propertyValue === undefined) {
return;
}
// Defaults
nodes = nodes || this._roots;
propertyName = propertyName || this.searchProperty;

if (propertyName === 'pathLocator') {
return this._getNodeByPathLocator(propertyValue);
}
return this.findNode(propertyValue, propertyName, nodes);
};

return this._searchAllRoots(propertyName, propertyValue);
/**
* Searches a (multi root) node and matches nodes based on a property and a value.
* @return {array} - All found nodes.
* @param {string} propertyName [this.searchProperty] (The name of the property the match should be based on. e.g. "name")
* @param {string} propertyValue (The value of the property the match should be based on. e.g. "Peter")
* @param {boolean} exact [true] (If the search should be executed exact or flaw. true wouldn't match "Pet")
* @param {object} nodes [this._treeData] (The nodes the search should be based on.)
*/
Cosmoz.DefaultTree.prototype.searchNodes = function (propertyValue, nodes, exact, propertyName) {
var options = {
propertyName: propertyName || this.searchProperty,
exact: exact !== undefined ? exact : true,
firstHitOnly: false
};
return this._searchNodes(propertyValue, options, nodes);
};

Cosmoz.DefaultTree.prototype.getPath = function (node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think we can drop getPath

if (node === undefined) {
return;
}
return this._getPathByPathLocator(node.pathLocator);
/**
* Searches a (multi root) node and matches nodes based on a property and a value.
* @return {object} - The first found node.
* @param {string} propertyValue (The value of the property the match should be based on. e.g. "Peter")
* @param {string} propertyName [this.searchProperty] (The name of the property the match should be based on. e.g. "name")
* @param {object} nodes [this._treeData] (The nodes the search should be based on.)
*/
Cosmoz.DefaultTree.prototype.findNode = function (propertyValue, propertyName, nodes) {
var options = {
propertyName: propertyName || this.searchProperty,
exact: true,
firstHitOnly: true
};
return this._searchNodes(propertyValue, options, nodes).shift();
};

Cosmoz.DefaultTree.prototype.getPathByProperty = function (propertyName, propertyValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't drop getPathByProperty

Copy link
Contributor

Choose a reason for hiding this comment

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

As in comment, needs to stay.

if (propertyName === undefined || propertyValue === undefined) {
return;
}
/**
* Searches a (multi root) node and matches nodes based on a property and a value.
* @return {array} - The found node(s).
* @param {string} propertyValue (The value of the property the match should be based on. e.g. "Peter")
* @param {object} options (Matching options)
* @param {string} options.propertyName (The name of the property the match should be based on. e.g. "name")
* @param {boolean} options.exact [false] (If the search should be executed exact or fuzzy. true wouldn't match "Pet")
* @param {boolean} options.firstHitOnly [false] (If the search should only return the first found node.)
* @param {object} nodes [this._roots] (The nodes the search should be based on.)
*/
Cosmoz.DefaultTree.prototype._searchNodes = function (propertyValue, options, nodes) {
var results = [];

if (propertyName === 'pathLocator') {
return this._getPathByPathLocator(propertyValue);
}
// Defaults
nodes = nodes || this._roots;

var node = this._searchAllRoots(propertyName, propertyValue);
if (node !== undefined) {
return this._getPathByPathLocator(node.pathLocator);
}
nodes.some(function (node) {
results = results.concat(this.search(node, propertyValue, options));
return options.firstHitOnly && results.length > 0;
}, this);

return results;
};

Cosmoz.DefaultTree.prototype.getChildren = function (node) {
if (node === undefined) {
return;
/**
* Returns the node of a given path.
* @return {object}
* @param {string} pathLocator (The string which describes the path. e.g. "1.2.9")
* @param {object} nodeObj [this._treeData] (The object the search should be based on.)
* @param {string} pathLocatorSeparator [this.pathLocatorSeparator] (The string which separates the path. e.g ".")
*/
Cosmoz.DefaultTree.prototype.getNodeByPathLocator = function (pathLocator, nodeObj, pathLocatorSeparator) {
if (!pathLocator) {
return this._roots;
}
return _objectValues(node.children);

// Defaults
nodeObj = nodeObj || this._treeData;
pathLocatorSeparator = pathLocatorSeparator || this.pathLocatorSeparator;

var pathNodes = this.getPathNodes(pathLocator, nodeObj, pathLocatorSeparator);
return pathNodes && pathNodes.pop();
};

Cosmoz.DefaultTree.prototype.getProperty = function (node, propertyName) {
if (node === undefined || propertyName === undefined) {
return;
/**
* Returns the nodes on a given path.
* A valid path 1.2.3 should return the items [1, 2, 3]
* - path 1.2.3.3 should return [1, 2, 3, undefined]
* - path 0.1.2.3 should return [1, 2, 3]
* - path 0.1.5.3 should return [1, undefined, undefined]
Copy link
Contributor

Choose a reason for hiding this comment

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

one important path would be 0.1.5.2, it seems like the current implementation would return [ 1, undefined, 2 ]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would actually return [1, undefined, undefined] because it has lost the path.

Copy link
Contributor

Choose a reason for hiding this comment

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

What prevents it from finding the path again next iteration?

Copy link
Contributor Author

@JaySunSyn JaySunSyn Aug 7, 2017

Choose a reason for hiding this comment

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

Imagine this tree:

1 - 9 - 2 - 3
  - 8 - 2 - 4
  - 7 - 2 - 5

We are looking for 0.1.5.2, which one is the correct path? Not sure, but I think when it's lost it should be lost? hm..

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, that's what I mean.

But as I understood the implementation, it continues looping on the pathlocator, so while 5 will be undefined, it will try again with 2, and the incorrectly "get back" on the path.
But as your last push proved, I'm missing something in the logic..

* @return {array}
* @param {string} pathLocator (The string which describes the path. e.g. "1.2.9")
* @param {object} nodeObj [this._treeData] (The object the search should be based on.)
* @param {string} pathLocatorSeparator [this.pathLocatorSeparator] (The string which separates the path.)
*/
Cosmoz.DefaultTree.prototype.getPathNodes = function (pathLocator, nodeObj, pathLocatorSeparator) {
if (!pathLocator) {
return this._roots;
}

return node[propertyName];
};
// Defaults
pathLocatorSeparator = pathLocatorSeparator || this.pathLocatorSeparator;

Cosmoz.DefaultTree.prototype._searchAllRoots = function (propertyName, propertyValue) {
var i,
var path = pathLocator.split(pathLocatorSeparator),
pathSegment = nodeObj || this._treeData,
nodes = [],
node;

for (i = 0; i < this._roots.length; i+=1) {
node = this.search(this._roots[i], propertyName, propertyValue);
if (node !== undefined) {
return node;
// Get the nodes on the path
path.forEach(function (nodeKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This implementation looks very nice, I would just like to see it as a map() again now since the array push is done on each iteration anyway. Also we need to make sure it handles the scenario I commented about in the method docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, we should also have an outer loop to see what root gives the most complete node list as well to handle the "missing ancestor multi root" scenario.

Maybe this can be solved by making this internal (_getPathNodes()) and having an external wrapper with something like:

  • Loop over the roots
  • Perform _getPathNodes() on each
  • Sort the results based on number of undefined elements
  • Return the one with the least (hopefullly 0)

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now we perform the check against the object tree. So whichever "root object" has the first matching key, gets taken.

{
    "2": {
        "id": "167d1485-7d4f-4c7d-86cd-a4fb00f31245",
        "name": "Node2",
        "pathLocator": "1.2",
        "children": {}
    },

    "301": {
        "id": "3a7654f1-e3e6-49c7-b6a8-a4fb00f31245",
        "name": "Node301",
        "pathLocator": "1.2.3.301",
        "children": {
            "401": {
                "id": "865065da-f44c-472e-a8df-a4fb00f3124b",
                "name": "Node401",
                "pathLocator": "1.2.3.301.401",
                "children": {
                }

            }
        }
    }
}
  • 1.2.3.301 returns [2, undefined, 301]
  • 1.2 returns [2]
  • 2.301 returns [2, 301]

What should be changed regarding the behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how the last would work. Wouldn't we find the 2 and start traversing? Never finding 301 since it's another root?

Anyway, both these discussions just needs to be supported with tests I guess and I'll rerrad the code 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the tests. But you are right, it only works here because Node2 does not have children.. Oh man..

Copy link
Contributor

@nomego nomego Aug 7, 2017

Choose a reason for hiding this comment

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

Well, I think the approach from #1 (comment) wouldn't change too much and be pretty secure.

(I feel less crazy now though :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually .. does this highlight another bug?

Consider the path 1.2.3.4 and a tree like

1
|- 2
   |- 3
   |- 4

since 3 doesn't have any children, will we actually end up with [ 1, 2, undefined, 4 ] with 4 pointing to 1.2.4 ?

Copy link
Contributor Author

@JaySunSyn JaySunSyn Aug 7, 2017

Choose a reason for hiding this comment

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

Yes, thats I guess actually what you meant with "What prevents it from finding the path again next iteration?" in above discussion. I falsely thought you want me to find the path again.

I'll try your approach with looping over the roots & will break (?) on undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well I think you can stop searching any more roots if you find a result without any undefined parts, otherwise we have to figure out what path has least number of undefined parts.. whatever that will help.. :)

Copy link
Contributor Author

@JaySunSyn JaySunSyn Aug 7, 2017

Choose a reason for hiding this comment

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

Thanks!

having this tree:

1 - 2 - 3

Do we actually need the undefined values at the end of the array or is it fine as below?

1.2.33 => [1, 2] instead of [1, 2, undefined]
0.1.2.3 => [1, 2, 3] stays same
1.2.3.5 => [1, 2, 3] instead of [1, 2, 3, undefined]

Copy link
Contributor

Choose a reason for hiding this comment

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

Well I would say that the right-most parts are the most significant, so we have to inform the caller that we couldn't find them, either by a non-value (null, undefined) or by invalidating the whole path.
So for the first example I think it should be either [1, 2, undefined] or null.

node = pathSegment[nodeKey] !== node ? pathSegment[nodeKey] : undefined;
nodes.push(node);
if (node && this.hasChildren(node)) {
pathSegment = node[this.childProperty];
}
}, this);

// Filter out undefined items of the start
while (nodes.length > 0 && nodes[0] === undefined) {
nodes.shift();
}
};

Cosmoz.DefaultTree.prototype._getNodeByPathLocator = function (pathLocator) {
var node,
path,
i;
return nodes;
},

for (i = 0; i < this._roots.length; i+=1) {
node = this._roots[i];
if (node.pathLocator === pathLocator) {
return node;
}
/**
* Returns a string which describes the path of a node (found by its path locator).
* @return {string} e.g. home/computer/desktop
* @param {string} pathLocator (The string which describes the path. e.g. "1.2.9")
* @param {string} pathProperty (The property of a node on which the path should be build on. e.g "location" with node = {"location": "home", ..})
* @param {string} pathStringSeparator [this.pathStringSeparator] (The string the path should get separated with.)
* @param {string} pathLocatorSeparator [this.pathLocatorSeparator] (The string which separates the path segments of pathLocator.)
*/
Cosmoz.DefaultTree.prototype.getPathString = function (pathLocator, pathProperty, pathStringSeparator, pathLocatorSeparator) {
// Defaults
pathProperty = pathProperty || this.searchProperty;
pathLocatorSeparator = pathLocatorSeparator || this.pathLocatorSeparator;
pathStringSeparator = pathStringSeparator || this.pathStringSeparator;

if (pathLocator.indexOf(node.pathLocator + '.') === 0) {
path = pathLocator.slice(node.pathLocator.length + 1).split('.');
for (i = 0; i < path.length; i+=1) {
if (node.children) {
node = node.children[path[i]];
} else {
node = null;
}
if (node === undefined || node === null) {
break;
}
}

if (node !== undefined && node !== null) {
return node;
}
}
var pathNodes = this.getPathNodes(pathLocator, this._treeData, pathLocatorSeparator);

if (!pathNodes) {
return;
}

return pathNodes.map(function (node) {
return node[pathProperty];
}).join(pathStringSeparator);
};

Cosmoz.DefaultTree.prototype._getPathByPathLocator = function (pathLocator) {
var node,
path,
subPath,
i;
/**
* Returns a string which describes the path of a node (found by a node's property and value).
* @return {string} e.g. home/computer/desktop
* @param {string} propertyValue (The value of the property the match should be based on. e.g. "Peter")
* @param {string} propertyName (The name of the property the match should be based on. e.g. "name")
* @param {string} pathProperty (The property of a node on which the path should be build on. e.g "location" if node = {"location": "home"})
* @param {string} pathStringSeparator [this.pathStringSeparator] (The string the path should get separated with.)
* @param {string} pathLocatorSeparator [this.pathLocatorSeparator] (The string which separates the path. e.g ".")
*/
Cosmoz.DefaultTree.prototype.getPathStringByProperty = function (propertyValue, propertyName, pathProperty, pathStringSeparator, pathLocatorSeparator) {
if (propertyValue === undefined) {
return;
}

for (i = 0; i < this._roots.length; i+=1) {
node = this._roots[i];
path = [node];
// Defaults
pathProperty = pathProperty || this.searchProperty;
propertyName = propertyName || this.searchProperty;
pathLocatorSeparator = pathLocatorSeparator || this.pathLocatorSeparator;
pathStringSeparator = pathStringSeparator || this.pathStringSeparator;

if (node.pathLocator === pathLocator) {
return path;
}
if (propertyName === 'pathLocator') {
return this.getPathString(propertyValue, pathProperty, pathStringSeparator, pathLocatorSeparator);
}

var node = this.getNodeByProperty(propertyValue, propertyName);

if (pathLocator.indexOf(node.pathLocator + '.') === 0) {
subPath = pathLocator.slice(node.pathLocator.length + 1).split('.');
for (i = 0; i < subPath.length; i+=1) {
if (node.children) {
node = node.children[subPath[i]];
} else {
node = null;
}
if (node === undefined || node === null) {
path = null;
break;
} else {
path.push(node);
}
}

if (path !== null) {
return path;
}
}
if (node) {
return this.getPathString(node.pathLocator || node.path, pathProperty);
}
};

/**
* Returns an Object or an Array representing the children of a node.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, multiple return types.
Any code doing this.getChildren(node) should be able to do node.children instead so drop the asList argument and consider it always true (revert to old return statement)

Copy link
Contributor

Choose a reason for hiding this comment

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

Docs needs updating

*/
Cosmoz.DefaultTree.prototype.getChildren = function (node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a function that only returns the children property of the argument seems pretty pointless, then it should be reversed to how it was before this PR - return _objectValues(node.children); as I commented on the last review.
For any cases where we don't need to convert it to an array, we can just use node.children instead of this method.

if (!node) {
return;
}
return _objectValues(node[this.childProperty]);
};

/**
* Returns true if a node has children.
*/
Cosmoz.DefaultTree.prototype.hasChildren = function (node) {
if (!node) {
return false;
}
var children = this.getChildren(node);
return children && children.length > 0;
};

/**
* Returns the property of a Node based on a given property name.
*/
Cosmoz.DefaultTree.prototype.getProperty = function (node, propertyName) {
Copy link
Contributor Author

@JaySunSyn JaySunSyn Jul 30, 2017

Choose a reason for hiding this comment

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

@nomego actually similar discussion as with getChildren. I don't get the purpose... Can we drop it? (or put it in a separate helper behavior)

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, it's pretty weird.
Not sure why we have it and it only seems to be used in cosmoz-treenode.
If we drop it from cosmoz-treenode I think we can drop it here, too.

if (!node || !propertyName) {
return;
}
return node[propertyName];
};

</script>
</script>
Loading