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

refactor: Un-singletonize Paths & Updates #5092

Merged
merged 33 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
0dcd189
Make all uses/storages of Paths const
pajlada Jan 15, 2024
ff87089
store Paths as a unique_ptr instead of a raw ptr
pajlada Jan 15, 2024
8d24a2b
Make Paths accessible from Application
pajlada Jan 15, 2024
eb96bd5
make Paths isPortable const
pajlada Jan 15, 2024
1bf40b7
mark Paths.isPortable as deprecated
pajlada Jan 15, 2024
8928f59
Remove getPaths() usage from Args
pajlada Jan 15, 2024
7c2ec8c
Remove getPaths() usage from NetworkPrivate
pajlada Jan 15, 2024
e85d060
Remove getPaths() usage from GeneralPage
pajlada Jan 15, 2024
e313575
Remove getPaths() usage from PluginsPage
pajlada Jan 15, 2024
53599a6
Remove getPaths() usage from Credentials
pajlada Jan 15, 2024
42def13
Remove getPaths() usage from Irc2
pajlada Jan 15, 2024
601e87f
Remove getPaths() usage from Toasts
pajlada Jan 15, 2024
f524f3b
Remove getPaths() usage from WindowManager
pajlada Jan 15, 2024
199b22d
Remove getPaths() usage from LoggingChannel
pajlada Jan 15, 2024
6ad5c63
Remove getPaths() usage from NetworkTask
pajlada Jan 15, 2024
3f599ea
Remove getPaths() usage from CrashHandler
pajlada Jan 15, 2024
a2e9177
Remove getPaths() usage from LastRunCrashDialog
pajlada Jan 15, 2024
72b3d6a
Remove getPaths() usage from ImageUploader
pajlada Jan 15, 2024
6d0db89
Remove getPaths() usage from Theme
pajlada Jan 15, 2024
4993a77
Remove getPaths() usage from ModerationPage
pajlada Jan 15, 2024
6955771
Remove getPaths() usage from UserDataController
pajlada Jan 15, 2024
b3d9f72
Remove getPaths() usage from PluginController
pajlada Jan 15, 2024
c98d548
Make Updates non-singletonized, removing usage of getPaths()
pajlada Jan 15, 2024
b258187
remove getPaths()
pajlada Jan 15, 2024
b649fe2
Add changelog entry
pajlada Jan 15, 2024
3eb4a14
Fix missing const args for PluginController
pajlada Jan 15, 2024
dac2ef3
const more args
pajlada Jan 16, 2024
98cae26
fix plugin compilation
pajlada Jan 16, 2024
b37b6c4
fix plugins compile
pajlada Jan 16, 2024
596484f
Add comment to the Updates class describing what it does
pajlada Jan 16, 2024
44503d2
fix changelog entry number
pajlada Jan 16, 2024
78c028d
Remove explicit CrashHandler template param
pajlada Jan 16, 2024
d46b553
Don't use getIApp()->getPaths() in Theme
pajlada Jan 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@
- Dev: Channels without any animated elements on screen will skip updates from the GIF timer. (#5042, #5043, #5045)
- Dev: Autogenerate docs/plugin-meta.lua. (#5055)
- Dev: Refactor `NetworkPrivate`. (#5063)
- Dev: Refactor `Paths` & `Updates`, focusing on reducing their singletoniability. (#5093)
- Dev: Removed duplicate scale in settings dialog. (#5069)
- Dev: Fix `NotebookTab` emitting updates for every message. (#5068)
- Dev: Added benchmark for parsing and building recent messages. (#5071)
Expand Down
19 changes: 19 additions & 0 deletions mocks/include/mocks/EmptyApplication.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,26 @@

#include "Application.hpp"
#include "common/Args.hpp"
#include "singletons/Paths.hpp"
#include "singletons/Updates.hpp"

namespace chatterino::mock {

class EmptyApplication : public IApplication
{
public:
EmptyApplication()
: updates_(this->paths_)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems scary. There should be a comment in the Updates constructor saying that it must never check for updates right away.

Copy link
Member Author

Choose a reason for hiding this comment

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

How so? (Also this is the mock app)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd assume that once I initialize the updates, it will eventually check for updates (or some other component will trigger it).

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing new in this PR really, except that we know where Updates is initialized. I have added some documentation to the Updates class in 596484f (#5092)

{
}

virtual ~EmptyApplication() = default;

const Paths &getPaths() override
{
return this->paths_;
}

const Args &getArgs() override
{
return this->args_;
Expand Down Expand Up @@ -168,8 +180,15 @@ class EmptyApplication : public IApplication
return nullptr;
}

Updates &getUpdates() override
{
return this->updates_;
}

private:
Paths paths_;
Args args_;
Updates updates_;
};

} // namespace chatterino::mock
23 changes: 13 additions & 10 deletions src/Application.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -107,18 +107,20 @@ IApplication::IApplication()
// It will create the instances of the major classes, and connect their signals
// to each other

Application::Application(Settings &_settings, Paths &_paths, const Args &_args)
: args_(_args)
Application::Application(Settings &_settings, const Paths &paths,
const Args &_args, Updates &_updates)
: paths_(paths)
, args_(_args)
, themes(&this->emplace<Theme>())
, fonts(&this->emplace<Fonts>())
, emotes(&this->emplace<Emotes>())
, accounts(&this->emplace<AccountController>())
, hotkeys(&this->emplace<HotkeyController>())
, windows(&this->emplace<WindowManager>())
, windows(&this->emplace(new WindowManager(paths)))
, toasts(&this->emplace<Toasts>())
, imageUploader(&this->emplace<ImageUploader>())
, seventvAPI(&this->emplace<SeventvAPI>())
, crashHandler(&this->emplace<CrashHandler>())
, crashHandler(&this->emplace<CrashHandler>(new CrashHandler(paths)))

, commands(&this->emplace<CommandController>())
, notifications(&this->emplace<NotificationController>())
Expand All @@ -127,14 +129,15 @@ Application::Application(Settings &_settings, Paths &_paths, const Args &_args)
, chatterinoBadges(&this->emplace<ChatterinoBadges>())
, ffzBadges(&this->emplace<FfzBadges>())
, seventvBadges(&this->emplace<SeventvBadges>())
, userData(&this->emplace<UserDataController>())
, userData(&this->emplace(new UserDataController(paths)))
, sound(&this->emplace<ISoundController>(makeSoundController(_settings)))
, twitchLiveController(&this->emplace<TwitchLiveController>())
, twitchPubSub(new PubSub(TWITCH_PUBSUB_URL))
, logging(new Logging(_settings))
#ifdef CHATTERINO_HAVE_PLUGINS
, plugins(&this->emplace<PluginController>())
, plugins(&this->emplace(new PluginController(paths)))
#endif
, updates(_updates)
{
Application::instance = this;

Expand All @@ -152,7 +155,7 @@ void Application::fakeDtor()
this->twitchPubSub.reset();
}

void Application::initialize(Settings &settings, Paths &paths)
void Application::initialize(Settings &settings, const Paths &paths)
{
assert(isAppInitialized == false);
isAppInitialized = true;
Expand Down Expand Up @@ -237,8 +240,8 @@ int Application::run(QApplication &qtApp)
}

getSettings()->betaUpdates.connect(
[] {
Updates::instance().checkForUpdates();
[this] {
this->updates.checkForUpdates();
},
false);

Expand Down Expand Up @@ -340,7 +343,7 @@ void Application::save()
}
}

void Application::initNm(Paths &paths)
void Application::initNm(const Paths &paths)
{
(void)paths;

Expand Down
20 changes: 17 additions & 3 deletions src/Application.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class Args;
class TwitchIrcServer;
class ITwitchIrcServer;
class PubSub;
class Updates;

class CommandController;
class AccountController;
Expand Down Expand Up @@ -55,6 +56,7 @@ class IApplication

static IApplication *instance;

virtual const Paths &getPaths() = 0;
virtual const Args &getArgs() = 0;
virtual Theme *getThemes() = 0;
virtual Fonts *getFonts() = 0;
Expand All @@ -78,10 +80,12 @@ class IApplication
virtual ITwitchLiveController *getTwitchLiveController() = 0;
virtual ImageUploader *getImageUploader() = 0;
virtual SeventvAPI *getSeventvAPI() = 0;
virtual Updates &getUpdates() = 0;
};

class Application : public IApplication
{
const Paths &paths_;
const Args &args_;
std::vector<std::unique_ptr<Singleton>> singletons_;
int argc_{};
Expand All @@ -90,7 +94,8 @@ class Application : public IApplication
public:
static Application *instance;

Application(Settings &_settings, Paths &_paths, const Args &_args);
Application(Settings &_settings, const Paths &paths, const Args &_args,
Updates &_updates);
~Application() override;

Application(const Application &) = delete;
Expand All @@ -104,7 +109,7 @@ class Application : public IApplication
*/
void fakeDtor();

void initialize(Settings &settings, Paths &paths);
void initialize(Settings &settings, const Paths &paths);
void load();
void save();

Expand Down Expand Up @@ -143,6 +148,10 @@ class Application : public IApplication
PluginController *const plugins{};
#endif

const Paths &getPaths() override
{
return this->paths_;
}
const Args &getArgs() override
{
return this->args_;
Expand Down Expand Up @@ -214,6 +223,10 @@ class Application : public IApplication
{
return this->seventvAPI;
}
Updates &getUpdates() override
{
return this->updates;
}

pajlada::Signals::NoArgSignal streamerModeChanged;

Expand All @@ -222,7 +235,7 @@ class Application : public IApplication
void initPubSub();
void initBttvLiveUpdates();
void initSeventvEventAPI();
void initNm(Paths &paths);
void initNm(const Paths &paths);

template <typename T,
typename = std::enable_if_t<std::is_base_of<Singleton, T>::value>>
Expand All @@ -242,6 +255,7 @@ class Application : public IApplication
}

NativeMessagingServer nmServer{};
Updates &updates;
};

Application *getApp();
Expand Down
13 changes: 7 additions & 6 deletions src/RunGui.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,9 @@ namespace {
installCustomPalette();
}

void showLastCrashDialog(const Args &args)
void showLastCrashDialog(const Args &args, const Paths &paths)
{
auto *dialog = new LastRunCrashDialog(args);
auto *dialog = new LastRunCrashDialog(args, paths);
// Use exec() over open() to block the app from being loaded
// and to be able to set the safe mode.
dialog->exec();
Expand Down Expand Up @@ -223,7 +223,8 @@ namespace {
}
} // namespace

void runGui(QApplication &a, Paths &paths, Settings &settings, const Args &args)
void runGui(QApplication &a, const Paths &paths, Settings &settings,
const Args &args, Updates &updates)
{
initQt();
initResources();
Expand All @@ -232,7 +233,7 @@ void runGui(QApplication &a, Paths &paths, Settings &settings, const Args &args)
#ifdef Q_OS_WIN
if (args.crashRecovery)
{
showLastCrashDialog(args);
showLastCrashDialog(args, paths);
}
#endif

Expand Down Expand Up @@ -269,9 +270,9 @@ void runGui(QApplication &a, Paths &paths, Settings &settings, const Args &args)
});

chatterino::NetworkManager::init();
chatterino::Updates::instance().checkForUpdates();
updates.checkForUpdates();

Application app(settings, paths, args);
Application app(settings, paths, args, updates);
app.initialize(settings, paths);
app.run(a);
app.save();
Expand Down
5 changes: 3 additions & 2 deletions src/RunGui.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ namespace chatterino {
class Args;
class Paths;
class Settings;
class Updates;

void runGui(QApplication &a, Paths &paths, Settings &settings,
const Args &args);
void runGui(QApplication &a, const Paths &paths, Settings &settings,
const Args &args, Updates &updates);

} // namespace chatterino
13 changes: 6 additions & 7 deletions src/common/Args.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ QStringList extractCommandLine(

namespace chatterino {

Args::Args(const QApplication &app)
Args::Args(const QApplication &app, const Paths &paths)
{
QCommandLineParser parser;
parser.setApplicationDescription("Chatterino 2 Client for Twitch Chat");
Expand Down Expand Up @@ -132,7 +132,7 @@ Args::Args(const QApplication &app)

if (parser.isSet(channelLayout))
{
this->applyCustomChannelLayout(parser.value(channelLayout));
this->applyCustomChannelLayout(parser.value(channelLayout), paths);
}

this->verbose = parser.isSet(verboseOption);
Expand Down Expand Up @@ -175,7 +175,7 @@ QStringList Args::currentArguments() const
return this->currentArguments_;
}

void Args::applyCustomChannelLayout(const QString &argValue)
void Args::applyCustomChannelLayout(const QString &argValue, const Paths &paths)
{
WindowLayout layout;
WindowDescriptor window;
Expand All @@ -187,10 +187,9 @@ void Args::applyCustomChannelLayout(const QString &argValue)
window.type_ = WindowType::Main;

// Load main window layout from config file so we can use the same geometry
const QRect configMainLayout = [] {
const QString windowLayoutFile =
combinePath(getPaths()->settingsDirectory,
WindowManager::WINDOW_LAYOUT_FILENAME);
const QRect configMainLayout = [paths] {
const QString windowLayoutFile = combinePath(
paths.settingsDirectory, WindowManager::WINDOW_LAYOUT_FILENAME);

const WindowLayout configLayout =
WindowLayout::loadFromFile(windowLayoutFile);
Expand Down
6 changes: 4 additions & 2 deletions src/common/Args.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

namespace chatterino {

class Paths;

/// Command line arguments passed to Chatterino.
///
/// All accepted arguments:
Expand All @@ -31,7 +33,7 @@ class Args
{
public:
Args() = default;
Args(const QApplication &app);
Args(const QApplication &app, const Paths &paths);

bool printVersion{};

Expand All @@ -56,7 +58,7 @@ class Args
QStringList currentArguments() const;

private:
void applyCustomChannelLayout(const QString &argValue);
void applyCustomChannelLayout(const QString &argValue, const Paths &paths);

QStringList currentArguments_;
};
Expand Down
6 changes: 4 additions & 2 deletions src/common/Credentials.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "common/Credentials.hpp"

#include "Application.hpp"
#include "debug/AssertInGuiThread.hpp"
#include "singletons/Paths.hpp"
#include "singletons/Settings.hpp"
Expand Down Expand Up @@ -40,7 +41,7 @@ bool useKeyring()
#ifdef NO_QTKEYCHAIN
return false;
#endif
if (getPaths()->isPortable())
if (getIApp()->getPaths().isPortable())
{
return false;
}
Expand All @@ -55,7 +56,8 @@ bool useKeyring()
// Insecure storage:
QString insecurePath()
{
return combinePath(getPaths()->settingsDirectory, "credentials.json");
return combinePath(getIApp()->getPaths().settingsDirectory,
"credentials.json");
}

QJsonDocument loadInsecure()
Expand Down
2 changes: 1 addition & 1 deletion src/common/Singleton.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class Singleton
Singleton(Singleton &&) = delete;
Singleton &operator=(Singleton &&) = delete;

virtual void initialize(Settings &settings, Paths &paths)
virtual void initialize(Settings &settings, const Paths &paths)
{
(void)(settings);
(void)(paths);
Expand Down
4 changes: 3 additions & 1 deletion src/common/network/NetworkPrivate.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "common/network/NetworkPrivate.hpp"

#include "Application.hpp"
#include "common/network/NetworkManager.hpp"
#include "common/network/NetworkResult.hpp"
#include "common/network/NetworkTask.hpp"
Expand Down Expand Up @@ -57,7 +58,8 @@ void loadUncached(std::shared_ptr<NetworkData> &&data)

void loadCached(std::shared_ptr<NetworkData> &&data)
{
QFile cachedFile(getPaths()->cacheDirectory() + "/" + data->getHash());
QFile cachedFile(getIApp()->getPaths().cacheDirectory() + "/" +
data->getHash());

if (!cachedFile.exists() || !cachedFile.open(QIODevice::ReadOnly))
{
Expand Down
Loading
Loading