Fetch and show pronouns for chatters. #5442
Conversation
|
Some comments before I make a proper review: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| namespace chatterino { | ||
|
|
||
| class IPronounsApi { |
There was a problem hiding this comment.
warning: destructor of 'IPronounsApi' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
class IPronounsApi {
^Additional context
src/providers/pronouns/IPronounsApi.hpp:12: make it public and virtual
class IPronounsApi {
^| using RequestT = PronounsApiRequest<ResultT>; | ||
|
|
||
| // Fetch pronouns from the API. | ||
| virtual void fetch(std::vector<PronounUser> users, RequestT::CallbackT &&onDone) {}; |
There was a problem hiding this comment.
warning: rvalue reference parameter 'onDone' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved]
virtual void fetch(std::vector<PronounUser> users, RequestT::CallbackT &&onDone) {};
^| using RequestT = PronounsApiRequest<ResultT>; | ||
|
|
||
| // Fetch pronouns from the API. | ||
| virtual void fetch(std::vector<PronounUser> users, RequestT::CallbackT &&onDone) {}; |
There was a problem hiding this comment.
warning: the parameter 'users' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
virtual void fetch(std::vector<PronounUser> users, RequestT::CallbackT &&onDone) {};
^|
|
||
| namespace chatterino { | ||
|
|
||
| PronounUser::PronounUser(): id{}, username{} {} |
There was a problem hiding this comment.
warning: initializer for member 'id' is redundant [readability-redundant-member-init]
| PronounUser::PronounUser(): id{}, username{} {} | |
| PronounUser::PronounUser(): , username{} {} |
|
|
||
| namespace chatterino { | ||
|
|
||
| PronounUser::PronounUser(): id{}, username{} {} |
There was a problem hiding this comment.
warning: initializer for member 'username' is redundant [readability-redundant-member-init]
| PronounUser::PronounUser(): id{}, username{} {} | |
| PronounUser::PronounUser(): id{}, {} |
| {"ziehir", "zie/hir"}, | ||
| }; | ||
|
|
||
| /*static*/ UserPronouns PronounsAlejoApi::parse(QJsonArray array) { |
There was a problem hiding this comment.
warning: method 'parse' can be made static [readability-convert-member-functions-to-static]
| /*static*/ UserPronouns PronounsAlejoApi::parse(QJsonArray array) { | |
| /*static*/ static UserPronouns PronounsAlejoApi::parse(QJsonArray array) { |
| {"ziehir", "zie/hir"}, | ||
| }; | ||
|
|
||
| /*static*/ UserPronouns PronounsAlejoApi::parse(QJsonArray array) { |
There was a problem hiding this comment.
warning: out-of-line definition of 'parse' does not match any declaration in 'chatterino::PronounsAlejoApi' [clang-diagnostic-error]
/*static*/ UserPronouns PronounsAlejoApi::parse(QJsonArray array) {
^| {"ziehir", "zie/hir"}, | ||
| }; | ||
|
|
||
| /*static*/ UserPronouns PronounsAlejoApi::parse(QJsonArray array) { |
There was a problem hiding this comment.
warning: the parameter 'array' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
| /*static*/ UserPronouns PronounsAlejoApi::parse(QJsonArray array) { | |
| /*static*/ UserPronouns PronounsAlejoApi::parse(const QJsonArray& array) { |
| return {}; | ||
| } | ||
|
|
||
| if (array.size() == 0) { |
There was a problem hiding this comment.
warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
| if (array.size() == 0) { | |
| if (array.empty()) { |
Additional context
.qtinstall/Qt/6.6.2/gcc_64/include/QtCore/qjsonarray.h:209: method 'QJsonArray'::empty() defined here
inline bool empty() const { return isEmpty(); }
^
I think it should be easily possible to fetch on a timer, although I'm not familiar enough with the code base to know how or where timers are usually registered and run. Could you give me a pointer to where timers are usually placed? There was a "recent chatters" variable somewhere iirc, we could use this to occasionally on timer fetch pronouns for those. Alejo.io unfortunately, to my understanding, has no documentation for the API at all, PronounDB has a documentation at https://pronoundb.org/wiki/api-docs and mentions setting a User-Agent (which should be set, as
I just realized I read and then completely skipped the entire It also seems some things are broken on windows build (I think it's some |
DeltaTimo
changed the title
DeltaTimo
changed the title
|
Any sort of nitpicks or windows/macos/linux-specific things you can't fix can be easily fixed by any of our other contributors as part of the review process - we don't 100% listen to clang-tidy, so you can make a judgement call whether you care to listen to it For the timer thing, I can bring up some example when I do my review of this PR in the weekend. |
Alright that sounds good! I'll go through the code again later or tomorrow.
Sounds good! When will you be reviewing? I might force-push new changes to my branch, but if needed I can just create a new branch and work on that.
I've already created a checkbox in the settings menu to opt-out, I'll default it to disable then, which should also disable fetching. |
|
I should be able to get a full review in on Saturday on Sunday on my stream, can delay to Sunday (or to whenever) if you want more time before I take a look We Squash & Merge here, so you don't have to keep commits clean, I just prefer no force pushes after I've started reviewing |
|
To fix compilation on Windows, you need to add an diff --git a/src/providers/pronouns/pronoundb/PronounsPronounDbApi.cpp b/src/providers/pronouns/pronoundb/PronounsPronounDbApi.cpp
index ca39f70f..728aed67 100644
--- a/src/providers/pronouns/pronoundb/PronounsPronounDbApi.cpp
+++ b/src/providers/pronouns/pronoundb/PronounsPronounDbApi.cpp
@@ -111,7 +111,7 @@ void PronounsPronounDbApi::fetch(std::vector<PronounUser> allUsers, IPronounsApi
auto request = std::make_shared<IPronounsApi::RequestT>(allUsers.size(), std::move(onDone));
for (std::size_t i {0}; i < allUsers.size(); i += 50) {
- auto end = static_cast<std::size_t>(std::min(allUsers.size() - i, 50ul));
+ auto end = std::min(allUsers.size() - i, 50ull);
auto users = std::vector<PronounUser>(allUsers.begin() + i, allUsers.begin() + i + end);
NetworkRequest(PronounsPronounDbApi::makeRequest(users))Just some general comments:
|
You can (and probably should) leave these changes until the implementation has been reviewed
this is the same as i mentioned above |
DeltaTimo
force-pushed
the
mr/chatterino2/add-pronoun-provider
branch
2 times, most recently
from
488b6a7
to
d72b09c
Compare
|
I just noticed both of you had commented since I last checked (and before I pushed)! @Nerixyz will do! The latest version now queues users for fetching when messages arrive and are regularly dequeued and fetched in a batch. I don't think the alejo.io API supports batch requests and the (open-source) browser extension fetches per message too, but I still added a limit to avoid completely flooding it. |
|
|
||
| namespace chatterino { | ||
|
|
||
| class IPronounsApi |
There was a problem hiding this comment.
warning: destructor of 'IPronounsApi' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
class IPronounsApi
^Additional context
src/providers/pronouns/IPronounsApi.hpp:13: make it public and virtual
class IPronounsApi
^| using RequestT = PronounsApiRequest<ResultT>; | ||
|
|
||
| // Fetch pronouns from the API. | ||
| virtual void fetch(std::vector<PronounUser> users, |
There was a problem hiding this comment.
warning: the parameter 'users' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
virtual void fetch(std::vector<PronounUser> users,
^|
|
||
| // Fetch pronouns from the API. | ||
| virtual void fetch(std::vector<PronounUser> users, | ||
| RequestT::CallbackT &&onDone){}; |
There was a problem hiding this comment.
warning: rvalue reference parameter 'onDone' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved]
RequestT::CallbackT &&onDone){};
^| namespace chatterino { | ||
|
|
||
| PronounUser::PronounUser() | ||
| : id{} |
There was a problem hiding this comment.
warning: initializer for member 'id' is redundant [readability-redundant-member-init]
| : id{} | |
| : |
|
|
||
| PronounUser::PronounUser() | ||
| : id{} | ||
| , username{} |
There was a problem hiding this comment.
warning: initializer for member 'username' is redundant [readability-redundant-member-init]
| , username{} | |
| , |
| } | ||
| UserPronouns::UserPronouns(std::string pronouns) | ||
| { | ||
| if (pronouns.length() == 0) |
There was a problem hiding this comment.
warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty]
| if (pronouns.length() == 0) | |
| if (pronouns.empty()) |
Additional context
/usr/include/c++/13/bits/basic_string.h:1219: method 'basic_string'::empty() defined here
empty() const _GLIBCXX_NOEXCEPT
^| UserPronouns(); | ||
| UserPronouns(std::string); | ||
|
|
||
| inline std::string format() const |
There was a problem hiding this comment.
warning: function 'format' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
| inline std::string format() const | |
| std::string format() const |
| /*static*/ std::optional<std::unordered_map<std::string, std::string>> | ||
| PronounsAlejoApi::pronounsFromId = {}; | ||
|
|
||
| /*static*/ UserPronouns PronounsAlejoApi::parse(QJsonObject object) |
There was a problem hiding this comment.
warning: method 'parse' can be made static [readability-convert-member-functions-to-static]
| /*static*/ UserPronouns PronounsAlejoApi::parse(QJsonObject object) | |
| /*static*/ static UserPronouns PronounsAlejoApi::parse(QJsonObject object) |
| /*static*/ std::optional<std::unordered_map<std::string, std::string>> | ||
| PronounsAlejoApi::pronounsFromId = {}; | ||
|
|
||
| /*static*/ UserPronouns PronounsAlejoApi::parse(QJsonObject object) |
There was a problem hiding this comment.
warning: out-of-line definition of 'parse' does not match any declaration in 'chatterino::PronounsAlejoApi' [clang-diagnostic-error]
/*static*/ UserPronouns PronounsAlejoApi::parse(QJsonObject object)
^
My main issue I outlined in #3346 was that alejo almost certainly can't keep up with the amount of requests we'd send, that 2 different implementations would return a different subset of users with pronouns, and it falls to blaming Chatterino if this feature we add works very sporadically (e.g. user reaction to bttv live emote updates) pronounsdb does look like it could better handle the brunt of our requests in comparison though |
PronounDB really does seem to handle it better, but it is unfortunately less widely used at least in the channels I've been lurking in. I'm wondering how alejo.io is working in the first place, as it seems that the browser extension is working in a flooding way too. If this is the intended way, and the way it is already implemented in its own browser extension, I don't see what sets apart Chatterino from just using a browser with the extension enabled (and then flooding the server in very lively channels). |
Browser chat is 1 channel and Chatterino is up to 100 |
True. I suppose only queuing new users on message in the active tab leads to the aforementioned weird sporadic behaviour? |
|
My recommendation is to only display the pronouns in the usercard - this minimizes http requests and avoids implementation complexity (of batching requests or caching thousands of pronouns) I understand that the usercard approach reduces the convenience factor for this feature, but the technical challenges (in my opinion) puts us in a situation where we need to apply more pressure on Twitch to natively support pronouns (to display it inline with each chat message). Similar to the no-video and no-audio badges, there should be he,she,they badges natively selectable on Twitch, which would be very easy for us to support in a scalable manner |
not even just a badge, pronouns sent in the tags would be fine too (don't care about people who hate tags, they work) |
|
|
||
| namespace chatterino { | ||
|
|
||
| class IPronounsApi |
There was a problem hiding this comment.
warning: destructor of 'IPronounsApi' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
class IPronounsApi
^Additional context
src/providers/pronouns/IPronounsApi.hpp:13: make it public and virtual
class IPronounsApi
^| using RequestT = PronounsApiRequest<ResultT>; | ||
|
|
||
| // Fetch pronouns from the API. | ||
| virtual void fetch(std::vector<PronounUser> users, |
There was a problem hiding this comment.
warning: the parameter 'users' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
virtual void fetch(std::vector<PronounUser> users,
^|
|
||
| // Fetch pronouns from the API. | ||
| virtual void fetch(std::vector<PronounUser> users, | ||
| RequestT::CallbackT &&onDone){}; |
There was a problem hiding this comment.
warning: rvalue reference parameter 'onDone' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved]
RequestT::CallbackT &&onDone){};
^| namespace chatterino { | ||
|
|
||
| PronounUser::PronounUser() | ||
| : id{} |
There was a problem hiding this comment.
warning: initializer for member 'id' is redundant [readability-redundant-member-init]
| : id{} | |
| : |
|
|
||
| PronounUser::PronounUser() | ||
| : id{} | ||
| , username{} |
There was a problem hiding this comment.
warning: initializer for member 'username' is redundant [readability-redundant-member-init]
| , username{} | |
| , |
| UserPronouns(); | ||
| UserPronouns(std::string); | ||
|
|
||
| inline std::string format() const |
There was a problem hiding this comment.
warning: function 'format' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
| inline std::string format() const | |
| std::string format() const |
| /*static*/ std::optional<std::unordered_map<std::string, std::string>> | ||
| PronounsAlejoApi::pronounsFromId = {}; | ||
|
|
||
| /*static*/ UserPronouns PronounsAlejoApi::parse(QJsonObject object) |
There was a problem hiding this comment.
warning: method 'parse' can be made static [readability-convert-member-functions-to-static]
| /*static*/ UserPronouns PronounsAlejoApi::parse(QJsonObject object) | |
| /*static*/ static UserPronouns PronounsAlejoApi::parse(QJsonObject object) |
| else | ||
| { | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
warning: do not use 'else' after 'return' [llvm-else-after-return]
| } | |
| return {}; | |
| NetworkRequest(PronounsAlejoApi::API_URL + PronounsAlejoApi::API_USERS + | ||
| "/" + user.username) | ||
| .concurrent() | ||
| .onSuccess([request, user](auto result) { |
There was a problem hiding this comment.
warning: no viable conversion from '(lambda at /github/workspace/src/providers/pronouns/alejo/PronounsAlejoApi.cpp:134:24)' to 'NetworkSuccessCallback' (aka 'function<void (NetworkResult)>') [clang-diagnostic-error]
.onSuccess([request, user](auto result) {
^Additional context
/usr/include/c++/13/bits/std_function.h:374: candidate constructor not viable: no known conversion from '(lambda at /github/workspace/src/providers/pronouns/alejo/PronounsAlejoApi.cpp:134:24)' to 'nullptr_t' (aka 'std::nullptr_t') for 1st argument
function(nullptr_t) noexcept
^/usr/include/c++/13/bits/std_function.h:385: candidate constructor not viable: no known conversion from '(lambda at /github/workspace/src/providers/pronouns/alejo/PronounsAlejoApi.cpp:134:24)' to 'const function<void (NetworkResult)> &' for 1st argument
function(const function& __x)
^/usr/include/c++/13/bits/std_function.h:403: candidate constructor not viable: no known conversion from '(lambda at /github/workspace/src/providers/pronouns/alejo/PronounsAlejoApi.cpp:134:24)' to 'function<void (NetworkResult)> &&' for 1st argument
function(function&& __x) noexcept
^/usr/include/c++/13/bits/std_function.h:434: candidate template ignored: requirement '_Callable<(lambda at /github/workspace/src/providers/pronouns/alejo/PronounsAlejoApi.cpp:134:24), (lambda at /github/workspace/src/providers/pronouns/alejo/PronounsAlejoApi.cpp:134:24), std::__invoke_result<(lambda at /github/workspace/src/providers/pronouns/alejo/PronounsAlejoApi.cpp:134:24) &, chatterino::NetworkResult>>::value' was not satisfied [with _Functor = (lambda at /github/workspace/src/providers/pronouns/alejo/PronounsAlejoApi.cpp:134:24)]
function(_Functor&& __f)
^src/common/network/NetworkRequest.hpp:46: passing argument to parameter 'cb' here
NetworkRequest onSuccess(NetworkSuccessCallback cb) &&;
^
|
I don't think I'll be able to update the PR this weekend, but I've installed the clang LS extension and I starting working through the suggestions yesterday. The owner of PronounDB also replied yesterday I'll read it carefully tomorrow or so and share her comments. Apologies for accidentally closing the PR yesterday, I was scrolling through the comments and somehow clicked "Close" 😒 |
|
Alright, I'll wait with reviewing the PR until you've had a chance to look at it further then. Poke me! |
There are certain pronoundb plugins for Discord, which show pronouns for a user. They show the pronouns inline with the messages, but only after you click on their profile, like so: User message before you open the profile: User message after you open the profile: In my opinion this is a fairly decent approach to this sort of situation, we could do it like so: Before: Open user card: After: I feel like this could be a decent middleground between only showing it in the usercard and making requests for like every user. thoughts? |
|
I would be fine with that compromise @treuks |
|
compromise is good; i'm not a maintainer but could be easier to review if 2 separate prs |
If you use clangd, you get |
|
@DeltaTimo if you're short on time i'd be happy to work on the review comments to get this past the finish line if you want to add me as a collaborator |
@iProdigy I have some changes ready to push, if I've missed anything feel free to work on it! @Nerixyz Do I force push the changes on my branch? |
Since there are review comments already, it's better not to (unless you substantially changed the design). In the end, they will be squashed anyway. |
There was a problem hiding this comment.
Looks much better! Only a few nitpicks and some of the old comments. I marked all comments that you addressed and all that are invalid (due to clang-tidy not knowing about the header) with 👎. I can't resolve them, but you can (by clicking on the "Resolve" button). This makes the https://github.com/Chatterino/chatterino2/pull/5442/files view a bit cleaner. I marked suggestions that are still "valid" with 👍 (really only nitpicks - looks good otherwise).
CHANGELOG.md
Outdated
| @@ -1,5 +1,9 @@ | |||
| # Changelog | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| - Minor: Add option to show pronouns in user card. (#5442) | |||
There was a problem hiding this comment.
This should be moved to Unversioned - and maybe a major entry(?).
src/Application.cpp
Outdated
| @@ -147,6 +148,7 @@ Application::Application(Settings &_settings, const Paths &paths, | |||
| , logging(new Logging(_settings)) | |||
| , linkResolver(new LinkResolver) | |||
| , streamerMode(new StreamerMode) | |||
| , pronouns(new pronouns::Pronouns) | |||
There was a problem hiding this comment.
Nit: use std::make_shared<...> here (it creates a single allocation at the cost of weak pointers keeping the object alive). Also makes it more clear that this is a shared_ptr instead of a unique_ptr.
src/providers/pronouns/Pronouns.cpp
Outdated
| { | ||
| // Only fetch pronouns if we haven't fetched before. | ||
| { | ||
| std::shared_lock<std::shared_mutex> lock(this->mutex); |
There was a problem hiding this comment.
Nit: The template argument could be removed, as it is automatically deduced (same for the other locations).
src/providers/pronouns/Pronouns.hpp
Outdated
| private: | ||
| // mutex for editing the saved map. | ||
| std::shared_mutex mutex; | ||
| // Login name -> Pronouns | ||
| std::unordered_map<QString, UserPronouns> saved; | ||
| AlejoApi alejoApi; |
There was a problem hiding this comment.
This should be moved below the public items (so that public items come first).
src/providers/pronouns/Pronouns.hpp
Outdated
| public: | ||
| Pronouns() = default; | ||
|
|
||
| void fetch(const QString &user, |
There was a problem hiding this comment.
| void fetch(const QString &user, | |
| void fetch(const QString &username, |
Signifying this isn't a userID (it's already renamed in the implementation)
| qCDebug(chatterinoPronouns) | ||
| << "Fetching pronouns from alejo.io for " << username; | ||
|
|
||
| alejoApi.fetch(username, [this, callbackSuccess, callbackFail, |
There was a problem hiding this comment.
Oh, I forgot this: As part of the "cleanly exit Chatterino" effort: This shouldn't capture this, but a weak_ptr to this (obtained through weak_from_this; and the class has to inherit from std::enable_shared_from_this).
Similarly, for AlejoApi, which shouldn't capture this in network requests. It could store a std::weak_ptr<void> to its parent (Pronouns) on construction and use that to ensure it's still alive when receiving a response for network requests.
But that isn't something you have to address - I suspect pajlada will take care of that before merging.
src/providers/pronouns/Pronouns.hpp
Outdated
| // mutex for editing the saved map. | ||
| std::shared_mutex mutex; | ||
| // Login name -> Pronouns | ||
| std::unordered_map<QString, UserPronouns> saved; |
There was a problem hiding this comment.
this will work for now - in the future if we start doing bulk lookups, we'll want to switch to lru_cache to avoid a (bigger) memory leak
Ah I was wondering where the 👎 comments were but Felanbird has apparently resolved them already. I'll work on the rest real quick! |
|
Afaik the reason for the CI not running are the conflicts |
|
Awesome! I'll try to merge and remove the draft tag. |
DeltaTimo
changed the title
benchmarks/src/RecentMessages.cpp
Outdated
| @@ -124,6 +129,7 @@ class MockApplication : public mock::BaseApplication | |||
| FfzEmotes ffzEmotes; | |||
| SeventvEmotes seventvEmotes; | |||
| DisabledStreamerMode streamerMode; | |||
| Pronouns pronouns; | |||
There was a problem hiding this comment.
warning: field has incomplete type 'chatterino::pronouns::Pronouns' [clang-diagnostic-error]
Pronouns pronouns;
^Additional context
src/Application.hpp:56: forward declaration of 'chatterino::pronouns::Pronouns'
class Pronouns;
^
benchmarks/src/RecentMessages.cpp
Outdated
| @@ -124,6 +129,7 @@ | |||
| FfzEmotes ffzEmotes; | |||
| SeventvEmotes seventvEmotes; | |||
| DisabledStreamerMode streamerMode; | |||
| Pronouns pronouns; | |||
There was a problem hiding this comment.
warning: unknown type name 'Pronouns'; did you mean 'chatterino::pronouns::Pronouns'? [clang-diagnostic-error]
| Pronouns pronouns; | |
| chatterino::pronouns::Pronouns pronouns; |
Additional context
src/Application.hpp:56: 'chatterino::pronouns::Pronouns' declared here
class Pronouns;
^| @@ -257,6 +257,13 @@ class EmptyApplication : public IApplication | |||
| return nullptr; | |||
| } | |||
|
|
|||
| pronouns::Pronouns *getPronouns() override | |||
There was a problem hiding this comment.
warning: method 'getPronouns' can be made static [readability-convert-member-functions-to-static]
| pronouns::Pronouns *getPronouns() override | |
| static pronouns::Pronouns *getPronouns() override |
| @@ -257,6 +257,13 @@ | |||
| return nullptr; | |||
| } | |||
|
|
|||
| pronouns::Pronouns *getPronouns() override | |||
| { | |||
| assert(false && "EmptyApplication::getPronouns was called without " | |||
There was a problem hiding this comment.
warning: implicit conversion 'const char *' -> 'bool' [readability-implicit-bool-conversion]
assert(false && "EmptyApplication::getPronouns was called without "
^this fix will not be applied because it overlaps with another fix
| pronouns::Pronouns *getPronouns() override | ||
| { | ||
| assert(false && "EmptyApplication::getPronouns was called without " | ||
| "being initialized"); |
There was a problem hiding this comment.
warning: redundant boolean literal supplied to boolean operator [readability-simplify-boolean-expr]
| "being initialized"); | |
| assert(false); |
tests/src/InputCompletion.cpp
Outdated
| AccountController accounts; | ||
| mock::MockTwitchIrcServer twitch; | ||
| Emotes emotes; | ||
| BttvEmotes bttvEmotes; | ||
| FfzEmotes ffzEmotes; | ||
| SeventvEmotes seventvEmotes; | ||
| Pronouns pronouns; |
There was a problem hiding this comment.
warning: field has incomplete type 'chatterino::pronouns::Pronouns' [clang-diagnostic-error]
Pronouns pronouns;
^Additional context
src/Application.hpp:56: forward declaration of 'chatterino::pronouns::Pronouns'
class Pronouns;
^
tests/src/InputCompletion.cpp
Outdated
| AccountController accounts; | ||
| mock::MockTwitchIrcServer twitch; | ||
| Emotes emotes; | ||
| BttvEmotes bttvEmotes; | ||
| FfzEmotes ffzEmotes; | ||
| SeventvEmotes seventvEmotes; | ||
| Pronouns pronouns; |
There was a problem hiding this comment.
warning: unknown type name 'Pronouns'; did you mean 'chatterino::pronouns::Pronouns'? [clang-diagnostic-error]
| Pronouns pronouns; | |
| chatterino::pronouns::Pronouns pronouns; |
Additional context
src/Application.hpp:56: 'chatterino::pronouns::Pronouns' declared here
class Pronouns;
^
tests/src/MessageBuilder.cpp
Outdated
| @@ -91,6 +91,20 @@ class MockApplication : public mock::BaseApplication | |||
| return &this->seventvEmotes; | |||
| } | |||
|
|
|||
| <<<<<<< HEAD:tests/src/TwitchMessageBuilder.cpp | |||
There was a problem hiding this comment.
warning: version control conflict marker in file [clang-diagnostic-error]
<<<<<<< HEAD:tests/src/TwitchMessageBuilder.cpp
^
tests/src/MessageBuilder.cpp
Outdated
| @@ -102,6 +116,7 @@ | |||
| BttvEmotes bttvEmotes; | |||
| FfzEmotes ffzEmotes; | |||
| SeventvEmotes seventvEmotes; | |||
| Pronouns::Pronouns pronouns; | |||
There was a problem hiding this comment.
warning: field has incomplete type 'pronouns::Pronouns' [clang-diagnostic-error]
Pronouns::Pronouns pronouns;
^Additional context
src/Application.hpp:56: forward declaration of 'chatterino::pronouns::Pronouns'
class Pronouns;
^
tests/src/MessageBuilder.cpp
Outdated
| @@ -102,6 +116,7 @@ | |||
| BttvEmotes bttvEmotes; | |||
| FfzEmotes ffzEmotes; | |||
| SeventvEmotes seventvEmotes; | |||
| Pronouns::Pronouns pronouns; | |||
There was a problem hiding this comment.
warning: use of undeclared identifier 'Pronouns'; did you mean 'pronouns'? [clang-diagnostic-error]
| Pronouns::Pronouns pronouns; | |
| pronouns::Pronouns pronouns; |
Additional context
src/Application.hpp:55: 'pronouns' declared here
namespace pronouns {
^
| qCDebug(chatterinoPronouns) << "Fetching available pronouns for alejo.io"; | ||
| NetworkRequest(AlejoApi::API_URL + AlejoApi::API_PRONOUNS) | ||
| .concurrent() | ||
| .cache() |
There was a problem hiding this comment.
How well or not well does this handle caching?
From my cursory glance at the caching system, I can see a scenario where:
- the data gets loaded and saved to cache
- a user adds themselves to the database
- this gets loaded from cache but still has the old data.
Even restarting Chatterino probably wouldn't help because the cached data will still be in cache and it will just be reloaded.
There was a problem hiding this comment.
This is only for the pronouns (i.e. some-pronoun-id → "they/them") not for the users, thus it's not frequently updated. But I agree that it should be removed, because we don't have any way of invalidating this.
tests/src/MessageBuilder.cpp
Outdated
| <<<<<<< HEAD:tests/src/TwitchMessageBuilder.cpp | ||
| IStreamerMode *getStreamerMode() override | ||
| { | ||
| return &this->streamerMode; | ||
| } | ||
|
|
||
| pronouns::Pronouns *getPronouns() override | ||
| { | ||
| return &this->pronouns; | ||
| } | ||
|
|
||
| Settings settings; | ||
| ======= | ||
| >>>>>>> master:tests/src/MessageBuilder.cpp |
There was a problem hiding this comment.
Still have merge conflict markers here
| <<<<<<< HEAD:tests/src/TwitchMessageBuilder.cpp | |
| IStreamerMode *getStreamerMode() override | |
| { | |
| return &this->streamerMode; | |
| } | |
| pronouns::Pronouns *getPronouns() override | |
| { | |
| return &this->pronouns; | |
| } | |
| Settings settings; | |
| ======= | |
| >>>>>>> master:tests/src/MessageBuilder.cpp | |
| IStreamerMode *getStreamerMode() override | |
| { | |
| return &this->streamerMode; | |
| } | |
| pronouns::Pronouns *getPronouns() override | |
| { | |
| return &this->pronouns; | |
| } | |
| Settings settings; |
There was a problem hiding this comment.
Huh, that's odd. git diff --check didn't catch that (or perhaps more likely, I forgot to run it one last time)
|
How can I run the benchmarks, tests, etc. on my machine? I'm getting In any case, I've pushed some changes that should hopefully fix the build errors. |
Are you sure you properly checked out all submodules ( |
I didn't, that was definitely it. |
Common pronoun providers like alejo.io and pronoundb.org have respective browser extensions for twitch, but so far Chatterino does not support these providers.
I have now implemented a provider that fetches pronouns from APIs.
Pronoun providers can be configured in code, where, if multiple are configured the latter ones serve as a fallback for the former ones.
Right now, pronoundb.org and pronouns.alejo.io are implemented.
Pronouns are fetched when a message from a new user arrives and is cached application wide per session (as opposed to cache per tab). Pronouns are not added to the first message from a user. In the future, pronouns could be added to previous messages too.
For future-proofing, it is possible to fetch pronouns for multiple users at once, but right now, pronouns are fetched per-message if required.