From 4fdccb283e65375784f46513c5f9e26c58cb749f Mon Sep 17 00:00:00 2001 From: Alex Hamlin Date: Tue, 26 Mar 2024 23:14:51 -0700 Subject: [PATCH 1/6] Simplify a bunch of client code I'm honestly shocked at how much complexity I put into the header implementation, with layers upon layers of functions obscuring the logic behind the various status indicators. Was this one of those cases where I did the whole "make all your functions as tiny as possible!" thing, before I permanently concluded that it's a terrible idea? Or was this more complex to start and I never refactored it as much as I needed to? --- client/src/App/ChannelSelector.tsx | 9 ++--- client/src/App/Header.tsx | 53 +++++++++--------------------- 2 files changed, 17 insertions(+), 45 deletions(-) diff --git a/client/src/App/ChannelSelector.tsx b/client/src/App/ChannelSelector.tsx index d8b2a83..7394b64 100644 --- a/client/src/App/ChannelSelector.tsx +++ b/client/src/App/ChannelSelector.tsx @@ -15,7 +15,6 @@ export default function ChannelSelector({ if (selected === undefined) { throw new Error("tried to tune before channels loaded"); } - try { await onTune(selected); } catch (e) { @@ -23,11 +22,7 @@ export default function ChannelSelector({ } }; - if (!(channelNames instanceof Array)) { - return null; - } - - return ( + return channelNames instanceof Array ? ( - ); + ) : null; } function Channel({ diff --git a/client/src/App/Header.tsx b/client/src/App/Header.tsx index 85c622e..102f43d 100644 --- a/client/src/App/Header.tsx +++ b/client/src/App/Header.tsx @@ -57,7 +57,10 @@ function StatusIndicator() { const webRTC = useWebRTC(); const tunerStatus = useTunerStatus(); - const indicatorActive = isActiveAndPlaying(webRTC, tunerStatus); + const indicatorActive = + webRTC.Connection.Status === "Connected" && + tunerStatus.Connection === "Connected" && + tunerStatus.State === "Playing"; return (
@@ -73,55 +76,29 @@ function StatusIndicator() { ); } -function isActiveAndPlaying( - webRTC: WebRTCState, - tunerStatus: TunerStatus, -): boolean { - if (webRTC.Connection.Status !== "Connected") { - return false; - } - - return ( - tunerStatus.Connection === "Connected" && tunerStatus.State === "Playing" - ); -} - function statusString(webRTC: WebRTCState, tunerStatus: TunerStatus): string { - const webRTCString = webRTCStatusString(webRTC); - if (webRTCString !== undefined) { - return webRTCString; - } - - return tunerStatusString(tunerStatus); -} - -function webRTCStatusString(state: WebRTCState): string | undefined { - if (state.Connection.Status !== "Connected") { - return state.Connection.Status; + if (webRTC.Connection.Status !== "Connected") { + return webRTC.Connection.Status; } - return undefined; -} - -function tunerStatusString(status: TunerStatus): string { - if (status.Connection !== "Connected") { - return status.Connection; + if (tunerStatus.Connection !== "Connected") { + return tunerStatus.Connection; } - if (status.State === "Stopped") { - if (status.Error !== undefined) { + if (tunerStatus.State === "Stopped") { + if (tunerStatus.Error !== undefined) { return "Failed to Tune"; } return "Powered Off"; } - if (status.State === "Starting") { - return `Tuning to ${status.ChannelName}`; + if (tunerStatus.State === "Starting") { + return `Tuning to ${tunerStatus.ChannelName}`; } - if (status.State === "Playing") { - return `Watching ${status.ChannelName}`; + if (tunerStatus.State === "Playing") { + return `Watching ${tunerStatus.ChannelName}`; } - return status.State; + return tunerStatus.State; } From 09b97e52ddcf3a168f57c8757782bacd1dc21864 Mon Sep 17 00:00:00 2001 From: Alex Hamlin Date: Tue, 26 Mar 2024 23:28:59 -0700 Subject: [PATCH 2/6] Simplify even more of the client The `handleTune` async function was the only unusually complex part of this. I guess I wanted to use await for the sake of using await, even though the Promise API can express the same logic in one line. The other stuff doesn't reduce actual complexity much, but I think it looks cleaner (in particular, how Prettier formats the new construction of the title text). --- client/src/App/ChannelSelector.tsx | 13 +------------ client/src/App/Title.tsx | 12 ++++++------ client/src/App/VideoPlayer.tsx | 9 +++------ 3 files changed, 10 insertions(+), 24 deletions(-) diff --git a/client/src/App/ChannelSelector.tsx b/client/src/App/ChannelSelector.tsx index 7394b64..fb9eed2 100644 --- a/client/src/App/ChannelSelector.tsx +++ b/client/src/App/ChannelSelector.tsx @@ -11,17 +11,6 @@ export default function ChannelSelector({ }) { const channelNames = useConfig("channels"); - const handleTune = async (selected: string) => { - if (selected === undefined) { - throw new Error("tried to tune before channels loaded"); - } - try { - await onTune(selected); - } catch (e) { - console.error("Tune request failed", e); - } - }; - return channelNames instanceof Array ? (
); } + +function Title() { + const tunerStatus = useTunerStatus(); + + const titleText = + tunerStatus.Connection === "Connected" && tunerStatus.State !== "Stopped" + ? `${tunerStatus.ChannelName} | Hypcast` + : "Hypcast"; + + return ( + + {titleText} + + ); +} + +function VideoPlayer({ stream }: { stream: undefined | MediaStream }) { + const videoElement = React.useRef(null); + + React.useEffect(() => { + if (videoElement.current !== null) { + videoElement.current.srcObject = stream ?? null; + } + }, [stream]); + + /* eslint-disable jsx-a11y/media-has-caption */ + // Lack of closed caption support is a longstanding deficiency in Hypcast. + // After experimenting with several approaches in GStreamer, I'm ashamed to + // say that I have yet to identify one that works reliably and consistently. + // Also, it is not clear that the eventual implementation of closed captions + // will involve WebVTT, which is what this rule actually looks for (through + // the presence of a element), so it may need to remain disabled even + // after closed caption support is in place. + return ( +
+
+ ); +} From 2e8e9d8efb760199486736cef8c5aa486c005717 Mon Sep 17 00:00:00 2001 From: Alex Hamlin Date: Tue, 26 Mar 2024 23:42:32 -0700 Subject: [PATCH 4/6] Keep simplifying things Once again, Prettier does a much better job with complex expressions outside of inline JSX attributes. Also, I was logging tune errors twice. Whoops. Just shows even more that the async stuff wasn't helpful. --- client/src/App/ChannelSelector.tsx | 6 ++---- client/src/App/index.tsx | 16 +++++++--------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/client/src/App/ChannelSelector.tsx b/client/src/App/ChannelSelector.tsx index fb9eed2..707c878 100644 --- a/client/src/App/ChannelSelector.tsx +++ b/client/src/App/ChannelSelector.tsx @@ -7,7 +7,7 @@ export default function ChannelSelector({ onTune, }: { selected?: string; - onTune: (ch: string) => Promise; + onTune: (ch: string) => void; }) { const channelNames = useConfig("channels"); @@ -18,9 +18,7 @@ export default function ChannelSelector({ key={ch} name={ch} active={ch === selected} - onClick={() => { - onTune(ch).catch((e) => console.error("Tune request failed", e)); - }} + onClick={() => onTune(ch)} /> ))} diff --git a/client/src/App/index.tsx b/client/src/App/index.tsx index 41fecf5..84c5b87 100644 --- a/client/src/App/index.tsx +++ b/client/src/App/index.tsx @@ -14,20 +14,18 @@ export default function App() { const webRTC = useWebRTC(); const tunerStatus = useTunerStatus(); + const selectedChannel = + tunerStatus.Connection === "Connected" && tunerStatus.State !== "Stopped" + ? tunerStatus.ChannelName + : undefined; + return (
<Header /> <ChannelSelector - selected={ - tunerStatus.Connection === "Connected" && - tunerStatus.State !== "Stopped" - ? tunerStatus.ChannelName - : undefined - } - onTune={(ChannelName) => - rpc("tune", { ChannelName }).catch(console.error) - } + selected={selectedChannel} + onTune={(ch) => rpc("tune", { ChannelName: ch }).catch(console.error)} /> <VideoPlayer stream={webRTC.MediaStream} /> </div> From 8cc293a8d7cfee8552737d20e4321d75d8c59b3e Mon Sep 17 00:00:00 2001 From: Alex Hamlin <alex@alexhamlin.co> Date: Tue, 26 Mar 2024 23:53:10 -0700 Subject: [PATCH 5/6] Make hook error messages more consistent --- client/src/WebRTC/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/WebRTC/index.tsx b/client/src/WebRTC/index.tsx index e695386..b380e9f 100644 --- a/client/src/WebRTC/index.tsx +++ b/client/src/WebRTC/index.tsx @@ -12,7 +12,7 @@ const Context = React.createContext<null | State>(null); export const useWebRTC = (): State => { const state = React.useContext(Context); if (state === null) { - throw new Error("useWebRTC must be called from within a <WebRTCProvider>"); + throw new Error("useWebRTC must be used within <WebRTCProvider>"); } return state; }; From 4044da35a258b9038938534899272b11a2822dd2 Mon Sep 17 00:00:00 2001 From: Alex Hamlin <alex@alexhamlin.co> Date: Wed, 27 Mar 2024 00:05:54 -0700 Subject: [PATCH 6/6] Don't limit color fades based on prefers-reduced-motion I'm not sure what made me think this counted as "motion." I guess it was more like: "here's an accessibility feature I just learned about; I must apply it somehow, because accessibility works best when you don't think too hard about the needs that drive it." Which is a bad way to (not) think. The docs for this on MDN give an example of a box with a growing and shrinking animation changing to a box that dissolves in and out (i.e. fades its color over time). Scaling and panning are where issues come in, and I can't think of where such motion would even make sense in the Hypcast UI. --- client/src/App/index.scss | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/client/src/App/index.scss b/client/src/App/index.scss index a9ee7bb..48976c7 100644 --- a/client/src/App/index.scss +++ b/client/src/App/index.scss @@ -19,12 +19,6 @@ $mobile-break: 639px; } } -@mixin if-motion-safe { - @media (prefers-reduced-motion: no-preference) { - @content; - } -} - .AppContainer { width: 100%; height: 100%; @@ -97,9 +91,7 @@ $mobile-break: 639px; cursor: pointer; background-color: $foreground; - @include if-motion-safe { - transition: background-color $transition-duration; - } + transition: background-color $transition-duration; &--Active { background-color: $accent; @@ -139,9 +131,7 @@ $mobile-break: 639px; margin-right: 6px; background-color: $foreground; - @include if-motion-safe { - transition: background-color $transition-duration; - } + transition: background-color $transition-duration; &--Active { background-color: $accent; @@ -175,14 +165,10 @@ $mobile-break: 639px; color: $foreground; background-color: $base-reallydark; - @include if-motion-safe { - transition: background-color ease-out $transition-duration; - } + transition: background-color ease-out $transition-duration; &:hover { background-color: $base-darker; - @include if-motion-safe { - transition-timing-function: step-start; - } + transition-timing-function: step-start; } &--Active {