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 Discord Rich Presence Support #3883

Merged
merged 6 commits into from Aug 20, 2018

Conversation

Projects
None yet
10 participants
@CaptV0rt3x
Member

CaptV0rt3x commented Jun 26, 2018

Discord Rich Presence

Rich Presence allows you to leverage the totally overhauled "Now Playing" section in a Discord user's profile. This implementation, lets users show off when they are playing their favorite games on Citra.

This PR is based off of Dolphin-emu's implementation (here) & takes inspiration from an earlier PR by Selby & Valentin (here).

  • This introduces a new external discord-rpc - A library for interfacing discord with our app.
  • A checkbox in the UI gives users the freedom to turn this on/off.
  • Currently this shows us only what game the user is playing & elapsed time.
  • Currently only the Qt frontend is supported.

To Do :

  • To create discord dev app, and use client ID from Citrabot's account.
  • To test builds on other OS and check for bugs. (Tested on Windows, MacOS, Linux (ubuntu)).
  • To modify CMakeLists, to properly build when USE_DISCORD_PRESENCE = OFF.
    - [ ] To add support for SDL frontend. (Is it necessary ?) Not doing it as of now.
  • To make namespace DiscordRPC into a class.
  • To do a NULL implementation like Telemetry.

Images

pic_1
The above image has been edited, to show how the option blacks out, when emulation is running. This is to workaround a known discord client bug, which shows presence even after closing app.

pic 2
121

This PR only contains the initial implementation for Discord Rich Presence.
Further work can be done to add Join Lobby feature for netplay. (Out of PR's scope)


This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented Jun 26, 2018

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

2018-06-26T15:31:22Z: 2625407...CaptV0rt3x:59ca6d95222f27d50551adbc0987b1bc47b23544

void Init();
void Shutdown();
void Update();
// void SetDiscordPresenceEnabled(bool enabled);

This comment has been minimized.

@wwylele

wwylele Jun 26, 2018

Member

what is this?

@@ -9,6 +9,9 @@
#include "core/hle/service/hid/hid.h"
#include "core/hle/service/ir/ir.h"
#include "core/settings.h"
#ifdef USE_DISCORD_PRESENCE
#include "citra_qt/discord.h"

This comment has been minimized.

@wwylele

wwylele Jun 26, 2018

Member

What is this for? Plus this is a circular dependency

// Discord rpc
#ifdef USE_DISCORD_PRESENCE
bool enable_discord_presence;

This comment has been minimized.

@wwylele

wwylele Jun 26, 2018

Member

Why is this duplicated here and in UISettings? I think this should be in UISettings only.

namespace DiscordRPC {
void Init();
void Shutdown();
void Update();

This comment has been minimized.

@wwylele

wwylele Jun 26, 2018

Member

Please merge Init and Update into one function if they are intended to use together as what you are doing now

@@ -184,6 +184,13 @@ void Config::ReadValues() {
Settings::values.citra_token = qt_config->value("citra_token").toString().toStdString();
qt_config->endGroup();
#ifdef USE_DISCORD_PRESENCE

This comment has been minimized.

@jroweboy

jroweboy Jun 26, 2018

Member

why does discord need its own settings group for just one setting?

This comment has been minimized.

@CaptV0rt3x

CaptV0rt3x Jun 26, 2018

Member

I tried to follow the existing code style. @jroweboy under which existing grp should I move it to?

@@ -184,6 +184,13 @@ void Config::ReadValues() {
Settings::values.citra_token = qt_config->value("citra_token").toString().toStdString();
qt_config->endGroup();
#ifdef USE_DISCORD_PRESENCE

This comment has been minimized.

@jroweboy

jroweboy Jun 26, 2018

Member

The setting should always be there whether or not it was compiled with discord integration.

@@ -17,6 +21,8 @@ ConfigureWeb::ConfigureWeb(QWidget* parent)
connect(ui->button_verify_login, &QPushButton::clicked, this, &ConfigureWeb::VerifyLogin);
connect(this, &ConfigureWeb::LoginVerified, this, &ConfigureWeb::OnLoginVerified);
ui->toggle_discordrpc->setEnabled(!Core::System::GetInstance().IsPoweredOn());

This comment has been minimized.

@jroweboy

jroweboy Jun 26, 2018

Member

why are we disabling the button when the game is running?

This comment has been minimized.

@CaptV0rt3x

CaptV0rt3x Jun 26, 2018

Member

This is to workaround a known discord client bug, which shows presence even after closing app. If you change this while a game is running, discord would still continue to show presence until you refresh discord or restart it.

This comment has been minimized.

@j-selby

j-selby Jun 28, 2018

Contributor

Might want to add a comment about this in the source itself

@@ -17,6 +21,8 @@ ConfigureWeb::ConfigureWeb(QWidget* parent)
connect(ui->button_verify_login, &QPushButton::clicked, this, &ConfigureWeb::VerifyLogin);
connect(this, &ConfigureWeb::LoginVerified, this, &ConfigureWeb::OnLoginVerified);
ui->toggle_discordrpc->setEnabled(!Core::System::GetInstance().IsPoweredOn());

This comment has been minimized.

@jroweboy

jroweboy Jun 26, 2018

Member

instead of showing/hiding the checkbox, can we just disable it if rich presence wasn't compiled in?

s64 StartTime;
void Init() {

This comment has been minimized.

@jroweboy

jroweboy Jun 26, 2018

Member

Prefer instead to do a null impl pattern like we did with Telemetry. When telemetry isn't compiled in, it uses the null web backend instead of the regular web backend, which keeps us from having to do #if DISCORD_ENABLED checks across the application.

#include <discord_rpc.h>
#endif
namespace DiscordRPC {

This comment has been minimized.

@jroweboy

jroweboy Jun 26, 2018

Member

This should really be a class instead of a few static functions in a namespace

#include <discord_rpc.h>
#endif
namespace DiscordRPC {

This comment has been minimized.

@jroweboy

jroweboy Jun 26, 2018

Member

None of this is specific to citra_qt. I don't see why we can't also add this to citra_sdl as well.

Core::System::GetInstance().GetAppLoader().ReadTitle(title);
DiscordRichPresence presence = {};
presence.largeImageKey = "citra_logo";
presence.largeImageText = "Citra is an emulator for the Nintendo 3DS";

This comment has been minimized.

@jroweboy

jroweboy Jun 26, 2018

Member

Should this "Citra is... 3DS" string be translated?

This comment has been minimized.

@CaptV0rt3x

CaptV0rt3x Jun 26, 2018

Member

As confirmed from discord-rpc (here), currently localization isn't supported.

< Playing Citra > would get translated to user's discord language (as it is rendered by discord) , but the strings we give to it remain the same. However, They support UTF-8 characters, so we can send localized strings if we wanted to.

DiscordRichPresence presence = {};
presence.largeImageKey = "citra_logo";
presence.largeImageText = "Citra is an emulator for the Nintendo 3DS";
presence.state = title.empty() ? "Not in-game" : title.c_str();

This comment has been minimized.

@jroweboy

jroweboy Jun 26, 2018

Member

Ditto with the other strings.

if (!Settings::values.enable_discord_presence)
return;
StartTime = time(NULL);

This comment has been minimized.

@jroweboy

jroweboy Jun 26, 2018

Member

use chrono instead of time

@@ -20,6 +20,8 @@ option(ENABLE_WEB_SERVICE "Enable web services (telemetry, etc.)" ON)
option(ENABLE_CUBEB "Enables the cubeb audio backend" ON)
option(USE_DISCORD_PRESENCE "Enables Discord Rich Presence, shows the current game on Discord" ON)

This comment has been minimized.

@jroweboy

jroweboy Jun 26, 2018

Member

Turn this off by default and have the build bots explicitly turn it on. I'd prefer people to not have to see what game I'm testing while developing

presence.largeImageKey = "citra_logo";
presence.largeImageText = "Citra is an emulator for the Nintendo 3DS";
presence.state = title.empty() ? "Not in-game" : title.c_str();
presence.details = "Playing Game :";

This comment has been minimized.

@BynariStar

BynariStar Jun 26, 2018

Contributor

Should probably remove the space before the colon

@cluezbot

This comment has been minimized.

cluezbot commented Jun 26, 2018

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

2018-06-26T20:25:43Z: 2625407...CaptV0rt3x:f1fa4c7067ce2790dbe4df18a257547eb3b68035

@CaptV0rt3x CaptV0rt3x force-pushed the CaptV0rt3x:DiscordRPC branch from f1fa4c7 to 59ca6d9 Jun 26, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jun 26, 2018

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

2018-06-26T20:55:12Z: 5e641d1...CaptV0rt3x:59ca6d95222f27d50551adbc0987b1bc47b23544

@cluezbot

This comment has been minimized.

cluezbot commented Jun 27, 2018

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

2018-06-27T05:57:24Z: 5e641d1...CaptV0rt3x:069acfbe7947426223e5ea4a56b80187b83f366b

@cluezbot

This comment has been minimized.

cluezbot commented Jun 27, 2018

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

2018-06-27T06:09:01Z: 5e641d1...CaptV0rt3x:ff64e6a118a09b180a2d717d72c9a2b6f06542de

@CaptV0rt3x CaptV0rt3x force-pushed the CaptV0rt3x:DiscordRPC branch from 069acfb to ff64e6a Jun 27, 2018

@CaptV0rt3x

This comment has been minimized.

Member

CaptV0rt3x commented Jun 27, 2018

Messed up in git. Will rebase off of master and create new PR.

@CaptV0rt3x CaptV0rt3x closed this Jun 27, 2018

@CaptV0rt3x CaptV0rt3x deleted the CaptV0rt3x:DiscordRPC branch Jun 27, 2018

@CaptV0rt3x CaptV0rt3x referenced this pull request Jun 27, 2018

Closed

Add Discord Rich Presence Support #3886

0 of 6 tasks complete

@jroweboy jroweboy reopened this Jun 27, 2018

@jroweboy jroweboy force-pushed the CaptV0rt3x:DiscordRPC branch from ff64e6a to 921f184 Jun 27, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jun 27, 2018

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

2018-06-27T15:09:39Z: 4387510...CaptV0rt3x:921f1843fcee741867708e7b4176a4ac1cff6c78

@cluezbot

This comment has been minimized.

cluezbot commented Jun 27, 2018

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

2018-06-27T19:12:59Z: 4387510...CaptV0rt3x:642c405aa013fc38826e755bdc0842e5e09715c5

@lioncash

Left some basic comments, didn't really point out anything related to organization of the namespace since its planned to be changed into a class

@@ -194,6 +194,14 @@ target_link_libraries(citra-qt PRIVATE audio_core common core input_common netwo
target_link_libraries(citra-qt PRIVATE Boost::boost glad nihstro-headers Qt5::OpenGL Qt5::Widgets Qt5::Multimedia)
target_link_libraries(citra-qt PRIVATE ${PLATFORM_LIBRARIES} Threads::Threads)
if (USE_DISCORD_PRESENCE)
add_library(discord STATIC
discord.cpp

This comment has been minimized.

@lioncash

lioncash Jun 28, 2018

Member

This is using tabs for indentation when spaces should be used instead

.gitmodules Outdated
@@ -34,3 +34,6 @@
[submodule "cubeb"]
path = externals/cubeb
url = https://github.com/kinetiknz/cubeb.git
[submodule "discord-rpc"]
path = externals/discord-rpc

This comment has been minimized.

@lioncash

lioncash Jun 28, 2018

Member

This is using tabs for indentation when spaced should be used

@@ -204,3 +212,7 @@ if (MSVC)
copy_citra_Qt5_deps(citra-qt)
copy_citra_SDL_deps(citra-qt)
endif()
if (USE_DISCORD_PRESENCE)
target_compile_definitions(citra-qt PRIVATE -DUSE_DISCORD_PRESENCE)

This comment has been minimized.

@lioncash

lioncash Jun 28, 2018

Member

Indentation doesn't match the rest of the file

discord.cpp
discord.h
)
target_link_libraries(citra-qt PUBLIC discord PRIVATE discord-rpc)

This comment has been minimized.

@lioncash

lioncash Jun 28, 2018

Member

The dependencies can both be private

@@ -161,6 +161,7 @@ struct Values {
std::string announce_multiplayer_room_endpoint_url;
std::string citra_username;
std::string citra_token;

This comment has been minimized.

@lioncash

lioncash Jun 28, 2018

Member

Unnecessary newline

int64_t StartTime;
int GetTime() {

This comment has been minimized.

@lioncash

lioncash Jun 28, 2018

Member

Why is this returning an int of the actual type it's being stored to is an int64_t? This truncates the value.

namespace DiscordRPC {
int64_t StartTime;

This comment has been minimized.

@lioncash

lioncash Jun 28, 2018

Member

We generally use our typedefs for these within common_types.h. In this case it would be s64

@zhaowenlan1779

I'm wondering if the Ask to Join can be implemented. It requires approval from discord, I can see.

@@ -46,7 +46,7 @@ before_build:
# redirect stderr and change the exit code to prevent powershell from cancelling the build if cmake prints a warning
cmd /C 'cmake -G "Visual Studio 15 2017 Win64" -DCITRA_USE_BUNDLED_QT=1 -DCITRA_USE_BUNDLED_SDL2=1 -DCITRA_ENABLE_COMPATIBILITY_REPORTING=${COMPAT} -DENABLE_COMPATIBILITY_LIST_DOWNLOAD=ON .. 2>&1 && exit 0'
} else {
C:\msys64\usr\bin\bash.exe -lc "cmake -G 'MSYS Makefiles' -DCMAKE_BUILD_TYPE=Release -DENABLE_QT_TRANSLATION=ON -DCITRA_ENABLE_COMPATIBILITY_REPORTING=${COMPAT} -DENABLE_COMPATIBILITY_LIST_DOWNLOAD=ON .. 2>&1"
C:\msys64\usr\bin\bash.exe -lc "cmake -G 'MSYS Makefiles' -DCMAKE_BUILD_TYPE=Release -DENABLE_QT_TRANSLATION=ON -DCITRA_ENABLE_COMPATIBILITY_REPORTING=${COMPAT} -DENABLE_COMPATIBILITY_LIST_DOWNLOAD=ON -DUSE_DISCORD_PRESESNCE=ON .. 2>&1"

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 28, 2018

Member

Typo

@@ -46,7 +46,7 @@ before_build:
# redirect stderr and change the exit code to prevent powershell from cancelling the build if cmake prints a warning
cmd /C 'cmake -G "Visual Studio 15 2017 Win64" -DCITRA_USE_BUNDLED_QT=1 -DCITRA_USE_BUNDLED_SDL2=1 -DCITRA_ENABLE_COMPATIBILITY_REPORTING=${COMPAT} -DENABLE_COMPATIBILITY_LIST_DOWNLOAD=ON .. 2>&1 && exit 0'

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 28, 2018

Member

Should add compile flag too.

Show resolved Hide resolved externals/CMakeLists.txt
add_library(discord STATIC
discord.cpp
discord.h
)

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 28, 2018

Member
  1. indention
  2. This is not going to compile. You are compiling discord as a separate library. It means discord is not linked to Qt.
    But you included ui_settings.h in discord.h, and it won't find Qt headers there.
    Fix:
if (USE_DISCORD_PRESENCE)
  target_sources(citra-qt PUBLIC
      discord.cpp
      discord.h
  )
  target_link_libraries(citra-qt PRIVATE discord-rpc)
endif()

(indention may not be right, but that's basically how this should look)

This comment has been minimized.

@CaptV0rt3x

CaptV0rt3x Jun 28, 2018

Member

Thank you for helping me fix the builds.

Show resolved Hide resolved src/citra_qt/configuration/configure_web.ui
// Refer to the license.txt file included.
#ifdef USE_DISCORD_PRESENCE
#include "citra_qt/discord.h"
#endif

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 28, 2018

Member

Unnecessary condition as this file will only be compiled with USE_DISCORD_PRESENCE

@@ -10,6 +10,7 @@
#include <QMetaType>
#include <QString>
#include <QStringList>
#include "core/core.h"

This comment has been minimized.

@zhaowenlan1779

This comment has been minimized.

@CaptV0rt3x

CaptV0rt3x Jun 28, 2018

Member

redundant code. Will be removed

}
void GMainWindow::ShutdownGame() {
#ifdef USE_DISCORD_PRESENCE
DiscordRPC::Pause();

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 28, 2018

Member

Is this correct?
If I understand correctly, this(Pause function) removes all presence data, but shouldn't you display Not in-game?
I think this should be replaced with Update, and maybe move it down a bit, after the emulation has stopped.

This comment has been minimized.

@CaptV0rt3x

CaptV0rt3x Jun 28, 2018

Member

Pause function only clears the presence. However update doesn't clear the existing presence, it just updates it. ShutdownGame just Stops the game (its the stop option from UI). So, we want to clear the presence when a game is stopped and when a new game is launched, then we update the presence.

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 29, 2018

Member

Um.
If you clear the presence, Discord will appear you are not playing any game, right?
Then what's that "Not in game" text needed? (in discord.cpp)

This comment has been minimized.

@CaptV0rt3x

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 29, 2018

Member

This line can be removed as you are Updating later.

This comment has been minimized.

@CaptV0rt3x

CaptV0rt3x Jun 30, 2018

Member

It is necessary for it to be there. When a game is stopped, we need to clear the presence before further updating the information. Or the info just won't properly relay to discord. Hence we are clearing presence upon closing and then after emu_thread is deleted , we update the presence.

presence.largeImageKey = "citra_logo";
presence.largeImageText = "Citra is an emulator for the Nintendo 3DS";
presence.state = title.empty() ? "Not in-game" : title.c_str();
presence.details = "Currently in game";

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 28, 2018

Member

This is not like detail.
Perhaps integrating with Multiplayer Rooms?
Not in a room
In Room: XXXXXXXX (3/16)

If you actually do so you should switch state and details

state   | char* | the user's current party status | "Looking to Play", "Playing Solo", "In a Group"
details | char* | what the player is currently doing | "Competitive - Captain's Mode", "In Queue", "Unranked PvP"

According to Discord developer doc

This comment has been minimized.

@CaptV0rt3x

CaptV0rt3x Jun 28, 2018

Member

This will be changed, if and when we implement join room features. As you can see here, default is what we implement now. https://discordapp.com/assets/43bef54c8aee2bc0fd1c717d5f8ae28a.png

@cluezbot

This comment has been minimized.

cluezbot commented Jun 28, 2018

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

2018-06-28T17:48:37Z: 4387510...CaptV0rt3x:dabce922b4b8d1dd083099152881e951c7874876

)
target_link_libraries(citra-qt PUBLIC discord PRIVATE discord-rpc)
)
target_link_libraries(citra-qt PRIVATE discord-rpc)

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 29, 2018

Member

indention is still incorrect

#include "citra_qt/ui_settings.h"
#include "common/common_types.h"

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 29, 2018

Member

Unnecessary include - You already included it in discord.h

@@ -1037,7 +1037,7 @@ void GMainWindow::OnStartGame() {
ui.action_Report_Compatibility->setEnabled(true);
#ifdef USE_DISCORD_PRESENCE
DiscordRPC::Update();
DiscordRPC::Pause();

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 29, 2018

Member

Why are you Pauseing when game is started?

@cluezbot

This comment has been minimized.

cluezbot commented Jun 29, 2018

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

2018-06-29T06:31:22Z: 4387510...CaptV0rt3x:03bd98714a5f242b6c5068760f40b07921331353

@cluezbot

This comment has been minimized.

cluezbot commented Jun 29, 2018

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

2018-06-29T21:25:34Z: 4387510...CaptV0rt3x:da60ffde75574fcb95e2a60c9568e9423a57176b

@@ -16,10 +16,12 @@ option(ENABLE_QT "Enable the Qt frontend" ON)
option(ENABLE_QT_TRANSLATION "Enable translations for the Qt frontend" OFF)
CMAKE_DEPENDENT_OPTION(CITRA_USE_BUNDLED_QT "Download bundled Qt binaries" ON "ENABLE_QT;MSVC" OFF)
option(ENABLE_WEB_SERVICE "Enable web services (telemetry, etc.)" ON)
option(ENABLE_WEB_SERVICE "Enable web services (telemetry, etc.)" OFF)

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jul 1, 2018

Member

Why?
If you do this, you should add build option -DENABLE_WEB_SERVICE=ON as well.

@cluezbot

This comment has been minimized.

cluezbot commented Jul 14, 2018

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

2018-07-14T10:35:24Z: 4387510...CaptV0rt3x:99d67fa740c056b4e7fe44a6155e3bff7990900d

@CaptV0rt3x CaptV0rt3x force-pushed the CaptV0rt3x:DiscordRPC branch from 99d67fa to 867f01a Jul 14, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jul 14, 2018

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

2018-07-14T11:24:40Z: 3799b16...CaptV0rt3x:867f01a88eb48bb9ec3719699e7dd9b7ef5c6a64

@cluezbot

This comment has been minimized.

cluezbot commented Aug 12, 2018

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

2018-08-12T01:12:22Z: c18a789...CaptV0rt3x:f0d0cf64c812b39db5e94ffd50b8017b53356966

@zhaowenlan1779

Mostly LGTM, but there's a critical point that make it not work..

Discord_Shutdown();
}
void Pause() {

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 12, 2018

Member

DiscordImpl::Pause

Discord_ClearPresence();
}
void Update() {

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 12, 2018

Member

DiscordImpl::Update

~DiscordImpl();
void Pause() override {}
void Update() override {}

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 12, 2018

Member

remove the braces. You should not define them here (and you defined them as empty functions...)

void Pause() override;
void Update() override;
@cluezbot

This comment has been minimized.

cluezbot commented Aug 12, 2018

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

2018-08-12T07:40:46Z: c18a789...CaptV0rt3x:4bc93e21a081280d010604e5f0adef1fbce4daff

@cluezbot

This comment has been minimized.

cluezbot commented Aug 12, 2018

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

2018-08-12T08:08:53Z: c18a789...CaptV0rt3x:3657b6cf464774a26dbfaf0a5e747be06fa8b51d

@CaptV0rt3x CaptV0rt3x force-pushed the CaptV0rt3x:DiscordRPC branch from 3657b6c to 5162e00 Aug 12, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Aug 12, 2018

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

2018-08-12T08:15:43Z: c18a789...CaptV0rt3x:5162e002f44070ba024c3a20c63e2f13dd439fb1

@zhaowenlan1779

LGTM pending minor nits

Show resolved Hide resolved externals/CMakeLists.txt
#pragma once
#include <memory>
#include "common/common_types.h"

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 12, 2018

Member

unused includes
("common/common_types.h" needs to be moved to discord_impl.cpp)

s64 StartTime = std::chrono::duration_cast<std::chrono::seconds>(
std::chrono::system_clock::now().time_since_epoch())
.count();

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 12, 2018

Member

<chrono> should be included.

void DiscordImpl::Update() {
if (UISettings::values.enable_discord_presence) {

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 12, 2018

Member

this newline does not look very useful.

@@ -9,6 +9,7 @@
#include <QMainWindow>
#include <QTimer>
#include <QTranslator>
#include "citra_qt/discord.h"

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 12, 2018

Member

can be moved to main.cpp

@@ -17,6 +18,7 @@
class AboutDialog;
class Config;
class ClickableLabel;
class DiscordInterface;

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 12, 2018

Member

this is not the correct forward declaration

namespace DiscordRPC {
    class DiscordInterface;
}
@cluezbot

This comment has been minimized.

cluezbot commented Aug 12, 2018

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

2018-08-12T09:19:23Z: c18a789...CaptV0rt3x:ce6bb1a782455378d8649377d2c6b0199d6f2272

@zhaowenlan1779

LGTM

@jroweboy

Getting closer to completion :) thanks for the hard work!

@@ -207,6 +207,10 @@ void Config::ReadValues() {
qt_config->beginGroup("UI");
UISettings::values.theme = ReadSetting("theme", UISettings::themes[0].second).toString();
#ifdef USE_DISCORD_PRESENCE

This comment has been minimized.

@jroweboy

jroweboy Aug 13, 2018

Member

Remove this check. It doesn't hurt to write it to the settings with the null impl

namespace DiscordRPC {
DiscordImpl::DiscordImpl() {
if (UISettings::values.enable_discord_presence) {

This comment has been minimized.

@jroweboy

jroweboy Aug 13, 2018

Member

shouldn't be checking this in here. instead you should only create the object if its enabled

}
DiscordImpl::~DiscordImpl() {
if (!UISettings::values.enable_discord_presence) {

This comment has been minimized.

@jroweboy

jroweboy Aug 13, 2018

Member

don't check this in here

}
void DiscordImpl::Pause() {
if (!UISettings::values.enable_discord_presence) {

This comment has been minimized.

@jroweboy

jroweboy Aug 13, 2018

Member

don't check this here as well

}
void DiscordImpl::Update() {
if (UISettings::values.enable_discord_presence) {

This comment has been minimized.

@jroweboy

jroweboy Aug 13, 2018

Member

and don't check this here as well.

DiscordImpl::DiscordImpl() {
if (UISettings::values.enable_discord_presence) {
s64 StartTime = std::chrono::duration_cast<std::chrono::seconds>(

This comment has been minimized.

@jroweboy

jroweboy Aug 13, 2018

Member

this variable is unused... was it meant to be a class variable?

void DiscordImpl::Update() {
if (UISettings::values.enable_discord_presence) {
s64 StartTime = std::chrono::duration_cast<std::chrono::seconds>(

This comment has been minimized.

@jroweboy

jroweboy Aug 13, 2018

Member

style: start_time

also why is it called start time? every time update is called it changes the start time. is that normal? i'm thinking you meant to make start time a member variable, and only update it when the game starts right? (i'm guessing this is used to say how long they've been in game for?)

This comment has been minimized.

@CaptV0rt3x

CaptV0rt3x Aug 13, 2018

Member

this just stores system_clock 's time point of "when the game started" as seconds since the epoch. by sending this in presence, discord will start counting the time elapsed.

presence.largeImageText = "Citra is an emulator for the Nintendo 3DS";
if (Core::System::GetInstance().IsPoweredOn()) {
presence.state = title.c_str();
presence.details = "Currently in game";

This comment has been minimized.

@jroweboy

jroweboy Aug 13, 2018

Member

this one says "in game" and the other says "in-game" pick on and be consistent

presence.startTimestamp = StartTime;
Discord_UpdatePresence(&presence);
} else {
Discord_ClearPresence();

This comment has been minimized.

@jroweboy

jroweboy Aug 13, 2018

Member

since this class will only be used when discord is enabled, we shouldn't need this else here either

Show resolved Hide resolved src/citra_qt/discord_impl.cpp

@CaptV0rt3x CaptV0rt3x force-pushed the CaptV0rt3x:DiscordRPC branch from a154f95 to b8bd699 Aug 14, 2018

CaptV0rt3x added some commits Jun 27, 2018

Initial Discord RPC support
Build with Discord Presence ON

Fix RPC detection

Fix Time elapsed on pause; will now continue to count.
Fix CI builds with compile flag
Addressed reviews

Fix silly mistakes

Fix 'Not in-game' display

class instead of namespace

Fix

Revamped

remove redundant code

Using Pimpl pattern

@CaptV0rt3x CaptV0rt3x force-pushed the CaptV0rt3x:DiscordRPC branch 2 times, most recently from ba23864 to 52ed249 Aug 14, 2018

Show resolved Hide resolved src/citra_qt/configuration/configure_web.cpp
@@ -5,6 +5,8 @@
#include <QIcon>
#include <QMessageBox>
#include "citra_qt/configuration/configure_web.h"
#include "citra_qt/ui_settings.h"
#include "core/core.h"

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 14, 2018

Member

unused include (core/core.h)

@@ -1184,11 +1197,14 @@ void GMainWindow::OnConfigure() {
connect(&configureDialog, &ConfigureDialog::languageChanged, this,
&GMainWindow::OnLanguageChanged);
auto old_theme = UISettings::values.theme;
auto old_discord_presence = UISettings::values.enable_discord_presence;

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 14, 2018

Member

const bool
make it auto does not look good..

@@ -61,6 +64,7 @@ class GMainWindow : public QMainWindow {
~GMainWindow();
GameList* game_list;
std::unique_ptr<DiscordRPC::DiscordInterface> discord;

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 14, 2018

Member

I think the meaning of the variable would be more explicit if you name it discord_rpc.

@@ -108,6 +112,7 @@ class GMainWindow : public QMainWindow {
void ShowUpdatePrompt();
void ShowNoUpdatePrompt();
void CheckForUpdates();
void SetDiscordEnabled(bool rpc);

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 14, 2018

Member

The parameter name can be improved.

std::string title;
if (Core::System::GetInstance().IsPoweredOn())
Core::System::GetInstance().GetAppLoader().ReadTitle(title);
DiscordRichPresence presence = {};

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 14, 2018

Member

DiscordRichPresence presence{}

@CaptV0rt3x CaptV0rt3x force-pushed the CaptV0rt3x:DiscordRPC branch from 17d3830 to cb1ad5a Aug 14, 2018

@@ -17,6 +18,9 @@ ConfigureWeb::ConfigureWeb(QWidget* parent)
connect(ui->button_verify_login, &QPushButton::clicked, this, &ConfigureWeb::VerifyLogin);
connect(this, &ConfigureWeb::LoginVerified, this, &ConfigureWeb::OnLoginVerified);
#ifndef USE_DISCORD_PRESENCE
ui->groupBox_2->setVisible(false);

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Aug 14, 2018

Member

huh, it's better not to use the default names when you want to use them in the code.
name it discordrpc_groupBox or whatever.

This comment has been minimized.

@CaptV0rt3x

CaptV0rt3x Aug 14, 2018

Member

renamed to discord_group

@zhaowenlan1779

LGTM otherwise

@CaptV0rt3x CaptV0rt3x force-pushed the CaptV0rt3x:DiscordRPC branch from 911bd94 to f5f898c Aug 14, 2018

@B3n30 B3n30 added the canary-merge label Aug 14, 2018

@FearlessTobi

This comment has been minimized.

Contributor

FearlessTobi commented Aug 17, 2018

For the record, this PR fixes #3547.

@B3n30 B3n30 merged commit 6cb9a45 into citra-emu:master Aug 20, 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