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.

Sign up for GitHub

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

Jump to bottom

Qt/NetPlay: Implement session/server browser #7945

Merged
merged 5 commits into from
Apr 6, 2019
Merged

Qt/NetPlay: Implement session/server browser #7945

merged 5 commits into from
Apr 6, 2019

Conversation

spycrab
Copy link
Contributor

@spycrab spycrab commented Mar 29, 2019 •
edited
Loading

TODO:

  • Get it to build on Windows
  • Restore browser settings in the NetPlay setup dialog
  • Provide a test server for people to try these changes out

@spycrab spycrab added the WIP / do not merge Work in progress (do not merge) label Mar 29, 2019
@spycrab spycrab force-pushed the np_browser branch 5 times, most recently from 27205c6 to 28f0b11 Compare March 29, 2019 21:53
@JMC47
Copy link
Contributor

JMC47 commented Mar 29, 2019 •
edited
Loading

Bad password provided should be Incorrect password provided.
The "this make take a while" message blocks you from joining until you say okay.
there's no feedback on hitting refresh.

@spycrab spycrab force-pushed the np_browser branch 6 times, most recently from 2e978e4 to 35b4518 Compare March 30, 2019 00:30
@spycrab
Copy link
Contributor Author

spycrab commented Mar 30, 2019

@JMC47

Bad password provided should be Incorrect password provided.

Fixed.

The "this make take a while" message blocks you from joining until you say okay.

This dialog has been removed.

jordan-woyak
jordan-woyak reviewed Mar 30, 2019
View reviewed changes
Source/Core/UICommon/NetPlayIndex.cpp

if (!response)
{
m_last_error = "NO_RESPONSE";
Copy link
Member

Choose a reason for hiding this comment

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

These response checks seem to be duplicated in a few places.

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 don't think those can be deduplicated in a reasonable manner.

Copy link
Member

@jordan-woyak jordan-woyak Mar 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like a few could be replaced by a function that takes a url, performs a request, parses json and returns the json + error/status.

Source/Core/UICommon/NetPlayIndex.cpp Show resolved Hide resolved
Source/Core/UICommon/NetPlayIndex.cpp Outdated Show resolved Hide resolved
@spycrab
Copy link
Contributor Author

spycrab commented Mar 30, 2019

  • Properly reports player count now
  • Allows for non-standard ports now
  • Shows whether the session is in-game at the moment

@spycrab spycrab mentioned this pull request Mar 30, 2019
Closed
@spycrab
Copy link
Contributor Author

spycrab commented Mar 30, 2019

  • Game gets updated now
  • Properly retain the lobby checkbox

@spycrab spycrab removed the WIP / do not merge Work in progress (do not merge) label Mar 30, 2019
@spycrab
Copy link
Contributor Author

spycrab commented Apr 2, 2019

Important:
Migrated netplay index to a new server software so the URL has changed.
Please update your configs accordingly.

@spycrab spycrab force-pushed the np_browser branch 2 times, most recently from 836abd5 to daaf8ac Compare April 3, 2019 12:51
@JMC47
Copy link
Contributor

JMC47 commented Apr 4, 2019

I don't like all the abbreviated regions? I don't know what most of them are at a glance and it could confuse users.

@JosJuice
Copy link
Member

JosJuice commented Apr 4, 2019

Yeah, they're not all very intuitive. Keep in mind that they preferably should be translatable as well.

@spycrab
Copy link
Contributor Author

spycrab commented Apr 4, 2019

Regions are now displayed as Region (Abbreviation) e.g. Europe (EU) and are fully translatable.

lioncash
lioncash reviewed Apr 4, 2019
View reviewed changes
Source/Core/Common/HttpRequest.h Outdated Show resolved Hide resolved
Source/Core/DolphinQt/NetPlay/NetPlayBrowser.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/NetPlay/NetPlayBrowser.cpp Outdated Show resolved Hide resolved
Source/Core/UICommon/NetPlayIndex.cpp Outdated Show resolved Hide resolved
Source/Core/UICommon/NetPlayIndex.cpp Outdated Show resolved Hide resolved
Source/Core/UICommon/NetPlayIndex.cpp Outdated Show resolved Hide resolved
Source/Core/UICommon/NetPlayIndex.cpp Outdated Show resolved Hide resolved
Source/Core/UICommon/NetPlayIndex.cpp Outdated Show resolved Hide resolved
Source/Core/UICommon/NetPlayIndex.cpp Outdated Show resolved Hide resolved
Source/Core/UICommon/NetPlayIndex.cpp Outdated Show resolved Hide resolved
@spycrab spycrab force-pushed the np_browser branch 3 times, most recently from 1f6b844 to 36f99ad Compare April 4, 2019 19:53
@spycrab spycrab merged commit 23986d4 into dolphin-emu:master Apr 6, 2019
@spycrab spycrab deleted the np_browser branch April 6, 2019 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Techjar Techjar Techjar left review comments

@lioncash lioncash lioncash left review comments

@BhaaLseN BhaaLseN BhaaLseN left review comments

@jordan-woyak jordan-woyak jordan-woyak left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@spycrab @JMC47 @JosJuice @Techjar @lioncash @BhaaLseN @jordan-woyak