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/game closed event on main win close #1172

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

BrettCleary
Copy link
Collaborator

@BrettCleary BrettCleary commented Nov 26, 2024

Summary

Give game launch handlers 1 second on main window close to clean up (i.e. sync playsession and send game closed event).

closes https://github.com/HyperPlay-Gaming/product-management/issues/714

also fixes an issue where call all abort controllers was not working on main window close

Testing

https://app.qase.io/case/HC-528

@BrettCleary BrettCleary self-assigned this Nov 26, 2024
Copy link
Contributor

@jiyuu-jin jiyuu-jin left a comment

Choose a reason for hiding this comment

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

code looks good.

@@ -1161,228 +1182,231 @@ ipcMain.handle('isClientUpdating', async () => {

ipcMain.on('restartClient', () => autoUpdater.quitAndInstall())

// get pid/tid on launch and inject
ipcMain.handle(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i basically moved all of this logic out into handleGameLaunch so that we could store the promise in allLaunchHandlerPromises to await on app quit

@@ -161,6 +161,8 @@ export const trackEvent = async ({
}
}

export const flushEvents = async () => rudderstack.flush()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we have to flush the events in the launch handler so that Game Closed is sent on app close. Otherwise it is added to the cache and will wait a little while to see if there are other events that can be batch sent along with it

@@ -256,9 +257,12 @@ async function handleExit() {
logInfo([`Unable to kill ${procName}, ignoring.`, error])
}
})
}
// Kill all child processes, closing browser and native games that are running
callAllAbortControllers()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this has to be called before the cleanUp function so that that launch handlers can finish executing

@@ -29,7 +29,7 @@ function callAbortController(id: string) {
}

function callAllAbortControllers() {
for (const key in abortControllers.keys()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was causing native games launched through HP to not be closed when HP was closed

@BrettCleary BrettCleary added blocked PR: Ready-For-Review PR is ready to be reviewed by peers labels Dec 3, 2024
Base automatically changed from metrics/property_sessions to main December 5, 2024 07:42
@@ -1,6 +1,6 @@
{
"name": "hyperplay",
"version": "0.22.1",
"version": "0.22.2-analytics",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just for testing so I can filter for this version on the charts

@BrettCleary
Copy link
Collaborator Author

Pretty sensitive code. Will need your review on this one @flavioislima

Copy link
Contributor

@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.

Code looks good! left a small comment.

Did some basic testing with Epic and GOG games to check if the sync-saves would work on app close and they are working as they should.

Ideally would be good to test the launch command pretty well just to guarantee nothing is broken on any platform for any runner.

const allGameLaunchHandlersSettled = Promise.allSettled(
allLaunchHandlerPromises
)
const oneSecondTimeout = new Promise((res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you could use the wait helper method we have here:

Would need to make it accept a string, or make it always return TIME_OUT which is fine as well, if you need the message ofc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Ready-For-Review PR is ready to be reviewed by peers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants