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

Improve server connection error messages #598

Merged
merged 4 commits into from
Apr 20, 2023

Conversation

pg9182
Copy link
Member

@pg9182 pg9182 commented Mar 5, 2023

It's already used for self auth and is available for both, but we currently only show it for the former.

recommends R2Northstar/NorthstarLauncher#433

It's already used for self auth and is available for both, but we
currently only show it for the former.
It never fully worked, and it's more useful to have non-localized
detailed error messages.
Copy link
Member Author

@pg9182 pg9182 left a comment

Choose a reason for hiding this comment

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

image

@pg9182 pg9182 changed the title Also show masterserver error for server connections Improve server connection error messages Mar 5, 2023
@BobTheBob9
Copy link
Member

i did not mean to approve this i meant to do requires changes FUCK

Copy link
Member

@BobTheBob9 BobTheBob9 left a comment

Choose a reason for hiding this comment

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

gecko said to do this

@pg9182 pg9182 force-pushed the ms-serverconn-err branch from 4fc3396 to 2f613e5 Compare April 19, 2023 14:09
@pg9182 pg9182 requested review from BobTheBob9, F1F7Y and uniboi April 19, 2023 14:10
@pg9182 pg9182 force-pushed the ms-serverconn-err branch 2 times, most recently from 83ebf63 to 2907d9c Compare April 19, 2023 14:15
The message always contains something like it, and it's always right
after a connectiom anyways.
@pg9182 pg9182 force-pushed the ms-serverconn-err branch from 2907d9c to fb87dbb Compare April 19, 2023 14:19
Copy link
Contributor

@uniboi uniboi left a comment

Choose a reason for hiding this comment

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

assuming reason is a string

Copy link
Contributor

@uniboi uniboi left a comment

Choose a reason for hiding this comment

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

This can only be a string. There is no point in making this untyped

@pg9182 pg9182 requested a review from uniboi April 19, 2023 16:11
Copy link
Contributor

@uniboi uniboi 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, didn't test in-game but no tests are needed since nothing was changed in NSGetAuthFailReason.

Copy link
Member

@F1F7Y F1F7Y 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, no need to test as this is a simple addition

@F1F7Y F1F7Y added the almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge label Apr 20, 2023
@F1F7Y
Copy link
Member

F1F7Y commented Apr 20, 2023

Waiting on @BobTheBob9 to be able to merge

@GeckoEidechse GeckoEidechse dismissed BobTheBob9’s stale review April 20, 2023 22:22

Requested changes have been addressed

@GeckoEidechse GeckoEidechse removed the almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge label Apr 20, 2023
@GeckoEidechse GeckoEidechse merged commit ecfdf4a into R2Northstar:main Apr 20, 2023
dialogData.message = Localize("#NS_SERVERBROWSER_CONNECTIONFAILED") + "\nERROR: " + reason + "\n" + Localize("#" + reason)
dialogData.message = reason
Copy link
Contributor

Choose a reason for hiding this comment

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

This, combined with R2Northstar/NorthstarLauncher@009482f, was a bad change that is causing some issues with helping users.

  • No localisation for the error reason
  • No longer shows the error code
  • Doesn't show the "Connection failed!" localisation, users don't know what happened

Before:

Connexión fallida!
ERROR: PLAYER_NOT_FOUND
No se encontró la cuenta del jugador

After:

Couldn't find player account

Copy link
Contributor

Choose a reason for hiding this comment

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

I like no localization more tbh

Copy link
Contributor

Choose a reason for hiding this comment

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

It's objectively worse for users. All helpers need is the error code so it doesn't affect them (except all our docs and stuff were based around the error code, which is no longer shown to the user at all)

Copy link
Member

Choose a reason for hiding this comment

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

We definitely should at the very least include the unlocalized error code.

We could also do something like this:

Connection failed! // Localized message
Code: <code> // Unlocalized code

Check 'wiki.northstar.tf/error_codes' for possible solutions // Localized message

Copy link
Contributor

Choose a reason for hiding this comment

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

My current thoughts is that we should move the "Connection failed!" bit to the header of the dialog as well.
Currently it looks like

ERROR // header

Couldn't find player account

Before it looked like

ERROR // header

Connection failed! // localised
ERROR: PLAYER_NOT_FOUND
Couldn't find player account // localised

IMO, it should look more like

Connection Failed! // header

Couldn't find player account // localised
Error code: PLAYER_NOT_FOUND

Check 'wiki.northstar.tf/error_codes' for possible solutions // localised

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way, launcher should be passing an error code if possible, not just a message

The error messages from atlas imo should be reserved for extra debugging information, and the error codes used to generate localised text

Copy link
Contributor

Choose a reason for hiding this comment

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

image
Something like this could be good, since this isnt a disconnect dialog we can add more buttons and do stuff like opening links. Could be good to get fewer people asking for help in discord?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants