Truncated menu #1572

Closed
wants to merge 3 commits into
from

Projects

None yet

4 participants

@evildmp
Contributor
evildmp commented Jan 4, 2013

Code and docs for a new option on menu templatetags: truncate.

It's intended for {% show_menu %}, and a better alternative to #1284. This way won't affect anyone who is using the menus as they, but allows users with large sites to speed menus up dramatically by adding the truncate argument to the template tag.

Basically, instead of building a menu out of every node for CMS pages, it's more selective, and eliminates ones that we won't be needing in the menu. This means that the expensive looping over nodes in the menu system (that could take 8 seconds on our site) is done over a much smaller set, fast enough to make cacheing unnecessary.

It won't be appropriate for every menu structure, but that's OK, because the default behaviour is unchanged.

I will follow this with tests, if this approach looks acceptable, but a lot need to be rewritten for this so I will wait for some feedback before tackling that.

@ojii
Collaborator
ojii commented Jan 7, 2013

breaks backwards compatibility of a very public api, so a -1 from me.

@evildmp
Contributor
evildmp commented Jan 7, 2013

Does https://github.com/divio/django-cms/pull/1572/files#L2R87 not help address this sufficiently?

@adaptivelogic adaptivelogic commented on an outdated diff Jan 10, 2013
menus/menu_pool.py
@@ -131,7 +139,7 @@ def _build_nodes(self, request, site_id):
final_nodes = []
for menu_class_name in self.menus:
- nodes = self.menus[menu_class_name].get_nodes(request)
+ nodes = self.menus[menu_class_name].get_nodes(request, truncate)
@adaptivelogic
adaptivelogic Jan 10, 2013 Contributor

To keep compatibility with existing menu classes, why not wrap in a try clause, assuming the code that follows can cope with more nodes than necessary when truncate is set?

@adaptivelogic
Contributor

I'll note that #1558 also speeds up menu building by skipping over or speeding up the loading of view restrictions.

@ojii
Collaborator
ojii commented Jan 10, 2013

Alternatively we could use inspect to figure it out when we register the
class.
On Jan 10, 2013 7:53 PM, "Bjorn Sandberg" notifications@github.com wrote:

I'll note that #1558 https://github.com/divio/django-cms/issues/1558also speeds up menu building by skipping over or speeding up the loading of
view restrictions.


Reply to this email directly or view it on GitHubhttps://github.com/divio/django-cms/pull/1572#issuecomment-12089663.

@evildmp
Contributor
evildmp commented Jan 10, 2013

The try clause is something I already thought about, along with a deprecation warning if it fails, so people can be alerted to the API change and be warned in advance.

@evildmp
Contributor
evildmp commented Jan 10, 2013

Looks like this should be perfectly compatible with #1558.

@digi604
Member
digi604 commented Mar 26, 2013

testsuite fails 😦

@digi604
Member
digi604 commented Feb 10, 2014

closing as outdated

@digi604 digi604 closed this Feb 10, 2014
@evildmp evildmp deleted the evildmp:truncated-menu branch Jul 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment