Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
41 changes: 41 additions & 0 deletions docs/source/settings_reference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -392,3 +392,44 @@ Use this to specify the 'depth' value of a project's 'section root' pages. For m
Default value: ``False``

By default, menu items linking to custom URLs are attributed with the 'active' class only if their ``link_url`` value matches the path of the current request _exactly_. Setting this to `True` in your project's settings will enable a smarter approach to active class attribution for custom URLs, where only the 'path' part of the ``link_url`` value is used to determine what active class should be used. The new approach will also attribute the 'ancestor' class to menu items if the ``link_url`` looks like an ancestor of the current request URL.


.. _LOCALIZE_MENU_ITEMS:

``WAGTAILMENUS_LOCALIZE_MENU_ITEMS``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Default value: ``False`` (or ``True`` when ``WAGTAIL_I18N_ENABLED = True``)

When enabled, wagtailmenus will resolve each menu item's ``link_page`` to its
translation in the currently active locale at render time, so that menu item
titles and URLs automatically reflect the visitor's language without any
additional configuration.

If ``WAGTAIL_I18N_ENABLED`` is set to ``True`` in your project's settings,
this feature is **enabled automatically** — you do not need to set
``WAGTAILMENUS_LOCALIZE_MENU_ITEMS`` explicitly. You can still opt out by
setting it to ``False`` even when ``WAGTAIL_I18N_ENABLED`` is ``True``.

.. code-block:: python

# e.g. settings/base.py

# Opt in explicitly (not required if WAGTAIL_I18N_ENABLED = True):
WAGTAILMENUS_LOCALIZE_MENU_ITEMS = True

# Opt out even when WAGTAIL_I18N_ENABLED = True:
WAGTAILMENUS_LOCALIZE_MENU_ITEMS = False

When active, menu items stored against a default-locale page will be displayed
using the equivalent page in the active locale (if a translation exists).
If no translation is found for the active locale, the original page is used as
a fallback. The localization is applied **at render time only** — the stored
``link_page`` foreign key on the menu item is never modified.

.. NOTE::
This feature requires `wagtail-localize <https://github.com/wagtail/wagtail-localize>`_
(or another package that provides ``Page.localized``) to be installed, and
pages to be translated via Wagtail's standard translation mechanism.
Menu items are configured once (against the default-locale pages) and the
correct locale is resolved automatically on each request.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
This feature requires `wagtail-localize <https://github.com/wagtail/wagtail-localize>`_
(or another package that provides ``Page.localized``) to be installed, and
pages to be translated via Wagtail's standard translation mechanism.
Menu items are configured once (against the default-locale pages) and the
correct locale is resolved automatically on each request.
Menu items are configured once, against the default-locale page. For this feature to do anything, your pages need translated versions. `wagtail-localize<https://github.com/wagtail/wagtail-localize>`_ is the usual way to produce them, but any Wagtail translation workflow that creates linked ``Locale`` copies will work.

Page.localized comes from the Wagtail core, so I'm suggesting the rewording above.

2 changes: 2 additions & 0 deletions wagtailmenus/conf/defaults.py
Comment thread
MrCordeiro marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@
# Miscellaneous settings
# ----------------------

LOCALIZE_MENU_ITEMS = False

ACTIVE_CLASS = 'active'

ACTIVE_ANCESTOR_CLASS = 'ancestor'
Expand Down
9 changes: 9 additions & 0 deletions wagtailmenus/conf/settings.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
import sys

from django.conf import settings as django_settings
from cogwheels import BaseAppSettingsHelper


class WagtailmenusSettingsHelper(BaseAppSettingsHelper):
deprecations = ()

def __getattr__(self, name):
value = super().__getattr__(name)
# Auto-detect locale-awareness from Wagtail's own i18n flag when the
# wagtailmenus-specific setting has not been explicitly overridden.
if name == 'LOCALIZE_MENU_ITEMS' and not value and not self.is_overridden('LOCALIZE_MENU_ITEMS'):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Quick question: what is not value guarding against?

is_overridden already tells us nothing was set explicitly, so value has to be the default. If we ever change the default, this check quietly disables the WAGTAIL_I18N_ENABLED fallback and nobody notices.

return bool(getattr(django_settings, 'WAGTAIL_I18N_ENABLED', False))
return value


sys.modules[__name__] = WagtailmenusSettingsHelper()
52 changes: 44 additions & 8 deletions wagtailmenus/models/menus.py
Original file line number Diff line number Diff line change
Expand Up @@ -963,6 +963,11 @@ def get_top_level_items(self):
# allow this query result to be reused by get_pages_for_display()
self._raw_menu_items = menu_items

# pages_for_display triggers get_pages_for_display(), which populates
# self._localize_id_map when LOCALIZE_MENU_ITEMS is active.
pages = self.pages_for_display
localize_map = getattr(self, '_localize_id_map', {})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This bit made me a bit anxious: _localize_id_map is built somewhere else so there an implicit contract being glued in line 968. The self.pages_for_display only works because pages_for_display is a @cached_property.

If we get_pages_for_display ever carries new side-effect

I was wondering whether we could to extract this computation to a cached_property so that get_top_level_items also computes the map and everyone just reads self._localize_id_map.


top_level_items = []
for item in menu_items:

Expand All @@ -971,10 +976,14 @@ def get_top_level_items(self):
top_level_items.append(item)
continue

# Translate the stored (default-locale) FK to the active-locale id
# when localization is active, without mutating item.link_page_id.
lookup_id = localize_map.get(item.link_page_id, item.link_page_id)

# But, we only want to include links to pages if the page was
# in the get_pages_for_display() result
try:
item.link_page = self.pages_for_display[item.link_page_id]
item.link_page = pages[lookup_id]
top_level_items.append(item)
except KeyError:
continue
Expand All @@ -996,22 +1005,49 @@ def get_pages_for_display(self):
# Start with an empty queryset, and expand as needed
queryset = Page.objects.none()

# When localization is active, record a mapping of stored (default-locale)
# page ID → active-locale page ID so that get_top_level_items() can look
# up pages in pages_for_display using the right key without mutating the
# stored FK on the menu item.
localize_id_map = {}

for item in (item for item in menu_items if item.link_page):
if settings.LOCALIZE_MENU_ITEMS:
# Resolve the active-locale page without touching item.link_page
# (avoids persisting the swap via admin save).
effective_page = item.link_page.localized or item.link_page

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion - feel free to ignore it!

This bit is effectively doing the same walk already done in localize_id_map. You may want to update that map store the resolved page rather than just the ID, so the second walk gets it for free (but then you rename it to localize_page_map or something):

    # more stuff above
    menu_items = getattr(self, '_raw_menu_items', None) or self.get_base_menuitem_queryset()
    mapping = {}
    for item in menu_items:
        if item.link_page:
            localized = item.link_page.localized
            if localized and localized.pk != item.link_page.pk:
                mapping[item.link_page.pk] = localized
    return mapping

Then this line becomes:

effective_page = self.localize_page_map.get(item.link_page.pk, item.link_page)

if effective_page.pk != item.link_page.pk:
localize_id_map[item.link_page.pk] = effective_page.pk
else:
effective_page = item.link_page

if(
item.allow_subnav and
item.link_page.depth >= settings.SECTION_ROOT_DEPTH
effective_page.depth >= settings.SECTION_ROOT_DEPTH
):
# Add this branch to the overall `queryset`
queryset = queryset | Page.objects.filter(
path__startswith=item.link_page.path,
depth__lt=item.link_page.depth + self.max_levels,
path__startswith=effective_page.path,
depth__lt=effective_page.depth + self.max_levels,
)
else:
# Add this page only to the overall `queryset`
queryset = queryset | Page.objects.filter(id=item.link_page_id)

# Filter out pages unsutable display
queryset = self.get_base_page_queryset() & queryset
queryset = queryset | Page.objects.filter(id=effective_page.pk)

# Store the mapping so get_top_level_items() can translate lookup keys.
self._localize_id_map = localize_id_map

if settings.LOCALIZE_MENU_ITEMS:
# `queryset` already contains the localized pages.
# Apply the same live/show_in_menus/expired constraints as
# get_base_page_queryset() but only against the small set of pages
# in `queryset` (scales with menu size, not site size).
queryset = self.get_base_page_queryset().filter(
id__in=queryset.values('id')
)
else:
# Filter out pages unsuitable for display
queryset = self.get_base_page_queryset() & queryset

# Always return 'specific' page instances
return queryset.specific()
Expand Down
Loading
Loading