-
-
Notifications
You must be signed in to change notification settings - Fork 442
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
[General] Beta-Feature: GOG implementation #872
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good overall. Great job here! ⚔️
Most of the comments are about improving readability or about missing comments or future improvement.
Looking at the code like this it's hard to find some bug so I didn't focus on that.
I believe the first iteration is amazing, better than we planned at first. So After release we can focus on improving the code, refactoring, etc. :D
Ah, do not forget to generate the i18n keys as well. |
I believe we also need the platform filter on Linux. |
Summary of the PRFeatures for GOG games
General features
ScreenshotsNotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Great!
Colossal work! ⚔️ 🛡️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey! I added comments mainly on code style, I don't really know how parts of this work to say if anything needs changes, hope the code style suggestions are useful
electron/constants.ts
Outdated
const icon = fixAsarPath(join(__dirname, '/icon.png')) | ||
const iconDark = fixAsarPath(join(__dirname, '/icon-dark.png')) | ||
const iconLight = fixAsarPath(join(__dirname, '/icon-light.png')) | ||
const installed = `${legendaryConfigPath}/installed.json` | ||
const libraryPath = `${legendaryConfigPath}/metadata/` | ||
const loginUrl = | ||
const fallBackImage = | ||
'https://user-images.githubusercontent.com/26871415/103480183-1fb00680-4dd3-11eb-9171-d8c4cc601fba.jpg' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this use an image inside the project? in case github is down or something weird
electron/gog/games.ts
Outdated
public async getExtraInfo(namespace: string): Promise<ExtraInfo> { | ||
const gameInfo = GOGLibrary.get().getGameInfo(this.appName) | ||
let targetPlatform: 'windows' | 'osx' | 'linux' = 'windows' | ||
switch (process.platform) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can use the isWindows isMac and isLinux from constants
?
like:
let targetPlatform: 'windows' | 'osx' | 'linux' = 'windows'
if (isMac && gameInfo.is_mac_native) {
targetPlatform = 'osx'
}
if (isLinux && gameInfo.is_linux_native) {
targetPlatform = 'linux'
}
electron/gog/games.ts
Outdated
}) | ||
} | ||
public async stop(): Promise<void> { | ||
const pattern = process.platform === 'linux' ? this.appName : 'gogdl' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here about using isLinux
from constants instead
electron/gog/games.ts
Outdated
const pattern = process.platform === 'linux' ? this.appName : 'gogdl' | ||
logInfo(['killing', pattern], LogPrefix.Gog) | ||
|
||
if (process.platform === 'win32') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and isWindows
const logPath = `"${heroicGamesConfigPath}${this.appName}.log"` | ||
const writeLog = isWindows ? `2>&1 > ${logPath}` : `|& tee ${logPath}` | ||
// In the future we need to add Language select option | ||
const command = `${gogdlBin} update ${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code above this line in this method is the same as the lines above line 233 in the repair method, maybe it's worth to extract?
electron/gog/library.ts
Outdated
* @param etag (optional) value returned in response, works as checksum so we can check if we have up to date data | ||
* @returns object {isUpdated, data}, where isUpdated is true when Etags match | ||
*/ | ||
public static async get_gamesdb_data( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here for the case, getGamesdbData
instead of get_gamesdb_data
for consistency, and for the rest of the functions in this file
electron/gog/setup.ts
Outdated
workingDir ? 'cd ' + workingDir + ' &&' : '' | ||
} ${commandPrefix} "${executablePath}" ${exeArguments}` | ||
// Requires testing | ||
if (process.platform == 'win32') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use isWindows
from constants
}) | ||
|
||
const gogAuthenticateUrl = | ||
'https://auth.gog.com/token?client_id=46899977096215655&client_secret=9d85c43b1482497dbbce61f6e4aa173a433796eeae2ca8c5f6129f2dc4de46d9&grant_type=authorization_code&redirect_uri=https%3A%2F%2Fembed.gog.com%2Fon_login_success%3Forigin%3Dclient&code=' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it ok that there's client_id and secret hardcoded like this? maybe it's needed but it looks suspicious to me, just checking (maybe this is the only way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is GOG Galaxy's client_id nothing to worry about
electron/launcher.ts
Outdated
winePath: string, | ||
appName: string | ||
) { | ||
if (platform() === 'darwin') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (isMac)
electron/main.ts
Outdated
@@ -971,7 +1026,7 @@ ipcMain.handle('updateGame', async (e, game) => { | |||
} | |||
|
|||
const epicOffline = await isEpicServiceOffline() | |||
if (epicOffline) { | |||
if (epicOffline && runner == 'legendary') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the check for the epic status service can be done only if runner === 'legendary'
? now it seems to trigger the check for the epic service even if not needed
WORK IN PROGRESS!
To do List:
Use the following Checklist if you have changed something on the Backend or Frontend: