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

Minor multiplayer ui fixes #3676

Merged
merged 3 commits into from Apr 27, 2018

Conversation

Projects
None yet
4 participants
@jroweboy
Member

jroweboy commented Apr 20, 2018

One bug was found and reported on discord today where if you attempt to direct connect to a room that doesn't exist, and simultaneously try to join a room in the lobby, citra would pop up a message about "do you want to close the current network connection?" and if you choose yes, citra would crash. The first commit fixes this by preventing any join (through the lobby, direct connect, or hosting a new room) from happening while the network state is "Joining". The old behavior where citra pops up a message still happens if you've successfully joined a room.

The second commit was a bug I found while fixing the first. If you host a room and close it using the disconnect button in the client room, it wouldn't stop the server (as it was using the old close handler). This commit moves the Disconnect button to call the new, centralized close room handler in MultiplayerState


This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented Apr 20, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-04-20T06:56:32Z: c8d4ca8...jroweboy:c7726f13e87c87d2516c93ecb008b1606013f481

if (auto member = Network::GetRoomMember().lock()) {
member->Leave();
auto parent = static_cast<MultiplayerState*>(parentWidget());
if (!parent->OnCloseRoom()) {

This comment has been minimized.

@wwylele

wwylele Apr 20, 2018

Member

I might be dumb but I feel that this condition is inverted. Could you elaborate the logic here?

@@ -5,6 +5,7 @@
#pragma once
#include <QWidget>
#include "core/announce_multiplayer_session.h"

This comment has been minimized.

@lioncash

lioncash Apr 21, 2018

Member

Note that there's already a forward declaration for this below.

@jroweboy

This comment has been minimized.

Member

jroweboy commented Apr 25, 2018

Addressed review

@jroweboy jroweboy merged commit 252e5f1 into citra-emu:master Apr 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment