This repository has been archived by the owner on Jan 25, 2023. It is now read-only.
Path Class #2619
Open
Path Class #2619
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 9 commits
April 2, 2020 21:46
…ll use a separate NativeWide function in the engine as refactoring will be easier this way. Compiled, fixed some errors in variable names.
…ll runs. Bug fix for adding/removing trailing slashes.
…e fixed, along with the Samples. Tools are updated, though.
…with some things like setting the resource paths.
…h the new Path methods.
|
Forgot the URHO3D_EXTRAS, I will fix those and re-upload later. |
|
Marking this stale since there has been no activity for 30 days. |
| @@ -344,7 +344,7 @@ void Sample::HandleKeyDown(StringHash /*eventType*/, VariantMap& eventData) | |||
| Image screenshot(context_); | |||
| graphics->TakeScreenShot(screenshot); | |||
| // Here we save in the Data folder with date and time appended | |||
| screenshot.SavePNG(GetSubsystem<FileSystem>()->GetProgramDir() + "Data/Screenshot_" + | |||
| screenshot.SavePNG(GetSubsystem<FileSystem>()->GetProgramDir() + "Data/Screenshot_" + | |||
There was a problem hiding this comment.
Why padding is so big? Better to remove such trash from PRs
|
I still don't understand why need to put a string with a path inside another class. Even in C# Path just contains collection of static functions to manipulate with strings https://docs.microsoft.com/en-us/dotnet/api/system.io.path?view=net-5.0 |
1vanK
reviewed
Dec 6, 2020
| @@ -74,7 +74,7 @@ class URHO3D_API Script : public Object | |||
| /// Set whether to execute engine console commands as script code. | |||
| void SetExecuteConsoleCommands(bool enable); | |||
| /// Print the whole script API (all registered classes, methods and properties) to the log. No-ops when URHO3D_LOGGING not defined. | |||
| void DumpAPI(DumpMode mode = DOXYGEN, const String& sourceTree = String::EMPTY); | |||
| void DumpAPI(DumpMode mode = DOXYGEN, Path sourceTree = Path::EMPTY); | |||
| @@ -190,7 +190,7 @@ void Script::OutputAPIRow(DumpMode mode, const String& row, bool removeReference | |||
| } | |||
| } | |||
|
|
|||
| void Script::DumpAPI(DumpMode mode, const String& sourceTree) | |||
| void Script::DumpAPI(DumpMode mode, Path sourceTree) | |||
|
The code didn't get any easier with the new class. You just added conversions from strings and back to strings. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rather than using strings everywhere that are almost exclusively treated as paths, I am proposing we switch to a separate class for paths and for strings. At present, this can't be considered finished (especially since the Script bindings have not been added for the new class), but I would estimate that it is most of the way to being complete at the moment. I haven't tested it on platforms other than Linux and Web at present, so we'll see the results from the CI build for that. Probably some more work remains to be done with Serializables especially, even if it compiles and runs fine at present (implicit conversion from String to the rescue).
This has been discussed some on the forum already, though that thread has more of a focus on specifically allowing relative paths as opposed to switching to a Path class: https://discourse.urho3d.io/t/adding-relative-paths-to-resource-loading/5911/42
Some design choices should probably be considered before this is merged, including (though not limited to):
const Path& GetName() constor should we stick with what I have done at the present with two methodsconst String& GetName() constandconst Path& GetNamePath() const, though internally it is just stored as aPath.With that said, I know it's a long set of changes, but I would very much appreciate feedback from others.