luci-app-upnp: Complete plugin wording/UI revision

[Self-Hosting-Group]

• Remove clean_ruleset_interval/threshold UI options as not working
• Add missing protocol term in status/overview UI and shorten menu option to UPnP IGD & PCP, otherwise two lines
• Update UI placeholders to actual service defaults and upstream change miniupnp/miniupnp@9339f0e
• Order Protocol table column, slightly fix wording and add help texts
• Hide dependent UI fields if UPnP IGD protocol is disabled
• Improve UI by using revised package's UCI config options
• Simplify UI by moving rarely used options to advanced settings and the rest to general setup and rearrangement
• Manual adaption of added non-translatable protocol acronyms in translation files, see Integrity of translation source texts and changes. #7175

This completes PR #6863, merged in #6975.

Proposed service UI

Full screenshot

Proposed status/overview UI

Proposed advanced settings UI

Port Control Protocol (PCP) is the successor to NAT-PMP and has similar protocol concepts and packet formats, but adds IPv6 support. For more information, see Port Mapping Protocols Overview:
https://github.com/Self-Hosting-Group/wiki/wiki/Port-Mapping-Protocols-Overview

• 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 (does not exist)
• Tested on: (architecture, openwrt version, browser) ✅
• ( Preferred ) Mention: @ the original code author for feedback
• ( Preferred ) Screenshot or mp4 of changes:
• Description: (describe the changes proposed in this PR)

Maintainer: @jow-

[systemcrash]

So, what we typically find in the luci repo, in GUI, is that the first or second instance where space allows (and isn't in a heading) is to clarify acronyms and abbreviations. e.g. DNS = Domain Name System.

The best practice, in order to save inundating i18n peeps with needless html guff in text which can be error prone, is to use the .format() directive, and if the string is unclear when out of context, add a translator comment.

e.g. this is OK tho it's shit for translators:

			_('Dnsmasq is a lightweight <abbr title="Dynamic Host Configuration Protocol">DHCP</abbr> server and <abbr title="Domain Name System">DNS</abbr> forwarder.'));

but this is better so i18n peeps don't also need to copy and write html:

			_('Dnsmasq is a lightweight %s server and %s forwarder.').format('<abbr title="Dynamic Host Configuration Protocol">DHCP</abbr>', '<abbr title="Domain Name System">DNS</abbr>');

and this is even better so i18n peeps can understand what %s is. The second string in the _() is i18n explanation:

			_('Dnsmasq is a lightweight %s server and %s forwarder.', 'Dnsmasq is a lightweight %s (%s = DHCP) server and %s (%s = DNS) forwarder').format('<abbr title="Dynamic Host Configuration Protocol">DHCP</abbr>', '<abbr title="Domain Name System">DNS</abbr>');

[systemcrash]

If you must link to an acronym at a web resource, use format also to avoid link injection. Best is to link to RFCs which often clarify and explain the standard. Many acronyms ossify via English since that's what RFC are usually written in, and helps translators also.

[systemcrash]

see comments

[Self-Hosting-Group]

@systemcrash Thanks for the constructive/fast review and the suggestion to encode and link the acronyms. Linked the protocols to the standards on Wikipedia (hopefully also good), as the UPnP IGD is not from the IETF (RFC) and is only available as several separate PDFs.

For the clean_ruleset_* options, where the corresponding miniupnpd special function does not seem to work at all and therefore could not be analysed, it might be useful to wait and see if this is fixed or even removed upstream, see miniupnp/miniupnp#769. Also not sure how exactly clean_ruleset_threshold is defined when looking at the code and miniupnpd.conf. Earlier attempt at clarification: miniupnp/miniupnp#699.

Part of the advanced settings UI

[systemcrash]

.

[systemcrash]

*_threshold and *_interval are both made clear in the linked issue. What is the wording for *_threshold found there (that seems unclear)?

[Self-Hosting-Group]

Thanks for the feedback. The PR has been adjusted and extended a bit with dependencies to enable UI fields.

How about we switch to the preferred port map (instead of port forward) wording anyway, as it also fits better with IPv6, is shorter, and mention both port maps/forwards once in the introduction to help translators?

Regarding the open questions about the clean_ruleset_* wording, I want to exclude it for the moment and check first if the function works on OpenWrt and makes sense, or if the UI options should be removed at the same time. Removed in the meantime.

I would like to adjust an upstream configuration change and at the same time make some improvements to the UI and configuration options that also require a package update. Can I create a PR for this and then merge it at the same time?

[systemcrash]

While it might be nice to track variable names in the miniupnpd, it's mandatory to follow the naming in its startup script. What version does openwrt use in master? Because if a setting name is not in master yet, we cannot change it here yet.

[Self-Hosting-Group]

While it might be nice to track variable names in the miniupnpd, it's mandatory to follow the naming in its startup script. What version does openwrt use in master? Because if a setting name is not in master yet, we cannot change it here yet.

The only necessary prerequisite for miniupnpd (the linked commit, included in 2.3.4) is already in the master since openwrt/packages@c9a1705, and all other configuration changes are implemented without additional prerequisites and include a uci-defaults migration.

[stangri]

@Self-Hosting-Group would it be possible to nest the .format calls to allow localization of text in things like: <abbr title="UPnP Internet Gateway Device (Control Protocol)">UPnP IGD</abbr></a>?

[systemcrash]

Nesting is an entirely reasonable use case. I've done similar but not for the purpose of i18n. Most of the abbreviations and acronyms are hardened around English in any case.

[Self-Hosting-Group]

These are the previously mentioned proposed changes to the package:

miniupnpd: Revise several upnpd UCI configuration options and defaults

• Remove clean_ruleset_interval/threshold UCI config options as not working
• Rename UCI config option enable_nat_pmp to enable_pcp_pmp as upstream, see miniupnp/miniupnp@02da705
• Allow third-party PCP (daemon/non-UCI) config option when secure_mode UCI config option is disabled
• Add (one-line) patch to use secure_mode UCI config also for UPnP IGD with IPv6, previously it was always enabled and the behaviour is undocumented. See miniupnp/miniupnp@c79e25a
• Convert download/upload UCI config option from KByte/s to kbit/s and rename to *_kbps, and update defaults to 100/50 Mbit/s (informational only)
• New/clearer UPnP IGD compatibility mode upnp_igd_compat UCI config option accepts igdv1/igdv2, replacing the current igdv1 boolean option, allowing future compatibility modes
• Better document and reformat default upnpd UCI config file and add (template) ACL entry for low ports (<1024) denied by default, current behaviour

Open questions

1. What do you think of the package proposal for easier and more predictable setup?
2. What do you think of the phrase UPnP IGD compatibility mode and the option name upnp_igd_compat or more clearly as upnp_igd_compat_mode or with compatiblity or other suggestions?
3. Now that we have a more complete secure_mode option, perhaps it is the right time to consider renaming/inverting it to Allow third-party port maps and adding a help text of e.g. Allow adding port maps for non-requesting IP addresses [and allow third-party PCP option], with the option disabled by default. This would be the more meaningful/security-by-default wording, the current secure mode has been enabled by default in OpenWrt for a long time (around 2018), and the non-specific secure mode does not say much, possibly with a hint to the former name.
4. Should the UI options for download/upload speed state in the title or in the help that it is the maximum or max. speed?

[systemcrash]

Do not use default here. Use placeholder. Edit: in fact, remove this altogether. Let the underlying scripts drive the defaults.

[Self-Hosting-Group]

Thanks for the review. PR and screenshots updated. What do you think of the open questions in the comment above?

[systemcrash]

At a minimum, if you want to make changes to the GUI, they must first exist in miniupnpd.init.

I'm going to close this PR since it includes new settings which don't exist there, but you're welcome to ping this PR when necessary changes are integrated there. Otherwise, change this PR to include GUI changes only. There are some acceptable changes here.

1. Open a PR to the packages repo (and therein...)
2. Include a newer version of the daemon (and possibly only do that).
3. Include new settings in the init script
4. Include any 'changes' with backwards compatible helpers. (possibly in an entirely separate PR)

Be aware of what this entails:

• a working build environment
• you verify that the new daemon compiles
• any new patches for it work, and old patches still apply and work
• the newer daemon and new settings have been tested and verified in a working openwrt environment.

Before undertaking this venture, I advise you to review previous packages repo PR to change variable names in the packages repo. Review other PRs that update daemons (there are tons of them both open and merged). If you try and do too much at once, your chances of success there fall drastically, will take a longer time, and have a higher chance of stalling and rotting.

[Self-Hosting-Group]

The idea was a linked PR in the package repository. And I have already prepared the PR for the package. I was just interested in feedback on the idea of the package update and feedback on the wording for the next step. There is also a prepared uci-defaults script.

[systemcrash]

Please submit the packages PR.

[Self-Hosting-Group]

Added package PR and extended plugin PR.