[UX] Filter settings that don't apply to current platform and game #3997
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arielj
requested review from
a team,
flavioislima,
CommandMC,
Etaash-mathamsetty,
Nocccer and
imLinguin
and removed request for
a team
|
looks good, but you should reduce the amount of repeated code in the filtering function |
I made a comment about the repetition of the code in the PR description, I intentionally left it repeated (and I can reduce this if we agree it's not good, but I feel it's more clear this way) |
oh whoops I missed that |
CommandMC
reviewed
Nov 1, 2024
src/backend/logger/logger.ts
Outdated
| delete gameSettings['wineCrossoverBottle'] | ||
|
|
||
| if (notNative) { | ||
| const wineType = gameSettings['wineVersion'] |
There was a problem hiding this comment.
Suggested change
| const wineType = gameSettings['wineVersion'] | |
| const wineVersion = gameSettings['wineVersion'] |
src/backend/logger/logger.ts
Outdated
| notNative: boolean | ||
| ): Partial<GameSettings> { | ||
| // remove gamescope settings if it's disabled | ||
| const gscope = gameSettings['gamescope'] as Partial<GameScopeSettings> |
There was a problem hiding this comment.
Suggested change
| const gscope = gameSettings['gamescope'] as Partial<GameScopeSettings> | |
| const gscope: Partial<GameScopeSettings> | undefined = gameSettings['gamescope'] |
This may seem a little excessive, but always avoid as if you can do it somehow.
Why? Try changing 'gamescope' to 'someOtherNonexistantName'. No errors! Alternatively, try changing the GameSettings type, changing or removing gamescope. Again, no error here
src/backend/logger/logger.ts
Outdated
| @@ -507,7 +635,11 @@ class GameLogWriter extends LogWriter { | |||
|
|
|||
| // log game settings | |||
| const gameSettings = await gameManagerMap[runner].getSettings(app_name) | |||
| const gameSettingsString = JSON.stringify(gameSettings, null, '\t') | |||
| const gameSettingsString = JSON.stringify( | |||
| this.filterGameSettingsForLog({ ...gameSettings }, notNative), | |||
There was a problem hiding this comment.
Suggested change
| this.filterGameSettingsForLog({ ...gameSettings }, notNative), | |
| this.filterGameSettingsForLog(structuredClone(gameSettings), notNative), |
Right now, this will only shallow-clone the settings object
src/backend/logger/logger.ts
Outdated
Comment on lines
483
to
596
| delete gscope['windowType'] | ||
| } | ||
| } | ||
|
|
||
| // remove settings that are not used on Linux | ||
| if (isLinux) { | ||
| delete gameSettings['enableMsync'] | ||
| delete gameSettings['wineCrossoverBottle'] | ||
|
|
||
| if (notNative) { | ||
| const wineType = gameSettings['wineVersion'] | ||
| if (wineType) { | ||
| if (wineType['type'] === 'proton') { | ||
| delete gameSettings['autoInstallDxvk'] | ||
| delete gameSettings['autoInstallVkd3d'] | ||
| } | ||
| } | ||
| } else { | ||
| // remove settings that are not used on native Linux games | ||
| delete gameSettings['wineVersion'] | ||
| delete gameSettings['winePrefix'] | ||
| delete gameSettings['autoInstallDxvk'] | ||
| delete gameSettings['autoInstallDxvkNvapi'] | ||
| delete gameSettings['autoInstallVkd3d'] | ||
| delete gameSettings['enableFsync'] | ||
| delete gameSettings['enableEsync'] | ||
| delete gameSettings['enableFSR'] | ||
| delete gameSettings['showFps'] | ||
| delete gameSettings['enableDXVKFpsLimit'] | ||
| delete gameSettings['eacRuntime'] | ||
| delete gameSettings['battlEyeRuntime'] | ||
| delete gameSettings['useGameMode'] | ||
| } | ||
| } | ||
|
|
||
| // remove settings that are not used on Mac | ||
| if (isMac) { | ||
| delete gameSettings['useGameMode'] | ||
| delete gameSettings['gamescope'] | ||
| delete gameSettings['nvidiaPrime'] | ||
| delete gameSettings['battlEyeRuntime'] | ||
| delete gameSettings['eacRuntime'] | ||
| delete gameSettings['enableFSR'] | ||
| delete gameSettings['showMangohud'] | ||
| delete gameSettings['showFps'] | ||
|
|
||
| if (notNative) { | ||
| const wineType = gameSettings['wineVersion'] | ||
| if (wineType) { | ||
| if (wineType['type'] === 'wine') { | ||
| delete gameSettings['wineCrossoverBottle'] | ||
| } | ||
|
|
||
| if (wineType['type'] === 'toolkit') { | ||
| delete gameSettings['autoInstallDxvk'] | ||
| delete gameSettings['autoInstallDxvkNvapi'] | ||
| delete gameSettings['autoInstallVkd3d'] | ||
| } | ||
|
|
||
| if (wineType['type'] === 'crossover') { | ||
| delete gameSettings['autoInstallDxvk'] | ||
| delete gameSettings['autoInstallDxvkNvapi'] | ||
| delete gameSettings['autoInstallVkd3d'] | ||
| } | ||
| } | ||
|
|
||
| delete gameSettings['wineVersion'] | ||
| delete gameSettings['winePrefix'] | ||
| } else { | ||
| // remove settings that are not used on native Mac games | ||
| delete gameSettings['enableDXVKFpsLimit'] | ||
| delete gameSettings['wineVersion'] | ||
| delete gameSettings['winePrefix'] | ||
| delete gameSettings['wineCrossoverBottle'] | ||
| } | ||
| } | ||
|
|
||
| // remove settings that are not used on Windows | ||
| if (isWindows) { | ||
| delete gameSettings['enableMsync'] | ||
| delete gameSettings['enableFSR'] | ||
| delete gameSettings['enableEsync'] | ||
| delete gameSettings['enableFsync'] | ||
| delete gameSettings['enableDXVKFpsLimit'] | ||
| delete gameSettings['DXVKFpsCap'] | ||
| delete gameSettings['autoInstallDxvk'] | ||
| delete gameSettings['autoInstallDxvkNvapi'] | ||
| delete gameSettings['autoInstallVkd3d'] | ||
| delete gameSettings['gamescope'] | ||
| delete gameSettings['useGameMode'] | ||
| delete gameSettings['showMangohud'] | ||
| delete gameSettings['showFps'] | ||
| delete gameSettings['preferSystemLibs'] | ||
| delete gameSettings['wineCrossoverBottle'] | ||
| delete gameSettings['winePrefix'] | ||
| delete gameSettings['wineVersion'] | ||
| delete gameSettings['battlEyeRuntime'] | ||
| delete gameSettings['eacRuntime'] | ||
| delete gameSettings['nvidiaPrime'] | ||
| } |
There was a problem hiding this comment.
This whole function should use dot notation to access properties. Bracket notation currently hides some errors, since we have noUncheckedIndexedAccess turned on in our tsconfig
arielj
force-pushed
the
filter-settings-in-logs
branch
from
7f607ec to
2cfbcaa
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the behavior of the Game logger to remove settings from the output when the settings don't make sense for a given game and platform.
For example: it will filter out all the wine-related settings for native games, or hide the gamescope settings when the parent gamescope setting is disabled.
The idea behind this is that these settings that don't make any sense for a given situation are still displayed in the logs, adding noise and confusing the users and during support.
I think this is a good enough first iteration, I think there are probably more settings that can be removed specially on windows, but I don't have a machine with windows to verify (I was looking at the code of the different settings only).
Note: I know the code is kinda verbose, but it was intentional to make it super clear which elements are being removed an in which conditions. I could refactor this to set settings in an array and looping but I kinda felt this was better. I can change this if we don't agree.
Use the following Checklist if you have changed something on the Backend or Frontend: