On Tue, 2012-01-24 at 16:07 +0200, Marius Gedminas wrote:
Incidentally, please do not hijack existing threads when you start a new topic, ok?
Yes, that was an honest mistake, no ill intentions. Won't happen again.
On Tue, Jan 24, 2012 at 01:35:49PM +0200, Jan-Carel Brand wrote:
On Fri, 2012-01-20 at 13:50 +0200, Jan-Carel Brand wrote:
Hi all
I've been working on porting the Dynatree (a dynamic tree-like) widget to z3c.form:
https://github.com/collective/collective.dynatree
My (temporary) fork is here:
https://github.com/syslabcom/collective.dynatree
And for this I needed a hierarchical tree-like vocabulary.
So I've created a TreeVocabulary in zope/schema/vocabulary.py, based upon the existing SimpleVocabulary.
Instead of fromValues or fromItems, it has fromDict, to construct the vocab from a dict. And the internal representation, self._terms, is a dictionary.
My branch is here: http://svn.zope.org/zope.schema/branches/jcbrand-treevocabulary/
The only changes are the new TreeVocabulary in zope/schema/vocabulary.py and the tests for it in zope/schema/tests/test_vocabulary.py
Can someone please take a look and give some feedback?
vocabulary.py, line 146:
"""Initialize the vocabulary given a dict of terms.
Please clarify what a 'dict of terms' means. AFAIC the *keys* of your dict are terms (instances of SimpleTerm or whatever), and the values are dicts representing the children.
Ok, I've clarified that a bit more.
vocabulary.py, line 150:
"gne or more interfaces may also be provided so that alternate"
s/gne/One/
Fixed
test_vocabulary.py, line 223:
self.assertTrue(dict, type(v._terms))
s/assertTrue/assertEqual/
Fixed.
test_vocabulary.py, line 249:
""" len returns the number of all nodes in die dict
s/die/the/
Fixed.
lines 255-257:
self.assertTrue('Regions' in self.tree_vocab_2 and \ 'Austria' in self.tree_vocab_2 and \ 'Bavaria' in self.tree_vocab_2)
The backslashes at the end of each line are not necessary. Same on lines 262-264.
Removed.
test_vocabulary, lines 192-194: you create list_vocab and items_vocab and then never use them.
Removed.
Missing tests: by inheriting from SimpleVocabulary you also gain .fromItems() and .fromValues(). Do those work? They pass a list of terms to __init__, which seems to expect a dict now. Override and add a raise NotImplementedError? Or just make them work?
I now subclass PersistentMapping instead of SimpleVocabulary, so this is not an issue anymore.
I think TreeVocabulary should have a corresponding interface ITreeVocabulary.
I agree, done.
The new getTermPath() method is currently undocumented.
I added documentation.
What's the use case for a tree vocabulary? A widget that displays the tree structure explicitly?
Yes. In my case, it's for the widget in collective.dynatree. This is a fairly common use-case in Plone. Products.ATVocabularyManager also has hierarchical vocabularies.
It seems... difficult to extract that tree structure using just the public API. Actually, it's impossible: __iter__ doesn't return all the terms, just top-level ones. Am I missing something?
I've changed the TreeVocabulary to subclass from PersistentDict. So the vocabulary itself now acts as a dict.
I'm unhappy about len(tree_vocab) !+ len(list(tree_vocab)).
You're right. I fixed that.
Perhaps I should rephrase :)
I would like my changes to be merged with the zope.schema trunk. The tests I've added provide 100% coverage of the TreeVocabulary code.
I would just like someone to sign it off.
-1 because of the concerns above.
Fair enough. Have your concerns been addressed properly? Thanks Marius, and Charlie, for taking the time to check the code. I appreciate it! JC