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

Rewrite it entirely #112

Closed
NikhilNarayana opened this issue Dec 9, 2020 · 5 comments
Closed

Rewrite it entirely #112

NikhilNarayana opened this issue Dec 9, 2020 · 5 comments
Assignees

Comments

@NikhilNarayana
Copy link
Member

NikhilNarayana commented Dec 9, 2020

The desktop app is long overdue for a rewrite. This issue is just so we can keep track of the overall work to be done. Work is being done in https://github.com/project-slippi/slippi-desktop-app/tree/refactor/rewrite and rewrite/* branches. Always make a branch off of refactor/rewrite with the name rewrite/<feature name>.

The rewrite is now open to outside contributions. However I suggest you discuss code design with me and Vince before implementing something so you don't waste time on something we don't like.

Deferred for now

Done

@vinceau
Copy link
Member

vinceau commented Dec 9, 2020

Some things to consider:

  1. How should we handle netplay Dolphin updates?
    Currently, when a user hits play, it auto-launches the netplay dolphin, which then prompts them to update. Ideally, we can check for updates before a user even clicks play, and downloads it for them with progress information. AFAIK there is no way for the Electron app to check the currently installed version.
    Perhaps, we could add a -v or --version flag when running the dolphin, which will just print the current version to stdout and then exit, which the app can then parse and determine if there's a new update.

  2. How can we download the playback Dolphin?
    When the app is launched, the netplay dolphin is auto-downloaded from the Ishiiruka Github releases. Ideally we can also download the playback dolphin too and auto-update that instance too if a new update is available (even more ideally we only have one dolphin which is either playback or netplay depending on the flags that it gets run with but I digress). The CI build has the ability to download the job artifacts but it's not possible to download artifacts using the public API. Perhaps we could have a slippi.gg endpoint which allows us to download the latest playback build, like slippi.gg/download/playback/win/latest or something.

  3. Terminology regarding "Play"
    Since we're combining "playing" Slippi Online, and "playing" back replays, It is probably better if we had separate terminology between the two. Perhaps "Watch" can be used for watching replays instead? But then would "Watch" confuse "watching" a replay versus watching a broadcast/console mirror?

@NikhilNarayana NikhilNarayana pinned this issue Dec 9, 2020
@NikhilNarayana
Copy link
Member Author

Perhaps, we could add a -v or --version flag when running the dolphin, which will just print the current version to stdout and then exit, which the app can then parse and determine if there's a new update.

This is easy enough to add.

  1. How can we download the playback Dolphin?
    When the app is launched, the netplay dolphin is auto-downloaded from the Ishiiruka Github releases. Ideally we can also download the playback dolphin too and auto-update that instance too if a new update is available (even more ideally we only have one dolphin which is either playback or netplay depending on the flags that it gets run with but I digress). The CI build has the ability to download the job artifacts but it's not possible to download artifacts using the public API. Perhaps we could have a slippi.gg endpoint which allows us to download the latest playback build, like slippi.gg/download/playback/win/latest or something.

It would be really nice if we had release channels on GitHub so we could release playback and netplay builds separately from each other. Since that isn't an option atm, we can just go with your endpoint idea.

  1. Terminology regarding "Play"
    Since we're combining "playing" Slippi Online, and "playing" back replays, It is probably better if we had separate terminology between the two. Perhaps "Watch" can be used for watching replays instead? But then would "Watch" confuse "watching" a replay versus watching a broadcast/console mirror?

Maybe you "play" netplay, "view" replays, and "watch" broadcasts?

@vinceau
Copy link
Member

vinceau commented Dec 19, 2020

I'm just going to briefly cover the project structure of the rewrite. Open to feedback and other changes.

The src folder is split into the following:

  • common
    • The code in common is for things that are shared between both main and renderer processes. Thus the code written should be agnostic to which thread its being imported from.
  • main
    • Code for the main process. e.g. electron config, menubars etc.
  • renderer
    • Most of your actual app code

The renderer folder is organised as follows:

  • components
    • Dumb components which are reusable throughout the app. These should not directly access or modify state but should accept handlers and state info via props.
  • containers
    • These components piece multiple dumb components together into a single "container". These can modify state and bind logic to the components but make sure most complex logic is in lib.
  • lib
    • This is basically your domain folder in the previous structure. Put all your reusable logic here so you can keep the components mainly representative and visual.
  • store
    • Your shared global app state.
  • styles
    • App styles and theming stuff
  • views
    • This one is pretty optional so it's cool if you think we should kill this folder. Basically every container here should represent a single page of the app that can be routed to. I find it gives me a starting point when I'm trying to find a specific component.

@vinceau
Copy link
Member

vinceau commented Jan 5, 2021

With the merging of #117 I think the refactor/rewrite codebase is probably mature enough for external contributors. In terms of next steps, I think the required tasks should be something along the lines of:

  1. Get versioning information into playback dolphin (--version command)
  2. Add downloading and updating of the playback dolphin (pending Auto download latest Playback Dolphin and auto update both dolphins #120)
  3. Redesign and implement a unified interface for console relaying/broadcasting.

@NikhilNarayana
Copy link
Member Author

The rewrite has been released as 2.0.0 🎉. Thank you to everyone that contributed, Vince and I are so excited about this release and we couldn't have done it without your help.

Deferred features will be made into separate Issues.

@NikhilNarayana NikhilNarayana unpinned this issue Jun 23, 2021
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

No branches or pull requests

2 participants