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: signal/slot macro -> func pointer & network fixes #1007

Merged
merged 4 commits into from May 13, 2023

Conversation

Ryex
Copy link
Contributor

@Ryex Ryex commented Apr 18, 2023

  • convert qt connect calls to use function pointers instead of the signal/slot macros wherever practical (UI classes were mostly left alone, target was tasks and processes)
  • give signals an explicit receivers to use the static method over the instance method wherever practical
  • ensure networks tasks are using the errorOccured signal added in Qt5.15 over the deprecated error signal
  • ensure all networks tasks have an sslErrors signal connected
  • add seemingly missing MinecraftAccount::authSucceeded connection for MSAInteractive login flow
  • set x-xbl-contract-version header during xbox auth step (GDLauncher and ATLauncher set this)

@Ryex Ryex force-pushed the fix/network_and_signals branch 3 times, most recently from 6e97885 to 980362a Compare April 18, 2023 01:17
- convert qt connect calls to use function pointers instead of the signal/slot macros wherever practical (UI classes were mostly left alone, target was tasks and processes)
- give signals an explicit receivers to use the static method over the instance method wherever practical
- ensure networks tasks are using the `errorOccured` signal added in Qt5.15 over the deprecated `error` signal
- ensure all networks tasks have an sslErrors signal connected
- add seemingly missing `MinecraftAccount::authSucceeded` connection for `MSAInteractive` login flow

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
@TheKodeToad TheKodeToad mentioned this pull request Apr 18, 2023
5 tasks
@Ryex Ryex marked this pull request as ready for review April 19, 2023 00:42
@Ryex
Copy link
Contributor Author

Ryex commented Apr 19, 2023

This appears to fix a crash in #904.
I also have tentative reports that some of users previously having account issues who tested the CI build from this PR had them resolved.

Refrencing GDlauncher and ATLauncher code for auth as well as https://learn.microsoft.com/en-us/gaming/gdk/_content/gc/reference/live/rest/additional/httpstandardheaders
it is possible some of microsoft's server's are rejecting our request because of this missing header?

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

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

Noteworth: The fix in #904 is mostly due to adding the QNetworkReply::errorOccurred to Upload, which is necessary for task abortion to work correctly.

damn there's a lot of code that should probably use our net/ stuff but doesn't xd

great work here tho! :D

launcher/LoggedProcess.cpp Outdated Show resolved Hide resolved
launcher/java/JavaChecker.cpp Outdated Show resolved Hide resolved
@flowln flowln added bug Something isn't working simple change labels May 11, 2023
@flowln flowln added this to the 7.0 milestone May 11, 2023
Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
@DioEgizio
Copy link
Member

Conflicting with develop

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>
@Ryex
Copy link
Contributor Author

Ryex commented May 12, 2023

fixed

Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

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

Thanks!

@Scrumplex Scrumplex merged commit d5c6a1b into PrismLauncher:develop May 13, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working simple change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants