add init function #355

utnapischtim · 2023-06-20T21:28:44Z

introduce shared menu outside of application context
global: refactor current_theme_icons

NOTE: it works without flask-menu and invenio-base release, BUT it works also if those are released first with flask>=3.0.0

slint · 2023-07-03T13:10:42Z

invenio_theme/shared.py

+from flask_menu.menu import MenuRoot
+
+menu = MenuRoot("", None)


Suggested change

from flask_menu.menu import MenuRoot

menu = MenuRoot("", None)

from flask_menu import Menu

menu = MenuRoot("", None)

menu_ext = Menu(root=menu)

major: see https://github.com/inveniosoftware/flask-menu/pull/85/files#r1250872290

Why i need menu_ext here?

For consistency with the Flask pattern we're going for tbh... I was even thinking that we could "proxy" the menu.submenu(...) call using a new Menu.submenu() method to avoid having to export a MenuNode object instead of the extension... But this can be easily done also later, once we know that it all works ™️

i personally don't like the Menu.submenu() approach. It feels like a singleton and i used them to often in the past. in my opinion they produce more problems that they solve.

the menu_ext will not be used somewhere so it is only to initialize the Menu on the parsing the python file level?

is it really necessary to initialize Menu directly? i think i didn't do that somewhere and the tests are all green also on other packages that are using the PR.

slint

Overall LGTM; some naming changes and consequences from the Flask-Menu comments

slint · 2023-07-03T13:17:41Z

invenio_theme/shared.py

minor: we're now establishing a naming convention for these types of "shared extensions" files. I'm aware that we have the same name in invenio-db, but e.g. the flask-cookiecutter uses extensions.py.

@ntarocco @zzacharo WDYT about the name?

slint · 2023-07-03T13:41:01Z

invenio_theme/shared.py

+from flask_menu.menu import MenuRoot
+
+menu = MenuRoot("", None)


For consistency with the Flask pattern we're going for tbh... I was even thinking that we could "proxy" the menu.submenu(...) call using a new Menu.submenu() method to avoid having to export a MenuNode object instead of the extension... But this can be easily done also later, once we know that it all works ™️

slint · 2023-07-03T13:52:42Z

invenio_theme/ext.py

@@ -81,9 +71,9 @@ def init_app(self, app, **kwargs):
        # Register context processor
        @app.context_processor
        def _theme_icon_ctx_processor():
-            from invenio_theme.proxies import current_theme_icons
+            return {"current_theme_icons": current_theme_icons}


Suggested change

return {"current_theme_icons": current_theme_icons}

return {

"current_theme_icons": self.icons,

"current_menu": menu,

}

use self.icons (since we have it now)

merge the dictionaries (and rename _theme_icon_ctx_processor() to _theme_ctx_processors()

I think current_menu is still fine as a name in templates because:

we should try not to break backward compatibility for Jinja template variables

there is a general convention for prefixing "added" Jinja variables with current_. The "current-ness" (i.e. relying on app/request context) is also real, and might e.g. give a hint in case one tries to use it in a non-request template (e.g. email notification template in a Celery task).

i thought i remove the current_ prefix, because the menu is not longer dependent on the context. (yes it was for the icons but it is not true there either, because we removed that part)

slint · 2023-07-03T13:55:42Z

invenio_theme/proxies.py

 current_theme_icons = LocalProxy(
    lambda: ThemeIcons(
        current_app.config["APP_THEME"], current_app.config["THEME_ICONS"]
    )
 )


Suggested change

current_theme_icons = LocalProxy(

lambda: ThemeIcons(

current_app.config["APP_THEME"], current_app.config["THEME_ICONS"]

)

)

current_theme_icons = LocalProxy(lambda: current_app.extensions["theme"].icons)

slint · 2023-07-03T13:57:50Z

tests/test_invenio_theme.py

    assert "THEME_SITENAME" in app.config


 def test_init_app(app):
    """Initialization."""
    theme = InvenioTheme()
-    assert theme.menu is None


minor: we should assert though that it's there now, even before init_app

but there exists no more theme.menu. so we should check against the existence of menu imported from shared.py?

I'm sorry, you're right, I was thinking of Menu.root (see also my correction)

On second thought, I'm not against keeping the InvenioTheme.menu and InvenioTheme.menu_ext pointers... The "global" instances in shared.py are there so that they can be used similarly to from invenio_db import db. We should still have some hard-connected references though to the InvenioTheme extension instance.

Should we merge menu and menu_ext somehow? invenio-db uses SQLAlchemy from flask-sqlalchemy but does not introduce two global variables. Further i think that menu_ext is somehow useless. i dont see the use case. i only see the use for menuand if we use theregister_menudecorator also themenu` use case is gone too.

* breadcrumbs are not longer used in invenio

* the main point of using a shared menu is to have the menu outside of the application context

* current_theme_icons doesn't need the application context anymore

* pytest-black seems out of support. the last commit was from 2020-10-05. Further pytest-black uses a out of date method which mentions pytest>=7.0 with a DeprecationWarning. The simplest solution is to use pytest-black-ng which fixes this situation.

* flask-menu adds current_menu as a context_processor * the current_ is a convention on jinja templates

* to have the blueprints consistently created

utnapischtim force-pushed the add-init-function branch from 975b629 to 78ade11 Compare June 29, 2023 21:59

slint reviewed Jul 3, 2023

View reviewed changes

utnapischtim force-pushed the add-init-function branch from e4ea646 to 48a5ee5 Compare August 21, 2023 20:51

utnapischtim marked this pull request as ready for review August 21, 2023 20:55

utnapischtim force-pushed the add-init-function branch from cd57c44 to 0799191 Compare December 9, 2023 23:07

utnapischtim added 8 commits March 19, 2024 22:35

global: remove breadcrumbs

a896927

* breadcrumbs are not longer used in invenio

global: introduce shared menu

b740de4

* the main point of using a shared menu is to have the menu outside of the application context

global: refactor current_theme_icons

65d2727

* current_theme_icons doesn't need the application context anymore

templates: use current_menu instead of menu

33ee9df

* flask-menu adds current_menu as a context_processor * the current_ is a convention on jinja templates

global: move blueprint and ui to views.py

a603135

* to have the blueprints consistently created

fix: docs

d6541d3

release: v3.0.0

64690c3

utnapischtim force-pushed the add-init-function branch from 0799191 to 64690c3 Compare March 20, 2024 13:05

chore: formatting

b743e9d

kpsherva merged commit 1f6618f into inveniosoftware:master Mar 20, 2024
3 checks passed

utnapischtim deleted the add-init-function branch March 20, 2024 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add init function #355

add init function #355

utnapischtim commented Jun 20, 2023 •

edited

Loading

slint Jul 3, 2023

utnapischtim Jul 3, 2023

slint Jul 3, 2023

utnapischtim Jul 3, 2023

slint left a comment

slint Jul 3, 2023

slint Jul 3, 2023

slint Jul 3, 2023

utnapischtim Jul 3, 2023

slint Jul 3, 2023

slint Jul 3, 2023

utnapischtim Jul 3, 2023

slint Jul 3, 2023

slint Jul 3, 2023

utnapischtim Jul 5, 2023

		from flask_menu.menu import MenuRoot

		menu = MenuRoot("", None)

add init function #355

add init function #355

Conversation

utnapischtim commented Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utnapischtim commented Jun 20, 2023 •

edited

Loading