Support for specifying STUN server: frontend (3/3)

[jotaen4tinypilot]

Resolves https://github.com/tiny-pilot/tinypilot/issues/1460. Stacked on https://github.com/tiny-pilot/tinypilot/pull/1646, blocked on https://github.com/tiny-pilot/tinypilot/pull/1657 and https://github.com/tiny-pilot/tinypilotkvm.com/issues/1053.

This PR adds the UI for specifying a STUN server, and eventually makes the feature available to the end-user.

→ Latest bundle off this branch, for testing the entire PR stack.

Demo

https://github.com/tiny-pilot/tinypilot/assets/83721279/8108dcf2-1fe4-42d1-8c2a-4066cc669ad0

Notes

• In contrast to the mockups in the ticket (e.g. the latest one), the proposed implementation here has a separate field for the host part and the port part of the STUN address. I realized that this simplifies a lot of things, because we don’t need to parse and split a unified address string (e.g. stun.example.org:3478), or serialize it again correctly. That would be especially tricky, since Janus supports IPv6, where the serialized format would look like e.g. [12:c1:1832::c1:2]:3478, so we can’t just naively split and join at the : character.
• I separated the STUN input part into its own component, otherwise the <video-settings-dialog> would have become pretty large.
  • I debated whether to split off the entire “Advanced Settings” section, or just the STUN input. I eventually settled with the latter, because I felt it was conceptually more fitting, and I thought it would be the more granular approach, e.g. should we add more controls to the advanced settings in the future. I’m not married to it, though – I also think the entire <video-settings-dialog> component is borderline big anyway, so should we ever expand it further, it would probably be worth to consider a refactoring.
• I had to refactor (reverse) the CSS logic for showing/hiding the .setting-... classes. With the current implementation, it only allowed for display: flex, but with the new code, we also need display: block (on .advanced-settings).
• Note that the #stun-validation-error is still part of <video-settings-dialog>, since validation errors are supposed to be “dialog-wide” errors, displayed at the bottom of the dialog, near the call-to-action button. If there were more inline / validation errors in the future, we’d also add them there, at top level.
• One note about terminology: in the Janus config, the two address components are called server and port. I took over that terminology in the entire code, but in the UI I labelled the server fragment “Host”. To me, that’s technically more accurate, and since we already say “STUN Server” next to the dropdown button already, I thought “Server” might be confusing. I’m not sure there is a “right” answer here. We could otherwise also call it “Address”, but I feel that’s even broader.
• The UI implementation has a few edge-cases with slightly unexpected/quirky behavior. We’d need to pay with more code and complexity to sort them out, though, and I felt that wouldn’t be worth it. Examples:
  • If you select “Custom” in the dropdown and then manually fill in the Google server details (stun.l.google.com + 19302), then it would display the “Google” option the next time you open the dialog, and not the custom input fields. That’s because both ways look the same internally, and we’d need to maintain extra state to distinguish whether this was a custom input or one of the predefined selections.
  • If you select “Custom” and enter an invalid or incomplete address, and then choose “MJPEG” as streaming mode again, and then hit the “Apply” button, it still displays the STUN validation error. The reason is that the entire code around processing video settings is designed to always send and apply all values, regardless of what mode you eventually chose.

[jotaen4tinypilot]

Automated comment from CodeApprove ➜

⏳ @jdeanwallace please review this Pull Request

[jotaen4tinypilot]

@mtlynch if you have time, could you look over the revised <video-settings-h264-stun> component of this PR? Specifically, whether the component setup and structure are sufficiently clear to you in its current state. The other files of this PR are already successfully reviewed.

Don’t worry if you don’t have time, I’m not blocked on this. The rest of the PR stack is already merged; this is the last PR, which would effectively release STUN to end users.

[mtlynch]

@jotaen4tinypilot - Yeah splitting that section into its own component looks like a good idea to me.

I'll leave the in-depth review to Jason.

[jotaen4tinypilot]

Note to self: this PR is also blocked on https://github.com/tiny-pilot/tinypilotkvm.com/issues/1053, as those FAQ are linked from our UI. It seems as if this task will be resolved soon, though.

For reference, I’ve also asked Charles to do a final end-to-end test, as discussed here.