Skip to content

Conversation

@stokito
Copy link
Contributor

@stokito stokito commented Mar 9, 2024

Replaces #6863

@Self-Hosting-Group @systemcrash please review and merge before it stalled and got merge conflicts

stokito added 2 commits March 9, 2024 19:03
Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
- Revise wording, remove redundancies and mention IPv6 limitations
- Mention `UPnP IGD` as `UPnP` is probably not specific enough
- Include current wording in `LUCI_TITLE` for easy finding

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
@systemcrash systemcrash merged commit 78d0c7e into openwrt:master Mar 9, 2024
@stokito stokito deleted the luci-app-upnp_pcp branch March 9, 2024 21:23
@stokito
Copy link
Contributor Author

stokito commented Mar 9, 2024

Thank you

@Self-Hosting-Group
Copy link
Contributor

Self-Hosting-Group commented Mar 9, 2024

@Self-Hosting-Group @systemcrash please review and merge before it stalled and got merge conflicts

@stokito: Thank you for continuing
@systemcrash: Thanks for merging

An error occurred when transferring the PR. The correct wording for the list of port maps on the status page was not transferred correctly and there was also no time for the desired review. If possible, I would also manually adjust the added acronyms in many translation strings to reduce the translation work. How about undoing the last commits (with git reset --hard HEAD~2) and then committing it again so that the whole translation machinery doesn't start up already? In the meantime, it is also possible to do the (old?) PR myself, now that the email address (for SOB) exists.

@stokito
Copy link
Contributor Author

stokito commented Mar 10, 2024

Screenshot of Active Port Forwards

Do you mean applications/luci-app-upnp/htdocs/luci-static/resources/view/status/include/80_upnp.js:31

Previously it was Active UPnP Redirects.

In your PR it was changed Active UPnP IGD & PCP/NAT-PMP Port Forwards. I felt that this is too much and simplified it to Active Port Forwards. This allowed to reuse the same key in applications/luci-app-upnp/htdocs/luci-static/resources/view/upnp/upnp.js:113 which previously was Active UPnP Redirects.

I don't feel like it's that important to show UPnP there because it's anyway clear for those who knows or confusing for those who don't. Even more, I would like to see also static port forwardings there because I may forget to remove obsolete. But IMHO that's not worth to be implemented.

@Self-Hosting-Group
Copy link
Contributor

Sorry, I'm not happy about this. Active Port Forwards on the global status page sounds to me like it has nothing to do with UPnP IGD & PCP/NAT-PMP, as the correct (static) port forwards are under Network -> Firewall.

And I thought you were happy with this, as you gave it a thumbs up in the #6863 (comment) comment earlier.

@stokito
Copy link
Contributor Author

stokito commented Mar 10, 2024

If add the UPnP IGD & PCP/NAT-PMP in a middle then we should make the same change on the settings page because this is a same component. But then it will look too clumsy as for me:

Screenshot luci-app-upnp Active forwardings

IMHO everybody knows the UPnP and it should be just enough to mention the NATPMP in a description. Something like this:

UPnP IGD
UPnP IGD & PCP/NAT-PMP allows clients on the local network to automatically configure port forwards on the router. Also called Universal Plug and Play.

Active UPnP IGD Port Forwards

But anyway, I have nothing to add here and it was too much discussions that it's hard to follow. If you wish to change anything ask someone to send a PR.

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