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

Removal of navtree_depth makes some customization difficult #292

Open
laulaz opened this issue Feb 7, 2022 · 7 comments
Open

Removal of navtree_depth makes some customization difficult #292

laulaz opened this issue Feb 7, 2022 · 7 comments

Comments

@laulaz
Copy link
Member

laulaz commented Feb 7, 2022

Since the removal of navtree_depth in 7e2178d, it is way more difficult to override it.

For example, we have a use case for so called "site sections", where the navigation tree depth needs to be different than everywhere else in the site.
Since it is now a direct global settings call, we cannot just subclass GlobalSectionsViewlet locally, as we did before.

Is there a performance reason for this method removal ? or maybe we could have just put a @memoize decorator and keep it ?

Our code was :

class SectionNavigationViewlet(GlobalSectionsViewlet):

    @property
    @memoize
    def navtree_depth(self):
        return self.section_root.menu_depth

Now, to do the same, we have to override the enormous navtree function (:zany_face:) or make a proxy class for navigation settings (which we did).

@yurj
Copy link

yurj commented Feb 7, 2022

subclassing GlobalSectionsViewlet, can't you can change the settings property, thus having more flexibility?

@laulaz
Copy link
Member Author

laulaz commented Feb 7, 2022

@yurj

yes ... that's what we did, but it's not that simple, because settings is a "shortcut" for the registry ...

    def settings(self):
        return self.registry.forInterface(INavigationSchema, prefix="plone")

What we had to do :

class NavigationSettingsProxy(object):
    generate_tabs = None
    navigation_depth = None
    displayed_types = None
    sort_tabs_on = None
    sort_tabs_reversed = None
    nonfolderish_tabs = None
    filter_on_workflow = None
    workflow_states_to_show = None
    show_excluded_items = None

[...]

    @property
    def settings(self):
        settings = super(SectionNavigationViewlet, self).settings
        mutable_settings = NavigationSettingsProxy()
        mutable_settings.generate_tabs = settings.generate_tabs
        mutable_settings.displayed_types = settings.displayed_types
        mutable_settings.sort_tabs_on = settings.sort_tabs_on
        mutable_settings.sort_tabs_reversed = settings.sort_tabs_reversed
        mutable_settings.nonfolderish_tabs = settings.nonfolderish_tabs
        mutable_settings.filter_on_workflow = settings.filter_on_workflow
        mutable_settings.workflow_states_to_show = settings.workflow_states_to_show
        mutable_settings.show_excluded_items = settings.show_excluded_items
        # we need to change navigation_depth (but we cannot change registry
        # value), so we use our own settings object proxy.
        # This is caused by navtree_depth property deprecation.
        mutable_settings.navigation_depth = self.section_root.menu_depth
        return mutable_settings

@yurj
Copy link

yurj commented Feb 7, 2022

Yes, fine, I was thinking to a dictionary instead of a proxy class but this is the idea. Can you can also subclass and then do something like this:

class SectionNavigationViewlet(GlobalSectionsViewlet):


    def update(self):

        GlobalSectionsViewlet.update(self)
        self.settings.navigation_depth = self.section_root.menu_depth

?

Just guessing...

@laulaz
Copy link
Member Author

laulaz commented Feb 7, 2022

self.settings.navigation_depth = self.section_root.menu_depth

@yurj with this line, as self.settings is a records proxy, it would change the value in the registry 😜

@ale-rt
Copy link
Member

ale-rt commented Feb 8, 2022

It seems to me that the simplest thing to do is to remove the deprecation of the old method and restore the line that uses it.
Maybe add some detailed docstring that explain why the small method is useful.

@laulaz
Copy link
Member Author

laulaz commented Jun 29, 2022

See #301 which should be taken into account after merge, for later work.

@MrTango
Copy link
Contributor

MrTango commented Feb 23, 2024

who is working on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants