Changeset View
Standalone View
source/ps/scripting/JSInterface_VFS.cpp
Show All 21 Lines | |||||
#include "ps/CLogger.h" | #include "ps/CLogger.h" | ||||
#include "ps/CStr.h" | #include "ps/CStr.h" | ||||
#include "ps/Filesystem.h" | #include "ps/Filesystem.h" | ||||
#include "scriptinterface/ScriptVal.h" | #include "scriptinterface/ScriptVal.h" | ||||
#include "scriptinterface/ScriptInterface.h" | #include "scriptinterface/ScriptInterface.h" | ||||
#include "ps/scripting/JSInterface_VFS.h" | #include "ps/scripting/JSInterface_VFS.h" | ||||
#include "lib/file/vfs/vfs_util.h" | #include "lib/file/vfs/vfs_util.h" | ||||
// Only allow engine compartments to read files they may be concerned about. | |||||
#define PathRestriction_GUI {L""} | |||||
wraitii: Think you should put this above the function definition rather than at the top. Also constexpr… | |||||
Not Done Inline ActionsShouldn't nonlocal variables always be mentioned at the top of the file? elexis: Shouldn't nonlocal variables always be mentioned at the top of the file?
I'm not familiar with… | |||||
Not Done Inline ActionsWell we declare globals at the top, but this is eminently local. In fact it should be static const, and put somewhere around the macro imo. "static" would make it unavailable to other compilation units. I don't have a complete mastery of constexpr admittedly, so I'll link http://en.cppreference.com/w/cpp/language/constexpr, but my understanding is that it's supposed to tell the compiler that this can be evaluated at compile-time, which is much stronger than const. Now in this case I'm actually wrong as none of this is a literal type so it wouldn't compile, but it'd still be better if possible, as the compiler could inline/precompute it all. Might be possible come C++20 or so. wraitii: Well we declare globals at the top, but this is eminently local. In fact it should be static… | |||||
Not Done Inline ActionsConstants at the top also help documenting the concepts used by the code that follows and configuration before being confronted with code. Are(n't) C++ compilers we care about are smart enough to evaluate at compile time that a constant map of constant vectors can be inlined? Correct, static was missing. elexis: Constants at the top also help documenting the concepts used by the code that follows and… | |||||
Not Done Inline Actions
Inlined, probably, but "completely taken apart and basically meta-programmed in", probably not. I still think it should go above/inside the function where it's used, but it's not a huge thing. wraitii: > Are(n't) C++ compilers we care about are smart enough to evaluate at compile time that a… | |||||
Not Done Inline ActionsThe last word wasnt spoken in this file anyway. I'm eager to see if this file can be come agnostic of the scripts afterwards. elexis: The last word wasnt spoken in this file anyway. I'm eager to see if this file can be come… | |||||
#define PathRestriction_Simulation {L"simulation/"} | |||||
#define PathRestriction_Maps {L"simulation/", L"maps/"} | |||||
Not Done Inline ActionsCould also be simulation/data/. Depends on whether we wish to enforce our will to only use that subdirectory on mods and future vanilla developers. Doesn't really seem needed to manifest it here. elexis: Could also be `simulation/data/`. Depends on whether we wish to enforce our will to only use… | |||||
// shared error handling code | // shared error handling code | ||||
#define JS_CHECK_FILE_ERR(err)\ | #define JS_CHECK_FILE_ERR(err)\ | ||||
/* this is liable to happen often, so don't complain */\ | /* this is liable to happen often, so don't complain */\ | ||||
if (err == ERR::VFS_FILE_NOT_FOUND)\ | if (err == ERR::VFS_FILE_NOT_FOUND)\ | ||||
{\ | {\ | ||||
return 0; \ | return 0; \ | ||||
}\ | }\ | ||||
/* unknown failure. We output an error message. */\ | /* unknown failure. We output an error message. */\ | ||||
▲ Show 20 Lines • Show All 135 Lines • ▼ Show 20 Lines | while (std::getline(ss, line)) | ||||
JS::RootedValue val(cx); | JS::RootedValue val(cx); | ||||
ScriptInterface::ToJSVal(cx, &val, CStr(line).FromUTF8()); | ScriptInterface::ToJSVal(cx, &val, CStr(line).FromUTF8()); | ||||
JS_SetElement(cx, line_array, cur_line++, val); | JS_SetElement(cx, line_array, cur_line++, val); | ||||
} | } | ||||
return JS::ObjectValue(*line_array); | return JS::ObjectValue(*line_array); | ||||
} | } | ||||
JS::Value JSI_VFS::ReadJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& filePath) | JS::Value JSI_VFS::ReadJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::vector<CStrW>& validPaths, const CStrW& filePath) | ||||
{ | { | ||||
if (!PathRestrictionMet(pCxPrivate, validPaths, filePath)) | |||||
return JS::NullValue(); | |||||
JSContext* cx = pCxPrivate->pScriptInterface->GetContext(); | JSContext* cx = pCxPrivate->pScriptInterface->GetContext(); | ||||
JSAutoRequest rq(cx); | JSAutoRequest rq(cx); | ||||
JS::RootedValue out(cx); | JS::RootedValue out(cx); | ||||
pCxPrivate->pScriptInterface->ReadJSONFile(filePath, &out); | pCxPrivate->pScriptInterface->ReadJSONFile(filePath, &out); | ||||
return out; | return out; | ||||
} | } | ||||
void JSI_VFS::WriteJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& filePath, JS::HandleValue val1) | void JSI_VFS::WriteJSONFile(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& filePath, JS::HandleValue val1) | ||||
{ | { | ||||
JSContext* cx = pCxPrivate->pScriptInterface->GetContext(); | JSContext* cx = pCxPrivate->pScriptInterface->GetContext(); | ||||
JSAutoRequest rq(cx); | JSAutoRequest rq(cx); | ||||
Not Done Inline ActionsIf would have been nicer to abstract these functions. One wrapper per context per function is about a dozen of lines each. Not having to require one wrapper for each function by using some C++ magic (macros? templates? classes?) would yield less repetition, less to read, less to maintain, less surface for inconsistency bugs. But such voodoo can still be done later. Also we don't want another NativeWrapperDecls.h. elexis: If would have been nicer to abstract these functions.
The next patch after this one should… | |||||
// TODO: This is a workaround because we need to pass a MutableHandle to StringifyJSON. | // TODO: This is a workaround because we need to pass a MutableHandle to StringifyJSON. | ||||
JS::RootedValue val(cx, val1); | JS::RootedValue val(cx, val1); | ||||
std::string str(pCxPrivate->pScriptInterface->StringifyJSON(&val, false)); | std::string str(pCxPrivate->pScriptInterface->StringifyJSON(&val, false)); | ||||
VfsPath path(filePath); | VfsPath path(filePath); | ||||
WriteBuffer buf; | WriteBuffer buf; | ||||
buf.Append(str.c_str(), str.length()); | buf.Append(str.c_str(), str.length()); | ||||
g_VFS->CreateFile(path, buf.Data(), buf.Size()); | g_VFS->CreateFile(path, buf.Data(), buf.Size()); | ||||
} | } | ||||
void JSI_VFS::RegisterReadOnlyScriptFunctions(const ScriptInterface& scriptInterface) | bool JSI_VFS::PathRestrictionMet(ScriptInterface::CxPrivate* pCxPrivate, const std::vector<CStrW>& validPaths, const CStrW& filePath) | ||||
{ | |||||
for (const CStrW& validPath : validPaths) | |||||
if (filePath.find(validPath) == 0) | |||||
Not Done Inline ActionsThis should definitely be a starts-with of some kind. wraitii: This should definitely be a starts-with of some kind. | |||||
Not Done Inline ActionsI won't be using boost elexis: I won't be using boost | |||||
Not Done Inline ActionsRight, I think that's either C++17 or C++20, still it should behave like one. Otherwise you can just name your JSON file "whaterversimulation/.json" and it's going to be loadable from anywhere. wraitii: Right, I think that's either C++17 or C++20, still it should behave like one. Otherwise you can… | |||||
Not Done Inline ActionsHad misread this as != end() initially, so never mind on the above, we're good to go. wraitii: Had misread this as != end() initially, so never mind on the above, we're good to go. | |||||
Not Done Inline Actionsnp elexis: np | |||||
return true; | |||||
CStrW allowedPaths; | |||||
for (std::size_t i = 0; i < validPaths.size(); ++i) | |||||
{ | |||||
if (i != 0) | |||||
allowedPaths += L", "; | |||||
allowedPaths += L"\"" + validPaths[i] + L"\""; | |||||
} | |||||
Not Done Inline ActionsThe above is one of those cases where C++ is pretty ugly… wraitii: The above is one of those cases where C++ is pretty ugly…
This might be a really terrible idea… | |||||
Not Done Inline ActionsWhy? elexis: Why? | |||||
Not Done Inline Actionscause then it's like allowedPaths.map(x=> '"' + x + '"').join(",") wraitii: cause then it's like
```
allowedPaths.map(x=> '"' + x + '"').join(",")
``` | |||||
Not Done Inline ActionsA string concatenation in JS is nicer than in C++. elexis: A string concatenation in JS is nicer than in C++.
But doing so here comes with additional… | |||||
Not Done Inline ActionsTrue. Well it was just a dumb suggestion anyways. wraitii: True. Well it was just a dumb suggestion anyways. | |||||
JS_ReportError( | |||||
pCxPrivate->pScriptInterface->GetContext(), | |||||
"This part of the engine may only read from: %s!", | |||||
utf8_from_wstring(allowedPaths).c_str()); | |||||
return false; | |||||
} | |||||
#define VFS_ScriptFunctions(context)\ | |||||
JS::Value Script_ReadJSONFile_##context(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& filePath)\ | |||||
{\ | |||||
return JSI_VFS::ReadJSONFile(pCxPrivate, PathRestriction_##context, filePath);\ | |||||
} | |||||
VFS_ScriptFunctions(GUI); | |||||
VFS_ScriptFunctions(Simulation); | |||||
VFS_ScriptFunctions(Maps); | |||||
#undef VFS_ScriptFunctions | |||||
void JSI_VFS::RegisterScriptFunctions_GUI(const ScriptInterface& scriptInterface) | |||||
Not Done Inline ActionsThis function doesn't need to be part of JSI_VFS, move it above ReadJSonFile and it can exist only in this cpp file. wraitii: This function doesn't need to be part of JSI_VFS, move it above ReadJSonFile and it can exist… | |||||
Not Done Inline Actionsdoable elexis: doable | |||||
{ | { | ||||
scriptInterface.RegisterFunction<JS::Value, std::wstring, std::wstring, bool, &JSI_VFS::BuildDirEntList>("BuildDirEntList"); | scriptInterface.RegisterFunction<JS::Value, std::wstring, std::wstring, bool, &JSI_VFS::BuildDirEntList>("BuildDirEntList"); | ||||
scriptInterface.RegisterFunction<bool, CStrW, JSI_VFS::FileExists>("FileExists"); | scriptInterface.RegisterFunction<bool, CStrW, JSI_VFS::FileExists>("FileExists"); | ||||
scriptInterface.RegisterFunction<double, std::wstring, &JSI_VFS::GetFileMTime>("GetFileMTime"); | scriptInterface.RegisterFunction<double, std::wstring, &JSI_VFS::GetFileMTime>("GetFileMTime"); | ||||
scriptInterface.RegisterFunction<unsigned int, std::wstring, &JSI_VFS::GetFileSize>("GetFileSize"); | scriptInterface.RegisterFunction<unsigned int, std::wstring, &JSI_VFS::GetFileSize>("GetFileSize"); | ||||
scriptInterface.RegisterFunction<JS::Value, std::wstring, &JSI_VFS::ReadFile>("ReadFile"); | scriptInterface.RegisterFunction<JS::Value, std::wstring, &JSI_VFS::ReadFile>("ReadFile"); | ||||
scriptInterface.RegisterFunction<JS::Value, std::wstring, &JSI_VFS::ReadFileLines>("ReadFileLines"); | scriptInterface.RegisterFunction<JS::Value, std::wstring, &JSI_VFS::ReadFileLines>("ReadFileLines"); | ||||
scriptInterface.RegisterFunction<JS::Value, std::wstring, &ReadJSONFile>("ReadJSONFile"); | scriptInterface.RegisterFunction<JS::Value, std::wstring, &Script_ReadJSONFile_GUI>("ReadJSONFile"); | ||||
scriptInterface.RegisterFunction<void, std::wstring, JS::HandleValue, &WriteJSONFile>("WriteJSONFile"); | |||||
} | } | ||||
void JSI_VFS::RegisterWriteScriptFunctions(const ScriptInterface& scriptInterface) | void JSI_VFS::RegisterScriptFunctions_Simulation(const ScriptInterface& scriptInterface) | ||||
{ | { | ||||
scriptInterface.RegisterFunction<void, std::wstring, JS::HandleValue, &WriteJSONFile>("WriteJSONFile"); | scriptInterface.RegisterFunction<JS::Value, std::wstring, &Script_ReadJSONFile_Simulation>("ReadJSONFile"); | ||||
} | |||||
void JSI_VFS::RegisterScriptFunctions_Maps(const ScriptInterface& scriptInterface) | |||||
{ | |||||
scriptInterface.RegisterFunction<JS::Value, std::wstring, std::wstring, bool, &JSI_VFS::BuildDirEntList>("BuildDirEntList"); | |||||
scriptInterface.RegisterFunction<bool, CStrW, JSI_VFS::FileExists>("FileExists"); | |||||
scriptInterface.RegisterFunction<JS::Value, std::wstring, &Script_ReadJSONFile_Maps>("ReadJSONFile"); | |||||
Done Inline ActionsThe context dependent functions could be moved to the functions, but the GUI context ScriptFunctions.cpp looks much nicer with only having one line each there. elexis: The context dependent functions could be moved to the functions, but the GUI context… | |||||
} | } |
Think you should put this above the function definition rather than at the top. Also constexpr perhaps? As we're going to drop VS13 soon