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

Netplay: Make messages about non-matching games clearer #10685

Merged

Conversation

Pokechu22
Copy link
Contributor

Requested by JMC. I haven't tested this at all beyond confirming that it compiles, as I have no experience with netplay.

@Pokechu22 Pokechu22 requested a review from JosJuice May 21, 2022 06:21
@Pokechu22 Pokechu22 force-pushed the netplay-sync-identifier-comparison branch from 00ef23f to 3fd4b2b Compare May 21, 2022 06:51
// Agent), and notifying the user of different regions first is more helpful than just saying
// the game doesn't match
if (m_game_id[3] != sync_identifier.game_id[3])
return NetPlay::SyncIdentifierComparison::DifferentRegion;
Copy link
Member

@JosJuice JosJuice May 21, 2022

Choose a reason for hiding this comment

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

This will be confusing for users. If the user doesn't have any version of the selected netplay game, but they have any game that has a different fourth character from the selected game, Dolphin will say that they have the wrong game region instead of not having the game. I don't think that's worth handling the edge case of games that use different initial three characters in different regions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Users probably will notice if one has a Japanese-region game and the other doesn't, since those tend to have actual different languages (unlike US vs Europe) so yeah, it's probably fine to not handle that edge-case.

@Pokechu22 Pokechu22 force-pushed the netplay-sync-identifier-comparison branch from 3fd4b2b to 144b967 Compare May 21, 2022 18:28
Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

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

Code seems OK. @JMC47, can you do a UX check? In particular, I wonder whether the increased length of the strings will cause problems in the GUI.

@@ -613,7 +613,11 @@ void NetPlayDialog::UpdateGUI()

static const std::map<NetPlay::SyncIdentifierComparison, QString> player_status{
{NetPlay::SyncIdentifierComparison::SameGame, tr("OK")},
{NetPlay::SyncIdentifierComparison::DifferentVersion, tr("Wrong Version")},
{NetPlay::SyncIdentifierComparison::DifferentHash, tr("Game file has a different hash")},
Copy link
Member

Choose a reason for hiding this comment

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

This is technically misleading, as it implies the hash of the file itself is different rather than the sync hash, but I'll let that slide since the concept of a sync hash isn't something we can succinctly explain to users. And it is true that if the sync hash is different, the file hash is different – just not vice versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is good enough, since as you say if the sync hash is different, the file hash is different. Perhaps we should direct the user to the verify tab in this case, though? (Assuming there is enough space on the UI)

Copy link
Member

Choose a reason for hiding this comment

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

There is very little space in the UI – you're already stretching it. So I'd say let's leave this as it is.

@Pokechu22 Pokechu22 force-pushed the netplay-sync-identifier-comparison branch from 144b967 to 55d503d Compare May 21, 2022 19:11
@JMC47
Copy link
Contributor

JMC47 commented May 24, 2022

Okay, yeah, none of these messages fit with the default UI length on Windows.

I estimate we have room for ~12 characters for messages.

It's been unused since DolphinWX was removed in 44b22c9.  Prior to that, it was used in Source/Core/DolphinWX/NetPlay/NetWindow.cpp.  But the new equivalent in Source/Core/DolphinQt/NetPlay/NetPlayDialog.cpp uses NetPlayClient::GetPlayers instead.  Stringifying (or creating a table, as is done now) should be done by the UI in any case.
@Pokechu22 Pokechu22 force-pushed the netplay-sync-identifier-comparison branch from 55d503d to be78b9b Compare May 24, 2022 22:19
@Pokechu22
Copy link
Contributor Author

I've shortened the messages and put longer ones on tooltips for the table cells.

Here's what it looked like on master:

image

Here's the available messages with the old version of this PR:

image

And here's what they look like now:

image

@JMC47
Copy link
Contributor

JMC47 commented May 25, 2022

LGTM now.

@Pokechu22 Pokechu22 force-pushed the netplay-sync-identifier-comparison branch from be78b9b to 2341ff0 Compare May 25, 2022 06:21
@Tilka Tilka merged commit 7fcc866 into dolphin-emu:master May 25, 2022
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants