Changeset View
Standalone View
source/ps/ModIo.h
- This file was added.
/* Copyright (C) 2018 Wildfire Games. | |||||
* | |||||
* Permission is hereby granted, free of charge, to any person obtaining | |||||
* a copy of this software and associated documentation files (the | |||||
* "Software"), to deal in the Software without restriction, including | |||||
* without limitation the rights to use, copy, modify, merge, publish, | |||||
* distribute, sublicense, and/or sell copies of the Software, and to | |||||
* permit persons to whom the Software is furnished to do so, subject to | |||||
* the following conditions: | |||||
* | |||||
* The above copyright notice and this permission notice shall be included | |||||
* in all copies or substantial portions of the Software. | |||||
* | |||||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, | |||||
* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF | |||||
* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. | |||||
* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY | |||||
* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, | |||||
* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE | |||||
* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | |||||
*/ | |||||
#ifndef INCLUDED_MODIO | |||||
#define INCLUDED_MODIO | |||||
#include <string> | |||||
leper: What is this doing in the header? That is an implementation detail only. | |||||
#include "lib/external_libraries/curl.h" | |||||
Done Inline ActionsWhy the newline? leper: Why the newline? | |||||
Not Done Inline ActionsI wanted to separate that from the 0 A.D. headers since it is almost a library include but not quite, but that's just silly. Fixed locally. Itms: I wanted to separate that from the 0 A.D. headers since it is almost a library include but not… | |||||
Done Inline ActionsNo newline after this? leper: No newline after this? | |||||
Not Done Inline ActionsFixed the include conventions for all the new files. Itms: Fixed the include conventions for all the new files. | |||||
class DownloadCallbackData; | |||||
class ScriptInterface; | |||||
struct ModIoModData | |||||
Done Inline Actions(ModIO?) elexis: (ModIO?) | |||||
{ | |||||
Done Inline ActionsIf I'm not mistaken this data is actually some ModData, since the result from the API is parsed to our own data format. This is not important as long as mod.io is the only way of retrieving it, however that means GetMods maybe should return something that can be used after g_ModIo is deleted (I'd pass a mutable reference to that function). Changing the name would clarify that. Itms: If I'm not mistaken this data is actually some `ModData`, since the result from the API is… | |||||
Not Done Inline ActionsIt is mostly ModData returned by the API (after some parsing). Storing the output from GetMods after g_ModIo stops existing leads you to issues of trying to download something that is not what you want, or might just not work. The data returned by GetMods is only valid for as long as the instance from which you got it exists, and only for that specific instance. So no. leper: It is mostly ModData returned by the API (after some parsing). Storing the output from… | |||||
std::map<std::string, std::string> properties; | |||||
std::vector<std::string> dependencies; | |||||
std::vector<std::string> minisigs; | |||||
}; | |||||
class ModIo | |||||
{ | |||||
NONCOPYABLE(ModIo); | |||||
public: | |||||
ModIo(); | |||||
~ModIo(); | |||||
const std::vector<ModIoModData>& GetMods(const ScriptInterface& scriptinterface); | |||||
void DownloadMod(size_t idx); | |||||
private: | |||||
static size_t ReceiveCallback(void* buffer, size_t size, size_t nmemb, void* userp); | |||||
static size_t DownloadCallback(void* buffer, size_t size, size_t nmemb, void* userp); | |||||
CURLcode PerformRequest(const std::string& url); | |||||
// TODO: better names for these... | |||||
Not Done Inline ActionsThis looks very over-engineered. leper: This looks very over-engineered. | |||||
Not Done Inline ActionsYeah structs seem to be my little indulgence. Do you see a better-looking alternative? Itms: Yeah `struct`s seem to be my little indulgence. Do you see a better-looking alternative? | |||||
Not Done Inline ActionsThat it uses a struct isn't bad, that it got so big that a struct was a nice solution is what is questionable. std::pair<int, double> seems like a possibility, or use an enum there instead as that already exists. Also while there use enum class. leper: That it uses a `struct` isn't bad, that it got so big that a `struct` was a nice solution is… | |||||
Not Done Inline ActionsI'll repeat the enum class thing since it seem to have been ignored. leper: I'll repeat the `enum class` thing since it seem to have been ignored. | |||||
static bool ParseGameIdResponse(const ScriptInterface& scriptInterface, const std::string& responseData, int& id); | |||||
static bool ParseModsResponse(const ScriptInterface& scriptInterface, const std::string& responseData, std::vector<ModIoModData>& modData); | |||||
bool ParseGameId(const ScriptInterface& scriptInterface); | |||||
bool ParseMods(const ScriptInterface& scriptInterface); | |||||
bool VerifyDownload(const OsPath& filePath, DownloadCallbackData& callbackData, const ModIoModData& modData); | |||||
void PrintMods(); | |||||
// Url parts | |||||
std::string m_BaseUrl; | |||||
std::string m_GamesRequest; | |||||
std::string m_GameId; | |||||
// Query parameters | |||||
std::string m_ApiKey; | |||||
std::string m_IdQuery; | |||||
CURL* m_Curl; | |||||
curl_slist* m_Headers; | |||||
char m_ErrorBuffer[CURL_ERROR_SIZE]; | |||||
std::string m_ResponseData; | |||||
std::vector<ModIoModData> m_ModData; | |||||
friend class TestModIo; | |||||
}; | |||||
extern ModIo* g_ModIo; | |||||
#endif // INCLUDED_MODIO | |||||
Not Done Inline ActionsIf that is a switch statement that is meant to handle everything, then not having a default clause (which NODEFAULT is with some additional code to ensure that at runtime) would cause certain compilers to emit a warning on a switch on an enum given the proper flags. leper: If that is a `switch` statement that is meant to handle everything, then not having a `default`… | |||||
Not Done Inline ActionsYes, so this is correct? Your sentence is not clear to me. Itms: Yes, so this is correct? Your sentence is not clear to me. | |||||
Not Done Inline ActionsSeems like it. leper: Seems like it. | |||||
Done Inline ActionsThis has absolutely not place in a header file, this is an implementation detail only. Leaking the structure of this, both means longer compile times on updates to purely internal structures, and could incite someone to use this outside of the implementation of this class. leper: This has absolutely not place in a header file, this is an implementation detail only. Leaking… | |||||
Done Inline ActionsShould probably be removed, once this has been understood and been implemented. If that does not seem sufficient then it should be rephrased into something that is not a TODO. leper: Should probably be removed, once this has been understood and been implemented. If that does… | |||||
Done Inline ActionsHas this been done? leper: Has this been done? | |||||
Not Done Inline ActionsAs I told you I read this with great interest. Since links stick out in editors I'm sure others have/will. Itms: As I told you I read this with great interest. Since links stick out in editors I'm sure others… | |||||
Not Done Inline ActionsThen I wonder why you answered a question about having downloaded more than the total and still going that way. leper: Then I wonder why you answered a question about having downloaded more than the total and still… | |||||
Done Inline ActionsShould this be a proper doxygen comment? leper: Should this be a proper doxygen comment? | |||||
Done Inline ActionsIf that handles all the cases, not having a default case is preferable since some modern compilers then emit warnings in case one is added, instead of having a runtime check like NODEFAULT. That said this function looks quite useless. leper: If that handles all the cases, not having a `default` case is preferable since some modern… | |||||
Not Done Inline ActionsI added that because of another warning I got, which I fixed differently. Itms: I added that because of another warning I got, which I fixed differently. | |||||
Not Done Inline ActionsWith (partially) manually assigned values for that enum one could do that with a single bit operation even. But that is just me thinking about how to make something shorter or nicer that should most likely not even exist in such a form. leper: With (partially) manually assigned values for that enum one could do that with a single bit… |
What is this doing in the header? That is an implementation detail only.