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

Rework MS list callback logic #927

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

lambdcalculus
Copy link
Contributor

@lambdcalculus lambdcalculus commented Nov 27, 2023

Removes the callback pattern from get_server_list and ms_request_finished since the only callback ever used in these was listing the servers anyhow.

Also fixes a segfault that happened when joining a server before the MS list was acquired (AO would try to have lobby list the servers while lobby didn't exist, after getting a reply for the MS list).

Technically to fix this crash only an if check around the callback was required, so if removing the callback pattern is too much I can have the pull request do only the check ¯\_(ツ)_/¯ (accessing lobby from the network manager IS kind of ugly).

edit: fix shrug emoji (important)

fixes crash when joining server before getting ms list back
Copy link
Member

@in1tiate in1tiate left a comment

Choose a reason for hiding this comment

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

admittedly I'm not sure why this callback pattern was implemented in the first place, I have a hunch it is vestigial. lgtm

@in1tiate in1tiate requested a review from Salanto November 28, 2023 01:25
Copy link
Contributor

@Salanto Salanto left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@stonedDiscord stonedDiscord merged commit e9469a5 into AttorneyOnline:master Nov 30, 2023
3 of 5 checks passed
@lambdcalculus lambdcalculus deleted the crash-fix branch July 5, 2024 18:03
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

Successfully merging this pull request may close these issues.

4 participants