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

fix StartupMessages; implement messages from master #211

Merged
merged 5 commits into from
Jan 24, 2025

Conversation

mxve
Copy link
Contributor

@mxve mxve commented Jan 21, 2025

What does this PR do?

  • Fixed StartupMessages
    • Fixed some logic errors
    • Created StartupMessages::Show
      • I could not determine how StartupMessages was initially meant to be called and I couldn't find it in the git history either, I assume it was called through menus at some point.
      • With the changes it should be possible to call StartupMessages::Show at a later point and re-use it to display information to the user, but this is untested
    • Added StartupMessages::Show to checkFirstLaunch UIScript which gets called by the main menu
  • Added overload for StartupMessages::AddMessage(message, title) to allow setting a title
  • Added startup messages retrieved from the master at https://iw4x.getserve.rs/v1/info
    • Verified that json parsing doesn't fail if content mismatches/no internet connection
  • Changed void Changelog::LoadChangelog() to void Changelog::SetChangelog(const std::string& changelog)
    • The original function was already called from News, it now passes the changelog content too, which allows for the next change
  • Combined motd, changelog and the new startup message web requests into a single request

How does this PR change IW4x's behaviour?

image

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:

{
   "motd":"Message of the day text",
   "popmenu":[
      {
         "title":"Everyone",
         "message":"This message is displayed
to every IW4x revision (after this is merged)",
         "revisions":[
            "any"
         ],
         "show":true
      },
      {
         "title":"Specific rev",
         "message":"This message is only displayed to revision r4809",
         "revisions":[
            "r4809"
         ],
         "show":true
      }
   ],
   "changelog":"What's changed in r4794 (client) | 2025-01-09\n* Fix gamepad performance\n\nWhat's changed in r4773 (client) | 2024-12-13\n* Fixed a security issue that allowed servers to send malicious commands to the game. This issue potentially affects other CoD games aswell.\n\nWhat's changed in r4768 (client) | 2024-12-08\n* Fix performance issues waiting on controller IO at a very high framerate\r\n* Fix of fps drop when using a high polling rate mouse\r\n* Wait for inputs to be read before reading them again\r\n* Fix editorconfig misconfiguration\r\n* Use master node for changelog and motd\r\n* Feat/improve exceptions\r\n* fix: make .str parsing with ZB useful\r\n* fixed codol LOD"
}

special thanks to wroy and louve for explaining c++ basics multiple times c:

Copy link
Collaborator

@wroyca wroyca left a 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! 🥳

src/Components/Modules/News.cpp Outdated Show resolved Hide resolved
src/Components/Modules/News.cpp Outdated Show resolved Hide resolved
src/Components/Modules/News.cpp Outdated Show resolved Hide resolved
src/Components/Modules/News.cpp Outdated Show resolved Hide resolved
src/Components/Modules/News.cpp Outdated Show resolved Hide resolved
src/Components/Modules/StartupMessages.cpp Outdated Show resolved Hide resolved
src/Components/Modules/StartupMessages.cpp Outdated Show resolved Hide resolved
src/Components/Modules/StartupMessages.cpp Outdated Show resolved Hide resolved
src/Components/Modules/StartupMessages.cpp Outdated Show resolved Hide resolved
src/Components/Modules/StartupMessages.cpp Outdated Show resolved Hide resolved
@wroyca wroyca requested a review from Rackover January 21, 2025 17:57
@mxve
Copy link
Contributor Author

mxve commented Jan 21, 2025

Additional change, updated website url used in the main menu to https://alterware.dev

Utils::OpenUrl("https://alterware.dev");

@wroyca wroyca assigned wroyca and Rackover and unassigned wroyca Jan 22, 2025
@mxve
Copy link
Contributor Author

mxve commented Jan 23, 2025

We should also decide whether we want to keep the 2 current startup messages

Components::StartupMessages::AddMessage("Warning:\nUnable to connect to Steam. Steam features will be unavailable");

StartupMessages::AddMessage("Warning:\n You only have some of DLCs 3-5 which are all required to be installed to work. There may be issues with those maps.");

Copy link
Contributor

@Rackover Rackover left a 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 😂

@Rackover
Copy link
Contributor

whoopsie please solve merge conflict

@Rackover Rackover merged commit 0b84424 into iw4x:develop Jan 24, 2025
2 checks passed
wroyca pushed a commit to SnowyWhite/iw4y that referenced this pull request Jan 24, 2025
fix StartupMessages; implement messages from master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants