-
Notifications
You must be signed in to change notification settings - Fork 44
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
fix StartupMessages; implement messages from master #211
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little too excessive on the if HasMember && IsType machinery for my taste, and it seems odd to me that RapidJSON doesn't validate the type in its getters by default. Perhaps it would make sense to write a utility like the following:
class JsonHelper
{
public:
explicit
JsonHelper (const rapidjson::Value& value) : json (value) {}
bool
HasAllKeys (const std::initializer_list<std::string_view>& keys)
{
return std::ranges::all_of (keys,
[this] (std::string_view key)
{
return json.HasMember (key.data ());
});
}
std::optional <std::string>
GetString (const std::string_view key) const
{
if (json.HasMember (key.data ()) && json[key.data ()].IsString ())
return json[key.data ()].GetString ();
return std::nullopt;
}
private:
const rapidjson::Value& json;
};
But overall, it's not that important. That said, one change I want to see here is a comment section, potentially at the beginning of the file, to explain the popup machinery, with perhaps an example and, more importantly, documentation on how it handles invalid cases
Minus that, lgtm, thank! 🥳
Additional change, updated website url used in the main menu to https://alterware.dev iw4x-client/src/Components/Modules/News.cpp Line 132 in 4b5dc52
|
We should also decide whether we want to keep the 2 current startup messages iw4x-client/src/Steam/Steam.cpp Line 128 in a81e7b3
iw4x-client/src/Components/Modules/Maps.cpp Line 611 in a81e7b3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good.
Tested it and it runs as it should, no side effects as far as I can see.
Before merging: Please remove the "missing steam" startup message if that's okay
And don't abuse your newfound power of displaying random stuff in front of the users 😂
whoopsie please solve merge conflict |
fix StartupMessages; implement messages from master
What does this PR do?
StartupMessages::Show
StartupMessages::Show
at a later point and re-use it to display information to the user, but this is untestedStartupMessages::Show
tocheckFirstLaunch
UIScript which gets called by the main menuStartupMessages::AddMessage(message, title)
to allow setting a titlehttps://iw4x.getserve.rs/v1/info
void Changelog::LoadChangelog()
tovoid Changelog::SetChangelog(const std::string& changelog)
How does this PR change IW4x's behaviour?
Anything else we should know?
In case the master should be down/inaccesible to maintainers there's still the fallback url
https://raw.githubusercontent.com/iw4x/iw4x-cache
which should take priority over the master url. To use it the repository has to be created and an "info" file has to be created with content like this:special thanks to wroy and louve for explaining c++ basics multiple times c: