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: form.js: keep config for disabled options #4921

Closed
wants to merge 1 commit into from

Conversation

thg2k
Copy link
Contributor

@thg2k thg2k commented Mar 14, 2021

When using "depends" functionality, fields that do not satisfy the constraint are hidden from view, and on save they are removed from the uci configuration. This causes a loss of information previously stored by the user when for example temporarily disabling a certain functionality. Upon reactivation of the said functionality, it is an improved user experience to restore any configuration previously used.

What do you think about this @jow- ? I'm not sure of side effects this change might cause.

When using "depends" functionality, fields that do not satisfy the constraint are hidden from view, and on save they are removed from the uci configuration. This causes a loss of information previously stored by the user when for example temporarily disabling a certain functionality. Upon reactivation of the said functionality, it is an improved user experience to restore any configuration previously used.

Signed-off-by: Giovanni Giacobbi <giovanni@giacobbi.net>
@hnyman
Copy link
Contributor

hnyman commented Mar 14, 2021

I have been thinking about the same "loss of previous config" at disable, as preserving the config details of dependencies of the disabled option might be interesting some times.

(Leaving the not-in-use options there might confuse the user in some cases, though)

@jow-
Copy link
Contributor

jow- commented Mar 15, 2021

This is what the rmempty flag was about, originally, but over the years its meaning was conflated with optional and nowadays it is not easy to untangle without a tree-wide refactoring and retaining backwards compatibility at the same time.

I'd suggest to introduce a new property .retain instead which defaults to false. If an option is found to have unsatisfied dependencies and if .retain is set to true, the value is not removed.

I am against making this the default behavior though since - in conjunction with default values - it'll cause configs to suddenly contain a lot of unused options, confusing users and violating LuCI's design goal of producing the smallest possible config without setting redundant (where value matches the default) or unused options.

Note that for places where it makes sense, a similar behavior can already be achieved by setting .remove to an empty function.

@hnyman
Copy link
Contributor

hnyman commented Mar 15, 2021

The largest negative impact is on complex suboptions depending on the high-level options. E.g. disabling 80211r functionality causes you to lose all 15-20 R0/R1 keys & other config items that depend on 80211r option being enabled.

Would that proposed new .retain need to be separately defined for all the 10-20 different 80211r suboption fields?

Or could that .retain be defined for the top-level 80211r enabled option, meaning that preserve the lower-level suboptions even if this main options is disabled. The dependency would be evaluated in style of "enabled or retain". Would that work?

(Ps. right now that 80211r is not a huge problem any more, as most 80211r settings can be automately generated)

@jow-
Copy link
Contributor

jow- commented Jun 9, 2021

My proposal would require each individual sub option to be tagged. Since there is no real concept of "sub options" unless you want to apply it to all options of a section or if you consider any option a sub-option that is somehow dependent on a given option.

But walking the dependency tree is expensive and complex, I'd rather avoid adding too much logic for this comparatively minor use case.

@jow-
Copy link
Contributor

jow- commented Nov 18, 2021

Closing this as it will not be merged as-is.

@jow- jow- closed this Nov 18, 2021
@jow-
Copy link
Contributor

jow- commented Dec 9, 2021

Commit f5fbecf introduced a new retain property for options. You can set it on individual options whose contents should be kept when the options are disabled due to unsatisfied dependencies.

See also http://openwrt.github.io/luci/jsapi/LuCI.form.AbstractValue.html#retain

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.

3 participants