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

Display origin auth failure in a dialog #648

Merged
merged 13 commits into from
Oct 7, 2023

Conversation

ASpoonPlaysGames
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames commented Jun 10, 2023

Currently, we don't do anything with origin auth failure, meaning the next request just fails, and we get not particularly relevant error messages (player not found, invalid masterserver token)

Depends on

Still todo:

  • Localisation
  • Cooler links

@F1F7Y
Copy link
Member

F1F7Y commented Jul 9, 2023

* [ ]  Localisation

Do you plan on adding other languages?

@ASpoonPlaysGames
Copy link
Contributor Author

Do you plan on adding other languages?

I would like to tbh, but I'm not rly fussed at this point

@GeckoEidechse
Copy link
Member

Do you plan on adding other languages?

I would like to tbh, but I'm not rly fussed at this point

There's no need to add other languages in original PR. We got a weblate setup via translate.harmony.tf where it will show any untranslated strings and translators can add them via weblate once the PR has been merged.

@Alystrasz
Copy link
Contributor

translators can add them via weblate once the PR has been merged.

We gotta be careful about merging in all translations before releasing, then.

@GeckoEidechse
Copy link
Member

translators can add them via weblate once the PR has been merged.

We gotta be careful about merging in all translations before releasing, then.

Eh, like we don't even have everything translated yet, so if we do that we would just wait forever

image
https://translate.harmony.tf/projects/northstar/client/

@Alystrasz
Copy link
Contributor

Eh, like we don't even have everything translated yet, so if we do that we would just wait forever

French is now 100% 😄
Gotta ping people in the appropriate Discord channel.

@ASpoonPlaysGames ASpoonPlaysGames added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Jul 30, 2023
@catornot catornot self-requested a review August 29, 2023 22:11
@ASpoonPlaysGames
Copy link
Contributor Author

image

here is an example of how the dialog looks in game

Copy link
Contributor

@NoCatt NoCatt left a comment

Choose a reason for hiding this comment

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

Colour and layout looks good, the help will now take the common errors to the right wiki section. Code is kept to a minimum ( thats good ).

@ASpoonPlaysGames
Copy link
Contributor Author

I've also tested this pretty thoroughly when making the colours and designing the UI. But if someone else can test this that would be great. Best way I found is to change your origin token with a debugger to get a bad response back from stryder when you try and auth

@ASpoonPlaysGames ASpoonPlaysGames removed the needs code review Changes from PR still need to be reviewed in code label Sep 2, 2023
Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Information exposed in R2Northstar/NorthstarLauncher#468 is correctly displayed in a dialog.
(test done by manually corrupting the uidStr variable through the debugger)

image

@ASpoonPlaysGames ASpoonPlaysGames added READY TO MERGE This mergeable right now and removed needs testing Changes from the PR still need to be tested labels Sep 16, 2023
Copy link
Contributor

@NoCatt NoCatt 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, the link opened automatically jumps to the correct position in the troubleshooting section ( I made a PR for that )
Otherwise this will help with tickets and finding the issue, and hopefully prevent some

@GeckoEidechse GeckoEidechse added depends on another PR Blocked until the PR it depends on is merged and removed depends on another PR Blocked until the PR it depends on is merged labels Oct 7, 2023
Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Signing off based on reviewer feedback. Did not really properly look at this myself.

@GeckoEidechse GeckoEidechse merged commit 15b3b65 into R2Northstar:main Oct 7, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
READY TO MERGE This mergeable right now
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants