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

feat(ui): add port mapping table #1681

Merged
merged 1 commit into from
Sep 30, 2023

Conversation

ReenigneArcher
Copy link
Member

@ReenigneArcher ReenigneArcher commented Sep 29, 2023

Description

Add a port configuration table to the UI.

Also validates the config port is is in the range of 1029 to 65514, and that all ports are in the range of 1024 to 65535.

Modifies alerts to include a font awesome icon.

Screenshot

Default value:
image

Custom value:
image

Invalid values:
image
image

Issues Fixed or Closed

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

  • I want maintainers to keep my branch updated

@LizardByte-bot
Copy link
Member

LizardByte-bot commented Sep 29, 2023

⚠️ Qodana is checking this PR ⚠️
Live results available here

@cgutman
Copy link
Collaborator

cgutman commented Sep 29, 2023

Can you add a description or some emphasis on the HTTP port entry that indicates that's the one that should be provided to Moonlight when adding a PC manually?

@yungleballz
Copy link

I would like to suggest to include an info that the Web UI Port does not have to (and should not) be forwarded.

@ReenigneArcher
Copy link
Member Author

@cgutman yes, good idea.

@yungleballz that will depend on user preferences. Many users forward it so they can modify settings away from home, or pair a new client.

@ReenigneArcher
Copy link
Member Author

ReenigneArcher commented Sep 29, 2023

Idea: Add port checker -> https://www.npmjs.com/package/is-port-reachable

This probably needs to be in a separate PR after webpack is implemented.

@TheElixZammuto
Copy link
Contributor

@yungleballz that will depend on user preferences. Many users forward it so they can modify settings away from home, or pair a new client.

I agree with yungle here, we need a big red scary warning to avoid people forwarding the UI without knowing what they do

Idea: Add port checker -> https://www.npmjs.com/package/is-port-reachable

This probably needs to be in a separate PR after webpack is implemented.

Due to NAT (and hairpin), having Sunshine itself make the test is not representitive, the Internet Streaming Tool uses a relay server IIRC

@ReenigneArcher
Copy link
Member Author

Due to NAT (and hairpin), having Sunshine itself make the test is not representitive, the Internet Streaming Tool uses a relay server IIRC

I wanted to add two columns.

  • Show if port is available on the localhost
  • Show if port is available using public ip address (maybe we can use this site somehow? https://canyouseeme.org/)

src/main.cpp Outdated Show resolved Hide resolved
src_assets/common/assets/web/config.html Show resolved Hide resolved
src_assets/common/assets/web/config.html Outdated Show resolved Hide resolved
@ReenigneArcher ReenigneArcher force-pushed the feat(ui)-add-port-mapping-table branch from 52b4d18 to 75b22df Compare September 30, 2023 15:12
@ReenigneArcher
Copy link
Member Author

@cgutman This will appear if the user specifies a non default port.

image

@ReenigneArcher
Copy link
Member Author

ReenigneArcher commented Sep 30, 2023

Modified UI to:

Default:
image

Non Default:
image

Too Low:
image

Too High:
image

@ReenigneArcher ReenigneArcher force-pushed the feat(ui)-add-port-mapping-table branch from 75b22df to d5bcefb Compare September 30, 2023 16:28
@ReenigneArcher
Copy link
Member Author

Added these warnings if web ui is allowed on WAN.

image

image

@ReenigneArcher ReenigneArcher force-pushed the feat(ui)-add-port-mapping-table branch from d5bcefb to ae7a9c4 Compare September 30, 2023 16:52
@ReenigneArcher ReenigneArcher force-pushed the feat(ui)-add-port-mapping-table branch from ae7a9c4 to 4298961 Compare September 30, 2023 16:55
@ReenigneArcher ReenigneArcher merged commit f76879e into nightly Sep 30, 2023
45 checks passed
@ReenigneArcher ReenigneArcher deleted the feat(ui)-add-port-mapping-table branch September 30, 2023 17:52
KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Jun 6, 2024
e-dong pushed a commit to e-dong/Sunshine that referenced this pull request Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants