Closed (fixed)
Project:
Taxonomy Term Reference Tree Widget
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
5 May 2011 at 19:29 UTC
Updated:
18 Feb 2020 at 14:27 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
sdrycroft commented+1 for this one. I'm happy to help you work on this if you would like - I was in the process of writing a similar module when I discovered your module.
Comment #2
bartk commentedWelcome aboard! :)
I haven't started on this particular feature, so if it's something you'd like to work on, go for it.
Thanks!
Bart
Comment #3
sdrycroft commentedWow, thanks very much Bart. I'm a little busy this week with meetings, but should hopefully get some work done towards this.
Comment #4
bartk commentedJust wanted to make a note here. I'm currently in the middle of a major project that this isn't strictly necessary for, so I don't have time to work on it. sdrycroft, feel free to poke at it if you'd like. :)
Comment #5
sdrycroft commentedThis is coming on quite well thanks Bart, although I have made a little mess with the current code. I'll report back here when it's working properly (currently having a little issue with setting values the correct "Drupal" way).
Comment #6
bartk commentedGlad to hear it. :)
Just a word of warning -- the code has undergone some significant changes in the beta version -- making in theme-able involved rewriting a lot of code from the ground up.
Anyway, if you'd like another set of eyes, check a new branch into the git repo and I'll take a look at it and maybe make some suggestions.
Bart
Comment #7
sdrycroft commentedI've added a branch to the repo (dynamic_tree_load). I'm afraid I've torn apart a lot of the code, so it may not be obvious which bits I've changed, and which bits are new. I'd certainly appreciate you taking a look at it, and seeing what you think. Note, you will need to uninstall the previous version before installing this (to ensure the menu is rebuilt for the AJAX/AHAH callback).
I was also struggling with themeing the buttons nicely, and integrating them properly into the form, so have rewritten all of the CSS - I'm happy for this to be changed back, especially as I've removed the dotted lines (personally I don't think they're necessary, but if you'd rather have them in, then we can add them back).
Things still to do:
Cheers, Simon.
Comment #8
sdrycroft commentedHave you had a chance to look at this Bart?
Comment #9
bartk commentedNot yet. I'm trying to get the other stuff stable and working first before I merge it in. I was also sick all of last week, which set me back a bit. :)
Comment #10
bartk commentedOkay, time to look at this now. At cursory glance, this looks like it's going to take a lot of work. I'll get back to you if I run into any problems.
Bart
Comment #11
mikgreen commentedAny movement with this? I might be able to contribute work to this effort.
BartK, did you take a look at branch sdrycroft made? Are latest changes to code significate enough and it should be started from scratch?
I don't believe there is another way to handle large taxonomies out there?
Comment #12
bartk commentedYes, I've looked at the branch, and unfortunately the changes were made before I did a major overhaul to the code, and thus the code is very divergent. I can't make heads or tails of the merge. :(
Comment #13
rhylos commentedWhile this is def getting there if the items are not expanded upon expansion anything below does not slide down on the page. The terms tend to overlay content such as the default image upload article option (tested with D7 default theme).
Update (Disregard this is the same as issue: http://drupal.org/node/1277268
)
Comment #14
nags338228 commentedAny movement with this? eagerly waiting for this feature. And in BEF in Views also....
Comment #15
nchar commentedI have worked on this feature, as it was crucial for one of my projects. In this project I had a huge taxonomy tree with hundreds of items and the page was responding too slow both in the frontend and in the backend.
The following patch adds a new setting in every "term reference tree" field to load subtree on click with ajax. I tested with all features of the module and it work fine so far.
Comment #16
bartk commentedI'll keep an eye on this. I don't have the time to test it myself extensively, but if some people try it out and it works without any issues, I'll merge it in and put out a new version.
Comment #17
metammar commented+1 Thanks a lot! works like a charm..
But after applying the patch i got a js error in console because of "on" function. i upgraded my jquery version in admin to 1.10 and error has been gone and all of my problems solved!
Comment #18
nchar commentedYes I missed that. I fixed the patch.
Comment #19
hgoto commentedThank you for the patch!
The patch #18 works well for me. I think it's almost OK but found one point to fix.
If the ajax function is disabled after the patch is applied, an error shows up in my browser console.
The exact line in the js file is the following one.
The steps to reproduce is as following.
I think the error happens because the file ajax.js in the core is not loaded but the line expects that the file is loaded and that the
Drupal.ajaxalready exists there.Comment #20
sano commentedAfter applying this patch the dynamic loading works, but the term labels no longer show in the language set in the user profile (they are always shown in the default language). The i18n is broken regardless of whether ajax loading is turned on or off.
Comment #21
herved commentedHello everyone,
Thank you very much for this patch! Great stuff.
In reply to #20 indeed the labels translation were removed.
I started #2936277: Performance improvements with i18n some time ago to improve the widget process function but found out that the rendering was taking a long time as well on the website I'm working on. So ajax would be the best to improve the render part.
So here is a patch with both ajax and process improvements.
The process function in dev branch is currently very slow because of the taxonomy_term_load() calls used just to get the label.
It also triggers multiple queries to build the tree. The new patch here aims to fix this.
I added some hard caching for the trees mainly to cache the term labels.
If you are on a multilingual website or if you have a specific label callback you'll still have to do this in a custom module:
I also fixed a bug where if term C is selected but terms A and B are not then the widget would not be expanded as it was checking for direct children only:
- A
-- B
--- C
Comment #22
herved commentedSwitching to needs review. Any feedback is greatly appreciated.
Comment #23
sano commentedreally busy at the moment, but looking forward to check your solution asap.
Comment #24
sano commentedLooking good. The i18n problem is fixed with a custom module utilizing your code. This is what I found:
1. editing a node with TTRT widget does not show the previously-made selection. In other words, the tree is collapsed, with no indication as to which branch/leaf is selected. Not sure if this is possible with the ajaxified version, but if I remember correctly I was able to do it using the older patch
2. default value can't be set - clicking the tree root does not expand into branches.
thank you
Comment #25
herved commentedHi sano,
You're right. These render arrays are so confusing...
I'm working on a new patch
Comment #26
herved commentedComment #27
herved commentedHere is an updated patch which should solve both issues.
Any feedback is greatly appreciated ;)
Comment #28
herved commentedComment #29
sano commentedI see the issue #1 is fixed, but see no change in #2. I still can't set a default value. If too much trouble, the ajax version of the widget could be turned off there and replaced with - say - a multi-select text field.
thank you
Comment #30
herved commentedOk bear with me :)
There was a small issue in the ajax callback - term_reference_tree_children_callback() - and issues with clearing the hard caching.
Here is I think my final take on this.
I removed the hard caching as the perf gain was not that high and I don't think that would ever be merged to develop as it prevents the dynamic loading on translations.
Here are the changes I did against #18:
1. Fixed can't set default value - issue in term_reference_tree_children_callback()
2. Refactored the _term_reference_tree_get_term_hierarchy() function. Retrieves the term hierarchy in 1 query only. Uses static caching (useful if several widgets use the same taxonomy and possibly different parents)
3. Moved the max_depth logic in the process function
4. Restored the the label resolution for multilingual websites and moved it in the process function + possibility to override it with a variable (useful for performance in order not to load the whole entity).
To use it with e.g. i18n: drush vset term_reference_tree_taxonomy_resolve_label_callback "mymodule_term_reference_tree_taxonomy_resolve_label_callback"
Possible improvements:
- The label resolution is heavy for multilingual websites as it loads full entities one by one then calls entity_label().
To improve this we could possibly flatten the array returned by _term_reference_tree_get_term_hierarchy(), pass terms in a drupal_alter() and then if some property is not set, resolve all labels (or tokens if used) using taxonomy_term_load_multiple() and entity_label().
Comment #31
herved commentedI eventually implemented my suggestion for possible improvements.
Final-final patch containing big changes is here.
But any suggestion is of course welcome.
Changes against #30:
- Improvements to the label resolution for multilingual websites and/or if using tokens.
- Added alter hook: hook_term_reference_tree_process_terms_alter().
If you are on a multilingual website labels will be resolved using taxonomy_term_load() and entity_label() which is the most reliable.
As you know loading full terms is rather heavy.
If using i18n or ET and title module, and to avoid loading terms we can now use the alter hook I added like this (No variable set, and possibility to load all terms at once):
Comment #32
herved commentedAAAh I messed up the info file...
New patch here.
Comment #33
herved commentedI didn't realize but we need to keep the logic about max_depth, use_ajax and expanded in the process function as well as in the _term_reference_tree_build_level() and _term_reference_tree_build_item() functions...
This is really confusing.
Probably the best would be to build the render arrays as part of _term_reference_tree_get_term_hierarchy()...
Anyway I hope this patch has no issues. I think so. We'll see.
Sorry for the mess here...
Comment #34
sano commentedLooking good. I tested it with English and Slovak - two languages my site is using atm. Setting default value works, selecting multiple values works, creating a new term/translating it and subsequently selecting works as well.
Great job. :)
Thank you
Comment #35
herved commentedGreat to hear sano ;)
On my side I can definitely see a performance improvement between patch #18 (/!\ with restored label translation) and #33.
This is even more noticeable if I use several fields using the same taxonomy but different parents.
However I'm still not 100% happy with the whole process function and I think it could be improved even more.
If I still find some time I might provide a new patch.
But if anyone wants to dive in or simply review what I did so far here's my logic:
1. Retrieve the whole tree (in 1 query, terms and hierarchy cached statically)
2. Filter terms to get only the ones about to be rendered (based on filter_view, max_depth, default, use_ajax, opened,...)
3. Provide an alter hook on the terms - provide all terms at once for perf (useful for example to resolve labels faster than using taxonomy_term_load() + entity_label(), maybe unset some terms, etc).
4. Resolve tokens and labels (if not resolved already).
5. Finally build the render arrays.
Comment #36
herved commentedMinor fixes here:
1. Fix issue reported in #19 by hgoto
2. Fix throbber-active.gif path for websites with sub-directory url
3. Some functions doc fixes
Comment #37
herved commentedNew patch that disables the row when we click on a "+" while ajax is loading.
Thanks to my colleague @pgingao ;)
Comment #38
Delphine Lepers commentedComment #39
sano commentedinstalled patch #37. I see a speed improvement from #33. No issues to report. Thank you
Comment #40
troybthompson commented#37 has made a huge difference in loading time for me using a large taxonomy for three fields on the same edit page. Thank you for this patch.
Comment #41
herved commentedGlad to hear ;)
If you are on a multilingual website (locale module installed) I highly suggest to use the snippet I posted in #31 which uses i18n and avoids taxonomy_term_load() calls.
If you use entity_translation something like this should work (untested):
Comment #42
kyuubi commentedHi guys,
What's the status of this patch? This one is critical for us (we have thousands of terms) and it's working great but it no longer applies to the most recent version of the module.
Any chance to get this committed soon?
Cheers,
Comment #43
sano commentedI am running the the patch on the 7.x-1.11 version and see no problems. What version do you have installed?
Comment #44
kyuubi commentedYou're right, don't know what happened there.
+1 for me, let's get this committed!
Cheers
Comment #45
yogeshmpawarLooks good to me, +1 for RTBC.
Comment #46
alex_optimNeed to upgrade the patch for version 7.x-1.x-dev
Comment #47
eduardo morales albertiRerroll patch to apply with the last version.
Not posible to do the interdiff.
Comment #49
alex_optimComment #50
alex_optimComment #51
alex_optimComment #52
alex_optimComment #53
arefen commentedHi. I check D8 version of this module and see doesn't this feature on it. Are you any plan to support in D8?