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

luci-base: force menu to regenerate after uci change #7058

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wryun
Copy link
Contributor

@wryun wryun commented Apr 12, 2024

Because the menu JSON can have 'depends' in them, uci changes should force the menu to regenerate.

Checklist suggested incrementing PKG_VERSION, but AFAIK for luci this is automatic.

Closes #6422

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <[email protected]> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Incremented 🆙 any PKG_VERSION in the Makefile
  • Tested on: (arm64, 23.05, Firefox + Chrome) ✅
  • ( Preferred ) Mention: @ the original code author for feedback
  • ( Preferred ) Screenshot or mp4 of changes:
  • ( Optional ) Closes: e.g. openwrt/luci#issue-number
  • ( Optional ) Depends on: e.g. openwrt/packages#pr-number in sister repo
  • Description: (describe the changes proposed in this PR)

Because the menu JSON can have 'depends' in them, uci changes
should force the menu to regenerate.

Signed-off-by: James Haggerty <[email protected]>
@systemcrash
Copy link
Contributor

Sooo, how does this differ from what happened in 97ebdcb ?

@jow-
Copy link
Contributor

jow- commented Apr 15, 2024

This time it tests for a .lua file extension before trying to execute a path as Lua controller. I still don't like this change though, uci depends in menus are meant for feature detection, not for real time display choices. There's other, non-uci depends such as filesystem or acl, those would still show the old caching behavior, leading to inconsistencies.

@systemcrash
Copy link
Contributor

There's other, non-uci depends such as filesystem or acl, those would still show the old caching behavior, leading to inconsistencies.

Could @wryun fix those also in this PR?

@jow-
Copy link
Contributor

jow- commented Apr 15, 2024

It still wouldn't change the fact that the menu would needlessly get regenerated on every single change.

@systemcrash
Copy link
Contributor

systemcrash commented Apr 15, 2024

It still wouldn't change the fact that the menu would needlessly get regenerated on every single change.

Needless, perhaps. But regeneration still is useful in some cases. Can't we gate those conditions to make triggering it more simple? Evidently there's a need.

@wryun
Copy link
Contributor Author

wryun commented Apr 16, 2024

From my perspective, I think the current behaviour is confusing, and realistically the cost of regenerating the menu vs the speed of applying changes in luci is low (any change automatically has a 4 sec hold-off, so what this adds is tiny by comparison).

Even without fixing the filesystem/acl stuff - which aren't currently causing user-facing issues, AFAIK - I think this is a useful improvement.

Note that even with the current base OpenWrt it can cause issues if people load the interface before the startup/hotplug stuff has finished: #6422 (comment)

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

Successfully merging this pull request may close these issues.

luci-base: menu item with uci depends doesn't update
4 participants