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

Add ability to send test cases through telemetry #3325

Merged
merged 11 commits into from Jan 26, 2018

Conversation

Projects
None yet
@BreadFish64
Contributor

BreadFish64 commented Dec 22, 2017

Adds a UI dialog that allows users to submit test cases through the QT frontend using a telemetry field.
@chris062689 has plans to integrate it with the compatibility list.
Pretty Pictures:
image
image
image
image


This change is Reviewable

@FearlessTobi

This comment has been minimized.

Contributor

FearlessTobi commented Dec 22, 2017

I would prefer having the Next Button greyed out until something is selected, instead of another message box. I don't know if its worth the effort to implement though.

@BreadFish64

This comment has been minimized.

Contributor

BreadFish64 commented Dec 22, 2017

@FearlessTobi the buttons at the bottom are rather inflexible, I investigated making the radio buttons a mandatory field, but it involved making a subclass of QWizardPage, more trouble than it's worth

~CompatDB();
private:
Ui::CompatDB* ui;

This comment has been minimized.

@lioncash

lioncash Dec 23, 2017

Member

This can be std::unique_ptr, which would remove the need to explicitly delete this in the destructor.

Q_OBJECT
public:
explicit CompatDB(QWidget* parent = 0);

This comment has been minimized.

@lioncash

lioncash Dec 23, 2017

Member

= nullptr

// Licensed under GPLv2 or any later version
// Refer to the license.txt file included.
#ifndef COMPATDB_H

This comment has been minimized.

@lioncash

lioncash Dec 23, 2017

Member

We use #pragma once as our header guard

if (compatibility->checkedId() != -1) {
Core::Telemetry().AddField(Telemetry::FieldType::UserFeedback, "Compatibility",
compatibility->checkedId());
delete compatibility;

This comment has been minimized.

@lioncash

lioncash Dec 23, 2017

Member

Qt should handle the automatic deletion of a QButtonGroup, since the wizard is being passed as a parent to it. the delete here and below seem unnecessary.

This comment has been minimized.

@BreadFish64

BreadFish64 Dec 23, 2017

Contributor

I guess you're probably more knowledgeable about it than I am, I just didn't know if would cause a memory leak if the user went back and forth between pages

CompatDB::CompatDB(QWidget* parent)
: QWizard(parent, Qt::WindowTitleHint | Qt::WindowCloseButtonHint | Qt::WindowSystemMenuHint),
ui(new Ui::CompatDB) {

This comment has been minimized.

@lioncash

lioncash Dec 23, 2017

Member

ui{std::make_unique<Ui::CompatDB>()}

#include "common/telemetry.h"
#include "compatdb.h"
#include "core/core.h"
#include "ui_compatdb.h"

This comment has been minimized.

@lioncash

lioncash Dec 23, 2017

Member

#include "citra_qt/ui_compatdb.h"

This comment has been minimized.

@BreadFish64

BreadFish64 Dec 23, 2017

Contributor

You sure about that? that file isn't in that directory

QList<QWizard::WizardButton> layout1;
layout1 << QWizard::Stretch << QWizard::BackButton << QWizard::CustomButton1
<< QWizard::CancelButton << QWizard::FinishButton;
this->setButtonLayout(layout1);

This comment has been minimized.

@lioncash

lioncash Dec 23, 2017

Member

this-> can be omitted here and below.

</font>
</property>
<property name="text">
<string>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;Independant of speed / performance, how well does the game play from start to finish on this version of Citra?&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>

This comment has been minimized.

@lioncash

lioncash Dec 23, 2017

Member

Independant of speed or performance, ...

#include <QMessageBox>
#include <QPushButton>
#include "common/telemetry.h"
#include "compatdb.h"

This comment has been minimized.

@lioncash

lioncash Dec 23, 2017

Member

#include "citra_qt/compatdb.h"

layout1 << QWizard::Stretch << QWizard::BackButton << QWizard::CustomButton1
<< QWizard::CancelButton << QWizard::FinishButton;
this->setButtonLayout(layout1);
this->setButtonText(QWizard::CustomButton1, tr("Next"));

This comment has been minimized.

@lioncash

lioncash Dec 23, 2017

Member

Why not just use QWizard::NextButton as opposed to QWizard::CustomButton1 with layout1 above? This would also alleviate the need for translators to translate one more string.

@chris062689

This comment has been minimized.

Member

chris062689 commented Dec 23, 2017

This functionality should be hidden from Canary builds, FYI.
Only Nightly builds should have this option.

~CompatDB();
private:
std::unique_ptr<Ui::CompatDB, std::default_delete<Ui::CompatDB>> ui;

This comment has been minimized.

@lioncash

lioncash Dec 24, 2017

Member

You don't need to specify default delete here. It's automatically chosen (hence the name signifying it's the in unique_ptr's template). It wouldn't be a very good smart pointer if it made you explicitly choose its default deleter ;)

</font>
</property>
<property name="text">
<string>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;Independant of speed or performance, how well does the game play from start to finish on this version of Citra?&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>

This comment has been minimized.

@lioncash

lioncash Dec 24, 2017

Member

I missed this the first time around, but 'Independant" should be "Independent"

@@ -795,6 +799,7 @@ void GMainWindow::OnStartGame() {
ui.action_Pause->setEnabled(true);
ui.action_Stop->setEnabled(true);
ui.action_Report_Compatibility->setEnabled(true);

This comment has been minimized.

@baka0815

baka0815 Dec 28, 2017

Contributor

Should probably only be enabled when the collection of telemetry-data is configured completely.

This comment has been minimized.

@BreadFish64

BreadFish64 Dec 28, 2017

Contributor

Right, I'll make sure the user is verified

@chris062689

Small nit - We call them "Citra Accounts", not forum accounts in the first dialog box.

@@ -13,6 +13,7 @@ option(ENABLE_QT "Enable the Qt frontend" ON)
option(CITRA_USE_BUNDLED_QT "Download bundled Qt binaries" OFF)
option(ENABLE_WEB_SERVICE "Enable web services (telemetry, etc.)" ON)
option(ENABLE_COMPATIBILITY_REPORTING "Enable compatibility reporting" OFF)

This comment has been minimized.

@lioncash

lioncash Dec 29, 2017

Member

You likely want to prefix this with CITRA_ so it doesn't potentially clash with an external in the future.

What is the benefit of making this toggleable, by the way?

This comment has been minimized.

@chris062689

chris062689 Dec 29, 2017

Member

We want to specifically enable this flag for nightly, but not make it the default.

The reasoning behind this is this feature shouldn't be used by canary or any unofficial build.
This is functionality specifically for nightly.

This comment has been minimized.

@BreadFish64

BreadFish64 Dec 29, 2017

Contributor

@lioncash I just followed the pattern set by ENABLE_WEB_SERVICE but I can certainly change it

button(NextButton)->setEnabled(true);
}
CompatDB::~CompatDB() {}

This comment has been minimized.

@lioncash

lioncash Dec 29, 2017

Member

The expectation is a constructor is followed by a destructor if it's necessary to specify one, so this should be moved up. This can also be defaulted like so:

CompatDB::~CompatDB() = default;
@@ -82,6 +82,16 @@ set(UIS
main.ui
)
if (ENABLE_COMPATIBILITY_REPORTING)
set(citra-qt ${citra-qt}

This comment has been minimized.

@lioncash

lioncash Dec 29, 2017

Member

This can be replaced with generator expressions in the respective lists.

I'd also be surprised if this worked as intended on a clean source tree and reporting is enabled. citra-qt is a target, not a list that can be set.

e.g.

set(UIS
     configuration/configure.ui
     configuration/configure_audio.ui
     configuration/configure_debug.ui
     configuration/configure_general.ui
     configuration/configure_graphics.ui
     configuration/configure_input.ui
     configuration/configure_system.ui
     configuration/configure_web.ui
     debugger/registers.ui
     aboutdialog.ui
     hotkeys.ui
     main.ui
     $<$<BOOL:${ENABLE_COMPATIBILITY_REPORTING}>:compatdb.ui>
)
if (compatibility->checkedId() == -1) {
button(NextButton)->setEnabled(false);
break;
}

This comment has been minimized.

@lioncash

lioncash Dec 29, 2017

Member

You likely want a break here as well, otherwise this falls through to the case below when it's not -1. If the fallthrough is intended, it should probably be restructured so that it isn't (i.e. put the behavior in a function and call it in both cases explicitly).

This comment has been minimized.

@BreadFish64

BreadFish64 Dec 29, 2017

Contributor

Woops, just put the break on the wrong side of the brackets, totally unintentional

@BreadFish64 BreadFish64 force-pushed the BreadFish64:CompatibiltyReporting branch 11 times, most recently from 32d267e to 36d0aa6 Dec 29, 2017

@BreadFish64 BreadFish64 force-pushed the BreadFish64:CompatibiltyReporting branch from 569ca6e to e768a92 Jan 2, 2018

@j-selby

j-selby approved these changes Jan 2, 2018

Awesome!

@@ -73,6 +73,7 @@
<addaction name="action_Pause"/>
<addaction name="action_Stop"/>
<addaction name="separator"/>
<addaction name="action_Report_Compatibility"/>

This comment has been minimized.

@j-selby

j-selby Jan 2, 2018

Contributor

Not sure about the placement in the Emulation menu.

This comment has been minimized.

@chris062689

chris062689 Jan 2, 2018

Member

I figure it's a good placement only because that's close to the Start / Stop options, where else would be a better place to put it?

This comment has been minimized.

@BreadFish64

BreadFish64 Jan 2, 2018

Contributor

It's a place where users will see it, and since it's disabled when emulation isn't running, I think it makes the most sense

This comment has been minimized.

@j-selby

j-selby Jan 2, 2018

Contributor

I would personally think that it would be better in the Help menu, where we can also have, say, direct links to the compat db, and where the other web service stuff is currently located. Just my 2c though.

@@ -235,6 +236,9 @@ endif()
if (ENABLE_WEB_SERVICE)
add_definitions(-DENABLE_WEB_SERVICE)
endif()
if (CITRA_ENABLE_COMPATIBILITY_REPORTING)
add_definitions(-DCITRA_ENABLE_COMPATIBILITY_REPORTING)

This comment has been minimized.

@j-selby

j-selby Jan 2, 2018

Contributor

Does this need to be prefixed with CITRA_? See the above statement on the web service module for consistency.

This comment has been minimized.

@chris062689

chris062689 Jan 2, 2018

Member

Lioncash commented up above that we should be prefixing CITRA_ to these.

This comment has been minimized.

@j-selby

j-selby Jan 2, 2018

Contributor

Sure!

@LogPod

This comment has been minimized.

LogPod commented Jan 4, 2018

I would personally like the Report Compatibility option below the Configure option because i pressed it accidentally so many times.

@FearlessTobi

This comment has been minimized.

Contributor

FearlessTobi commented Jan 4, 2018

Imo the compatibility report option doesn't fit into the Emulation submenu. The other entries all have to do with the Emulation in Citra itself. I would also prefer it in the Help Tab or at least below the configure button. Just my opinion.

@neobrain

This comment has been minimized.

Member

neobrain commented Jan 6, 2018

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

2018-01-06T17:56:17Z: e203c10...BreadFish64:68959823e92260cfc095d27a2faebcc65676fc45

@cluezbot

This comment has been minimized.

cluezbot commented Jan 11, 2018

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

2018-01-11T02:39:37Z: e203c10...BreadFish64:f985d277c8b2129ee3a17da37a3ad1b5dcca4eca

LOG_DEBUG(
Frontend,
tr("Compatibility Rating: %1").arg(compatibility->checkedId()).toStdString().c_str());
LOG_DEBUG(Frontend, "Compatibility Rating: %1", compatibility->checkedId());

This comment has been minimized.

@wwylele

wwylele Jan 11, 2018

Member

%1 should be %d for logging system

@cluezbot

This comment has been minimized.

cluezbot commented Jan 11, 2018

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

2018-01-11T16:57:45Z: e203c10...BreadFish64:ca960776015ee75cc3f289a557e5c28c90fa9e60

@cluezbot

This comment has been minimized.

cluezbot commented Jan 17, 2018

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

2018-01-17T19:57:44Z: e203c10...BreadFish64:11d2825070ae57538332b23feca27975ac89836d

@wwylele wwylele added pr:reviewed and removed pr:needs-review labels Jan 18, 2018

@wwylele

This comment has been minimized.

Member

wwylele commented Jan 18, 2018

@chris062689 / @jroweboy would you like to give an additional review?

@jroweboy

I did a quick read through of the changes, and I'm pretty fine with it overall. I'll need to test the code out at some point, but I don't have the time right now to do that. Don't let me forget please <3

@@ -6,7 +6,7 @@ export MACOSX_DEPLOYMENT_TARGET=10.9
export Qt5_DIR=$(brew --prefix)/opt/qt5
mkdir build && cd build
cmake .. -DUSE_SYSTEM_CURL=ON -DCMAKE_OSX_ARCHITECTURES="x86_64;x86_64h" -DCMAKE_BUILD_TYPE=Release
cmake .. -DUSE_SYSTEM_CURL=ON -DCMAKE_OSX_ARCHITECTURES="x86_64;x86_64h" -DCMAKE_BUILD_TYPE=Release -DCITRA_ENABLE_COMPATIBILITY_REPORTING=${ENABLE_COMPATIBILITY_REPORTING:-"OFF"}

This comment has been minimized.

@jroweboy

jroweboy Jan 18, 2018

Member

I see -DCITRA_ENABLE_COMPATIBILITY_REPORTING=${ENABLE_COMPATIBILITY_REPORTING:-"OFF"} but enable compat reporting isn't set for mac anywhere.

This comment has been minimized.

@BreadFish64

BreadFish64 Jan 18, 2018

Contributor

It takes it directly from the CI environment variable and defaults to off if not set

This comment has been minimized.

@jroweboy

jroweboy Jan 18, 2018

Member

who set the CI env var for mac and where?

if (ENABLE_WEB_SERVICE AND NOT CITRA_USE_BUNDLED_CURL AND MINGW)
message(AUTHOR_WARNING "Turning on CITRA_USE_BUNDLED_CURL. Override it only if you know what you are doing.")
SET(CITRA_USE_BUNDLED_CURL ON CACHE BOOL "" FORCE)
endif()
if (CITRA_ENABLE_COMPATIBILITY_REPORTING AND NOT ENABLE_WEB_SERVICE)
message("Compatibility reporting requires web services to be enabled")

This comment has been minimized.

@jroweboy

jroweboy Jan 18, 2018

Member

message(AUTHOR_WARNING "..") instead of default message type

@@ -14,14 +14,14 @@ option(CITRA_USE_BUNDLED_QT "Download bundled Qt binaries" OFF)
option(ENABLE_WEB_SERVICE "Enable web services (telemetry, etc.)" ON)
option(CITRA_USE_BUNDLED_CURL "FOR MINGW ONLY: Download curl configured against winssl instead of openssl" OFF)
if (ENABLE_WEB_SERVICE AND CITRA_USE_BUNDLED_CURL AND WINDOWS AND MSVC)

This comment has been minimized.

@jroweboy

jroweboy Jan 18, 2018

Member

Why was this removed?

This comment has been minimized.

@BreadFish64

BreadFish64 Jan 18, 2018

Contributor

oops

compatibility->addButton(ui->radioButton_IntroMenu, 4);
compatibility->addButton(ui->radioButton_WontBoot, 5);
switch (currentId()) {
case 1:

This comment has been minimized.

@jroweboy

jroweboy Jan 18, 2018

Member

what is 1 and 2 in this switch? What page they are on? Also it should have a default with some error logging in there i think.

This comment has been minimized.

@BreadFish64

BreadFish64 Jan 18, 2018

Contributor

yes, that's the ID of current page

@cluezbot

This comment has been minimized.

cluezbot commented Jan 18, 2018

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

2018-01-18T16:36:35Z: e2a8f15...BreadFish64:c3afd7359219660f9ff7fd633d24fc655bdda1a1

@cluezbot

This comment has been minimized.

cluezbot commented Jan 18, 2018

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

2018-01-18T16:40:26Z: e2a8f15...BreadFish64:d462eacefba24344fc707f14faff212dc80076ed

@cluezbot

This comment has been minimized.

cluezbot commented Jan 19, 2018

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

2018-01-19T03:03:02Z: e2a8f15...BreadFish64:169b07691391e3cfee09a41ac6a239278d08f283

@jroweboy

Some more minor nit picks (should be fast to fix, thanks for your hard work!)

Still missing confirmation that it works in build bot built OSX builds. I don't see anywhere that the env var is set in this PR so I don't see how it could possibly work right now. (Maybe I just missed it though)

@@ -25,3 +25,5 @@ private slots:
void Submit();
void EnableNext();
};
enum CompatDBPage { IntroPage = 0, SelectionPage = 1, FinalPage = 2 };

This comment has been minimized.

@jroweboy

jroweboy Jan 19, 2018

Member

Put this in the cpp file above CompatDB::Submit (no point in exposing it in the header)

@@ -33,19 +33,21 @@ void CompatDB::Submit() {
compatibility->addButton(ui->radioButton_IntroMenu, 4);
compatibility->addButton(ui->radioButton_WontBoot, 5);
switch (currentId()) {

This comment has been minimized.

@jroweboy

jroweboy Jan 19, 2018

Member

switch (static_cast<CompatDBPage>(currentId())) {

@@ -25,3 +25,5 @@ private slots:
void Submit();
void EnableNext();
};
enum CompatDBPage { IntroPage = 0, SelectionPage = 1, FinalPage = 2 };

This comment has been minimized.

@jroweboy

jroweboy Jan 19, 2018

Member

also enum class CompatDBPage { ... };

@@ -25,3 +25,5 @@ private slots:
void Submit();
void EnableNext();
};
enum CompatDBPage { IntroPage = 0, SelectionPage = 1, FinalPage = 2 };

This comment has been minimized.

@jroweboy

jroweboy Jan 19, 2018

Member

nit: the name of the enum already includes Page, so adding it to the end of each item is really redundant <3

@@ -33,19 +33,21 @@ void CompatDB::Submit() {
compatibility->addButton(ui->radioButton_IntroMenu, 4);
compatibility->addButton(ui->radioButton_WontBoot, 5);
switch (currentId()) {
case 1:
case CompatDBPage::SelectionPage:

This comment has been minimized.

@jroweboy

jroweboy Jan 19, 2018

Member

still need a case for CompatDBPage::Intro (which can just break;)

This comment has been minimized.

@BreadFish64

BreadFish64 Jan 19, 2018

Contributor

this is executed after the page id is changed (but before the UI updates), so technically CompatDBPage::Intro should never be encountered as a case, but I can still add it if you want

@cluezbot

This comment has been minimized.

cluezbot commented Jan 20, 2018

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

2018-01-20T15:56:09Z: e2a8f15...BreadFish64:fc1bdc3c626aa088ffa84a9903e134eeddb5dad8

@B3n30

This comment has been minimized.

Contributor

B3n30 commented Jan 24, 2018

@jroweboy, @chris062689 You requested changes on that PR. Are those fixed? Can it get merged? Do you want to give it a final review?

@BreadFish64

This comment has been minimized.

Contributor

BreadFish64 commented Jan 26, 2018

@wwylele wwylele merged commit e53e07b into citra-emu:master Jan 26, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Leo626 Leo626 referenced this pull request Feb 7, 2018

Closed

Progress Report 2018 January #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment