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

Qt: Add controller (overview) window #5377

Merged
merged 1 commit into from
May 9, 2017

Conversation

spycrab
Copy link
Contributor

@spycrab spycrab commented May 5, 2017

Mostly finished, but I want to get some Feedback before finishing this up.

TODO List:

  • Overview Interface
  • Load Settings
  • Save Settings
  • Bluetooth Passthrough Sync / Reset Buttons functional
  • Fix Windows build
  • Disable certain options while Emulation is running
    Configuration Windows (For Mapping etc.) Separate PR(s).

Current "Controller Overview" Window Look
Current "Controller Overview" Window Look

@Helios747
Copy link
Contributor

Helios747 commented May 5, 2017

I don't really like how this is split.

You have GC controllers all by themselves on the left with a ton of empty space, with everything else on the right. And background input isn't really worthy of it's own visual section. I get what you were going for, but I feel the old design was better.

@spycrab
Copy link
Contributor Author

spycrab commented May 5, 2017

Yeah, the split thing was more of an afterthought, but I can change this in one minute, so that's not really a problem.

The separate section is also present in the original Wx Dialog so I'm not sure what to do about that (As it's neither a GameCube nor a Wii option), I could slim it down a bit by not wrapping it in this "Advanced" box if that's preferable?

@Helios747
Copy link
Contributor

Ah, you're right, it looks fine in the old Wx UI because it isn't stretched across two sections

@spycrab
Copy link
Contributor Author

spycrab commented May 5, 2017

Yeah, I'll just see how it looks when I use the classic "top-down" approach and see what to do afterwards.

@Ryanel
Copy link
Contributor

Ryanel commented May 5, 2017

Hmm, everyone has pretty valid points, so here's an outside the box view:

If two sections is a problem, and the advanced groupbox is a problem, then why not just make it one section?

Let me explain using an image:
dolphinqt2d_2017-05-05_04-10-00

The settings dialog is tabbed. There are sections on the left, content on the right. Copy this, except have sections for GameCube, Wii, and Advanced.

  • This solves the problem of having side by side options, with lots of whitespace.
  • This familiar layout will make the UI more consistant
  • The code for that kind of layout is already present

I'm not proposing that Controller settings should be merged into the Main Settings Dialog, just simply that they mimic it's structure.

@spycrab spycrab force-pushed the qt_controller branch 2 times, most recently from ba7393d to 3bd86f3 Compare May 5, 2017 19:54

#pragma once

#include <QBoxLayout>

This comment was marked as off-topic.

QHBoxLayout* m_advanced_layout;
QCheckBox* m_advanced_bg_input;

static const std::unordered_map<SerialInterface::SIDevices, int> m_gc_types;

This comment was marked as off-topic.

static int ToGCMenuIndex(const SerialInterface::SIDevices&);
static SerialInterface::SIDevices FromGCMenuIndex(const int& menudevice);

static int ToWiimoteMenuIndex(const int& device);

This comment was marked as off-topic.

Q_OBJECT
public:
explicit ControllersWindow(QWidget* parent);
private slots:

This comment was marked as off-topic.

QGroupBox* m_gc_box;
QLabel* m_gc_label;
QFormLayout* m_gc_layout;
QComboBox* m_gc_controller_boxes[4];

This comment was marked as off-topic.

{
auto* box = static_cast<QComboBox*>(QObject::sender());

for (int i = 0; i < 4; i++)

This comment was marked as off-topic.

#include <QLabel>
#include <QMessageBox>
#include <QPixmap>
#include <iostream>

This comment was marked as off-topic.


void ControllersWindow::ConnectWidgets()
{
connect(m_wiimote_passthrough, SIGNAL(toggled(bool)), this, SLOT(OnWiimoteModeChanged(bool)));

This comment was marked as off-topic.


static int ToWiimoteMenuIndex(const int& device);
static int FromWiimoteMenuIndex(const int& menudevice);
};

This comment was marked as off-topic.

@@ -1,81 +1,100 @@
find_package(Qt5Widgets REQUIRED)
message(STATUS "Found Qt version ${Qt5Core_VERSION}")
find_package(Qt5Widgets REQUIRED) message(STATUS "Found Qt version ${Qt5Core_VERSION}")

This comment was marked as off-topic.

@spycrab
Copy link
Contributor Author

spycrab commented May 5, 2017

Updated GUI as suggested by @Helios747.

auto device = ios->GetDeviceByName("/dev/usb/oh1/57e/305");
if (device != nullptr)
{
static_cast<IOS::HLE::Device::BluetoothBase*>(device.get())->TriggerSyncButtonHeldEvent();

This comment was marked as off-topic.


if (device != nullptr)
{
static_cast<IOS::HLE::Device::BluetoothBase*>(device.get())->TriggerSyncButtonPressedEvent();

This comment was marked as off-topic.


for (int i = 0; i < 4; i++)
{
if (m_gc_controller_boxes[i] == box)

This comment was marked as off-topic.

{
auto* box = static_cast<QComboBox*>(QObject::sender());

for (int i = 0; i < 4; i++)

This comment was marked as off-topic.

@spycrab spycrab force-pushed the qt_controller branch 3 times, most recently from aa9cdbb to 87f3934 Compare May 5, 2017 21:26
@@ -1,7 +1,8 @@
// Copyright 2015 Dolphin Emulator Project

This comment was marked as off-topic.

@@ -1,4 +1,4 @@
// Copyright 2015 Dolphin Emulator Project
// Copyright 2017 Dolphin Emulator Project

This comment was marked as off-topic.

@@ -9,6 +9,7 @@
#include <QString>
#include <QToolBar>

#include "DolphinQt2/Config/ControllersWindow.h"

This comment was marked as off-topic.

{
auto it = std::find_if(gc_types.begin(), gc_types.end(),
[=](auto pair) { return pair.second == menudevice; });
return (*it).first;

This comment was marked as off-topic.

{
auto it = std::find_if(wiimote_types.begin(), wiimote_types.end(),
[=](auto pair) { return pair.second == menudevice; });
return (*it).first;

This comment was marked as off-topic.

#include "Core/IOS/USB/Bluetooth/BTReal.h"
#include "DolphinQt2/Settings.h"

#include "ControllersWindow.h"

This comment was marked as off-topic.


#include "ControllersWindow.h"

static const std::unordered_map<SerialInterface::SIDevices, int> gc_types = {

This comment was marked as off-topic.

QHBoxLayout* m_advanced_layout;
QCheckBox* m_advanced_bg_input;

static int ToGCMenuIndex(const SerialInterface::SIDevices);

This comment was marked as off-topic.

Copy link
Contributor

@MayImilae MayImilae left a comment

Choose a reason for hiding this comment

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

Can you place sub-options offset under them, like below? It makes it a lot clearer when an option is specific to a mode being active! (this is something our WX GUI doesn't have, but it really should!)

mockupcontrollers

@lioncash
Copy link
Member

lioncash commented May 5, 2017

@MayImilae should the emulation settings come first, and then the passthrough ones? (I'm assuming most users would emulate it, so it would likely be better to place that first if so).

@lioncash lioncash added RFC Request for comments WIP / do not merge Work in progress (do not merge) labels May 5, 2017
@spycrab
Copy link
Contributor Author

spycrab commented May 6, 2017

@MayImilae Adjusted it.

@MayImilae
Copy link
Contributor

@lioncash Honestly I'm totally fine with putting bluetooth passthrough up top. It makes it more apparent and encourages users to investigate it, instead of just using emulated forever.

@spycrab Much better! In the latest shot "Bluetooth Passthrough" is underlined. What does that mean?

@spycrab
Copy link
Contributor Author

spycrab commented May 6, 2017

That shows up when you use the keyboard to switch between options, it shows what you have selected.

@MayImilae
Copy link
Contributor

Hmm, interesting.

Oh btw, the config icons weren't meant to be the size you are displaying them there. That's not really something for this PR, but if you want size adjusted versions for any icons or new icons, please let me know!

@spycrab spycrab force-pushed the qt_controller branch 2 times, most recently from 234b416 to 3040084 Compare May 6, 2017 01:11

#include <array>

#include "Core/HW/SI/SI.h"

This comment was marked as off-topic.

@spycrab spycrab force-pushed the qt_controller branch 6 times, most recently from 608f8e4 to f97cfc0 Compare May 6, 2017 12:23
@spycrab spycrab changed the title [RFC] [WIP] Qt: Add controller (overview) window Qt: Add controller (overview) window May 6, 2017
@spycrab
Copy link
Contributor Author

spycrab commented May 6, 2017

@lioncash @JosJuice Should I create a separate PR for the configuration dialogs? I think the work could be split more efficiently if there was a base everyone can utilize.

@spycrab
Copy link
Contributor Author

spycrab commented May 6, 2017

Added a Windows screenshot.

@JosJuice JosJuice removed RFC Request for comments WIP / do not merge Work in progress (do not merge) labels May 6, 2017
@leoetlino
Copy link
Member

Really subjective opinion: I think the dialog would look a lot better if the controls were aligned. Right now, it's all very, very inconsistent :(

@spycrab
Copy link
Contributor Author

spycrab commented May 7, 2017

@leoetlino Hm, I'll see what I can do about that.

Copy link
Contributor

@Helios747 Helios747 left a comment

Choose a reason for hiding this comment

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

Other than the silly nitpicks, I think this looks fine to merge.

{
m_gc_box = new QGroupBox(m_gc_label);
m_gc_layout = new QFormLayout();

This comment was marked as off-topic.

m_wiimote_pt_labels[0] = new QLabel(tr("Sync real Wii Remotes and pair them"));
m_wiimote_pt_labels[1] = new QLabel(tr("Reset all saved Wii Remote pairings"));
m_wiimote_emu = new QRadioButton(tr("Emulate the Wii Bluetooth Adapter"));

This comment was marked as off-topic.

QRadioButton* m_wiimote_passthrough;
QPushButton* m_wiimote_sync;
QPushButton* m_wiimote_reset;
QCheckBox* m_wiimote_continous_scanning;

This comment was marked as off-topic.

m_wiimote_pt_labels[1] = new QLabel(tr("Reset all saved Wii Remote pairings"));
m_wiimote_emu = new QRadioButton(tr("Emulate the Wii Bluetooth Adapter"));

m_wiimote_continous_scanning = new QCheckBox(tr("Continous Scanning"));

This comment was marked as off-topic.

@spycrab spycrab force-pushed the qt_controller branch 2 times, most recently from 0602c5a to 7e3a4ba Compare May 8, 2017 19:34
@spycrab
Copy link
Contributor Author

spycrab commented May 9, 2017

@leoetlino Can this be merged now? I fixed the alignment and spelling mistakes (Can't update the screenshots ATM).

@Helios747
Copy link
Contributor

This has merge conflicts now.

@spycrab
Copy link
Contributor Author

spycrab commented May 9, 2017

Should be rather easy to fix.

@Helios747 Helios747 merged commit d21f1b2 into dolphin-emu:master May 9, 2017
@spycrab spycrab deleted the qt_controller branch May 9, 2017 18:06
@leoetlino leoetlino modified the milestone: Qt Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants