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

Broadband adapter refactorings #7817

Open
wants to merge 13 commits into
base: master
from

Conversation

4 participants
@0xFEEDC0DE64
Copy link
Contributor

0xFEEDC0DE64 commented Feb 24, 2019

Makes the diff in my second pull request to add tcp support smaller / more readable

@0xFEEDC0DE64 0xFEEDC0DE64 force-pushed the 0xFEEDC0DE64:bba-refactorings branch 2 times, most recently from 1e7c35f to 5f435db Feb 24, 2019

@BhaaLseN
Copy link
Member

BhaaLseN left a comment

Mostly skipped the parts that only looked like renamings, but there is quite a few locations that would benefit from further cleanup...

Show resolved Hide resolved Source/Core/Core/HW/EXI/EXI_DeviceEthernetBase.cpp
Show resolved Hide resolved Source/Core/Core/HW/EXI/EXI_DeviceEthernetBase.cpp
Show resolved Hide resolved Source/Core/Core/HW/EXI/EXI_DeviceEthernetTAP.h Outdated
Show resolved Hide resolved Source/Core/DolphinQt/Settings/GameCubePane.cpp Outdated
Show resolved Hide resolved Source/Core/DolphinQt/Config/BBAConfigWidget.cpp
{
public:
CEXIETHERNET();
virtual ~CEXIETHERNET();

This comment has been minimized.

@BhaaLseN

BhaaLseN Feb 24, 2019

Member

I wonder if we should leave the Deactivate method alive (for symmetry with Activate) instead of moving it into the dtor. Any thoughts on that?

This comment has been minimized.

@0xFEEDC0DE64

0xFEEDC0DE64 Feb 24, 2019

Author Contributor

Activate is being called sometime during runtime (from MXCommandHandler). But Deactivate() was only called from inside the destructor.
We could instead just activate from the beginning?

This comment has been minimized.

@BhaaLseN

BhaaLseN Feb 24, 2019

Member

That would also be fine, assuming it would be symmetric (since it is a lot easier to find that way). I'm not familiar enough with that part of the code to know whether it can (or should) be in the ctor though.

This comment has been minimized.

@0xFEEDC0DE64

0xFEEDC0DE64 Mar 2, 2019

Author Contributor

Right now with this Activate() approach we would skip connecting to TAP or server for games which don't use the LAN adapter (like luigi's mansion for example).

@0xFEEDC0DE64 0xFEEDC0DE64 force-pushed the 0xFEEDC0DE64:bba-refactorings branch from 2382c4f to 8eb64fe Mar 2, 2019

@0xFEEDC0DE64

This comment has been minimized.

Copy link
Contributor Author

0xFEEDC0DE64 commented Mar 2, 2019

rebased branch again...

@@ -105,7 +105,7 @@ void GameCubePane::CreateWidgets()
for (const auto& entry :
{std::make_pair(tr("<Nothing>"), ExpansionInterface::EXIDEVICE_NONE),
std::make_pair(tr("Dummy"), ExpansionInterface::EXIDEVICE_DUMMY),
std::make_pair(tr("Broadband Adapter (TAP)"), ExpansionInterface::EXIDEVICE_ETH_TAP)})
std::make_pair(tr("Broadband Adapter (TAP)", "virtual network device"), ExpansionInterface::EXIDEVICE_ETH_TAP)})

This comment has been minimized.

@BhaaLseN

BhaaLseN Mar 2, 2019

Member

I wasn't aware that tr had a second parameter. I meant a comment like this:

// i18n: IR stands for infrared and refers to the pointer functionality of Wii Remotes

0xFEEDC0DE64 added some commits Mar 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.