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

Change cinema mode state to use query search params #131

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

cgmartin
Copy link
Contributor

@cgmartin cgmartin commented May 31, 2024

Hello! Thank you for this project!

My use-case is aggregating multiple remote game streams into OBS for a 4-way split screen (ie. Halo/Goldeneye multiplayer from days past).

When using browser sources in OBS, I wanted to automatically turn cinemaMode on via a URL param in the WHIP URLs, rather than manually pushing the Enable Cinema Mode button each time.

ie. http://localhost:3000/foo?cinemaMode=true

This is the code I'm using, in case it may be of any use to you.

Note: This change does not preserve backwards compatibility with the previous localstorage implementation.

@Sean-Der
Copy link
Collaborator

Glad it is useful :)

Could we do both localStorage + searchParam? I would do a || on the values maybe?

@cgmartin
Copy link
Contributor Author

Could we do both localStorage + searchParam? I would do a || on the values maybe?

I think I can manage that change. Just to confirm, should it work as follows?:

  1. Use the searchParam as priority if true, otherwise (else) use the localstorage value if true
  2. Update the searchParams AND localstorage whenever state changes.
  3. Delete the key/value in searchParams AND localstorage when value is false

@Sean-Der
Copy link
Collaborator

Sean-Der commented Jun 4, 2024

@cgmartin That sounds great! Thank you so much :)

I think everyone would be happy, and code wise that is pretty simple. Definitely don't need a distinct path for local + url param.

@cgmartin
Copy link
Contributor Author

cgmartin commented Jun 8, 2024

Hi @Sean-Der, I thought it would be simple as well, but I believe I may be getting hung up on this bug:
remix-run/react-router#9991

I'm not very familiar with React, honestly. Here is the new code I'm trying to make work:

export function CinemaModeProvider({ children }) {
  const [searchParams, setSearchParams] = useSearchParams();
  const [cinemaMode, setCinemaMode] = useState(() => localStorage.getItem("cinema-mode") === "true");

  useEffect(() => {
    const paramCinemaMode = searchParams.get("cinemaMode");
    const storedCinemaMode = localStorage.getItem("cinema-mode");
    const newValue = paramCinemaMode === "true" || (paramCinemaMode === null && storedCinemaMode === "true");
    setCinemaMode(newValue);
  }, [searchParams]);

  useEffect(() => {
    localStorage.setItem("cinema-mode", cinemaMode ? "true" : "false");
  }, [cinemaMode]);

  const state = useMemo(() => ({
    cinemaMode,
    setCinemaMode,
    toggleCinemaMode: () => {
      setCinemaMode(!cinemaMode);
      searchParams.set("cinemaMode", cinemaMode ? "true" : "false");
      setSearchParams(searchParams);
    }
  }), [cinemaMode, setCinemaMode, searchParams, setSearchParams]);

  return (
    <CinemaModeContext.Provider value={state}>
      {children}
    </CinemaModeContext.Provider>
  );
}

It is very odd. When I try modifying the search param value from ?cinemaMode=true to ?cinemaMode=false, the param key will unexpectedly change to lowercase and the delete param functionality breaks.

I doubt I will spend much more time on this to try getting working, but feel free to change and utilize how you see fit. Cheers!

@Sean-Der Sean-Der force-pushed the cinema-mode-param branch 3 times, most recently from 99f64d4 to 0c80733 Compare December 18, 2024 03:20
@Sean-Der
Copy link
Collaborator

Sorry this took so long @cgmartin but I am landing it now!

I made it so URL Param doesn't touch localStorage. The URL Param just overrides the local storage. I think that is a good experience.

I also added a note in the README

@Sean-Der Sean-Der merged commit 9eba6d6 into Glimesh:main Dec 18, 2024
1 check passed
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.

2 participants