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

[Fix] Some things missed in the EOS Overlay PR #1563

Merged
merged 10 commits into from
Jul 16, 2022

Conversation

CommandMC
Copy link
Collaborator

  • We no longer try to get the latest overlay version if the overlay isn't installed & overlay_version.json isn't present (Legendary only creates overlay_version.json if the overlay is installed)
  • After installing the overlay on Windows, the Enable button will now correctly show Disable (as the overlay is automatically enabled on Windows)
  • When enabling the overlay on Linux without the Wineprefix being present, the Wineprefix will now get created
  • Some code cleanup

Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@CommandMC CommandMC added the pr:ready-for-review Feature-complete, ready for the grind! :P label Jul 11, 2022
@CommandMC CommandMC requested a review from Nocccer July 11, 2022 11:16
@CommandMC CommandMC self-assigned this Jul 11, 2022
Copy link
Collaborator Author

@CommandMC CommandMC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/screens/Settings/components/AdvancedSettings/index.tsx still invokes the isEosOverlayEnabled IPC call with one empty string (only on Windows, so it doesn't cause any issues, but it's still a little misleading IMO)

electron/legendary/eos_overlay/eos_overlay.ts Outdated Show resolved Hide resolved
src/screens/Game/GameSubMenu/index.tsx Outdated Show resolved Hide resolved
src/screens/Game/GameSubMenu/index.tsx Outdated Show resolved Hide resolved
src/screens/Game/GameSubMenu/index.tsx Outdated Show resolved Hide resolved
src/screens/Game/GameSubMenu/index.tsx Outdated Show resolved Hide resolved
@Nocccer
Copy link
Collaborator

Nocccer commented Jul 11, 2022

es the isEosOverlayEnabled with one empty

Wait a minute. I didn't know that is also in advancded settings? Can we only put it in one of the section for now?
I don't mind if we put it in gameSubMenu or Advancded page. For me it makes sense, in the advancded options part.

@CommandMC
Copy link
Collaborator Author

The advanced settings is where the main "control panel" is, where you can check for overlay updates, install/uninstall the overlay, and (on Windows) enable/disable it globally. You can only en-/disable it globally on Windows, so having a game-specific setting/button doesn't make sense. That's why it's there on Windows, and in the Tools page on Linux

@Nocccer
Copy link
Collaborator

Nocccer commented Jul 11, 2022

Ok make sense. From my side this whole thing needs more time and clean up. Better frontend handling (component).
We need a proper design how we wanna provide this to the user.

dxvk and vdk3d are for example downloaded on startup in the background if i'm correct. why not downloading eos_overlay on startup and add a checkbox to settings for eos like dxvk?

@CommandMC
Copy link
Collaborator Author

dxvk and vdk3d are for example downloaded on startup in the background if i'm correct. why not downloading eos_overlay on startup and add a checkbox to settings for eos like dxvk?

One simple issue: DXVK and VKD3D are 8 and 2 MB in size respectively. The overlay is ~330MB. We don't just want to reserve 1/3rd of a GB of Hard Drive space for a feature the user might never use/want

@CommandMC
Copy link
Collaborator Author

As for

From my side this whole thing needs more time and clean up. Better frontend handling (component).

Yes, definitely. I just wanted an initial implementation that's usable, so I don't have to run users through the song-and-dance of running Legendary manually (and don't even get me started on Flatpak permission issues)

@Nocccer
Copy link
Collaborator

Nocccer commented Jul 11, 2022

dxvk and vdk3d are for example downloaded on startup in the background if i'm correct. why not downloading eos_overlay on startup and add a checkbox to settings for eos like dxvk?

One simple issue: DXVK and VKD3D are 8 and 2 MB in size respectively. The overlay is ~330MB. We don't just want to reserve 1/3rd of a GB of Hard Drive space for a feature the user might never use/want

Also true. Still i think as ariel pointed out, we can bundle all the frontend stuff into one component. Maybe in another PR?
We can also provide a way to update eos automatically in the future.

So would say this is then a good usable starting point and we can final test/review it.

Copy link
Collaborator

@Nocccer Nocccer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this stable enough now. Frontend can be imrpoved in another PR.

@flavioislima
Copy link
Member

TEsting here and it has a bug with the SEttings.
If you are on the game settings and click on the Settings button to go to Global settings, the page breaks.
You need to change one line to fix it:

value={defaultInstallPath.replaceAll("'", '')}

Change to:
value={defaultInstallPath?.replaceAll("'", '')}

Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good besides the bug I found.

@CommandMC
Copy link
Collaborator Author

This isn't such a simple fix. The whole "General" settings are not loaded when switching from Game Settings -> General Settings.
This was not caused by this PR (I believe it was introduced all the way back in #1464), and it's way out of my league to fix it

@flavioislima
Copy link
Member

This isn't such a simple fix. The whole "General" settings are not loaded when switching from Game Settings -> General Settings. This was not caused by this PR (I believe it was introduced all the way back in #1464), and it's way out of my league to fix it

wdym by not loaded? for me they are showing fine.

@flavioislima flavioislima added the pr:ready-to-merge This PR is fully ready for merge. label Jul 15, 2022
@Nocccer
Copy link
Collaborator

Nocccer commented Jul 15, 2022

This isn't such a simple fix. The whole "General" settings are not loaded when switching from Game Settings -> General Settings. This was not caused by this PR (I believe it was introduced all the way back in #1464), and it's way out of my league to fix it

wdym by not loaded? for me they are showing fine.

It still breaks on specific tabs, but this happens in beta aswell.
https://user-images.githubusercontent.com/61798668/179235196-3c47b667-4a7f-42a1-88ba-a5ed413dd2b7.mp4

@CommandMC CommandMC force-pushed the fix/eos-overlay-empty-prefix branch from 8b97883 to 11c2e1c Compare July 15, 2022 14:58
@CommandMC
Copy link
Collaborator Author

CommandMC commented Jul 15, 2022

wdym by not loaded? for me they are showing fine.

Let me demonstrate: I've enabled "Exit to System Tray" in the Global Settings (and "Default Installation Path" is also set):
image

When going to the Game Settings -> General Settings, the checkbox isn't checked, default installation path isn't set, and the title still says "Fall Guys":
image

This error is also printed out when checking the box again:
image

@Nocccer
Copy link
Collaborator

Nocccer commented Jul 15, 2022

wdym by not loaded? for me they are showing fine.

Let me demonstrate: I've enabled "Exit to System Tray" in the Global Settings (and "Default Installation Path" is also set): image

When going to the Game Settings -> General Settings, the checkbox isn't checked, default installation path isn't set, and the title still says "Fall Guys": image

This error is also printed out when checking the box again: image

Can you check if it still happens on this PR?
#1579

@CommandMC CommandMC merged commit a82ccd8 into beta Jul 16, 2022
@CommandMC CommandMC deleted the fix/eos-overlay-empty-prefix branch July 16, 2022 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P pr:ready-to-merge This PR is fully ready for merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants