This repository was archived by the owner on Jan 25, 2023. It is now read-only.
Repository navigation
Path Class - #2619
Open
SirNate0 wants to merge 9 commits into
Open
Path Class#2619SirNate0 wants to merge 9 commits into
SirNate0 wants to merge 9 commits into
Conversation
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.
Contributor
Author
|
Forgot the URHO3D_EXTRAS, I will fix those and re-upload later. |
|
Marking this stale since there has been no activity for 30 days. |
| 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_" + |
Contributor
There was a problem hiding this comment.
Why padding is so big? Better to remove such trash from PRs
Contributor
|
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
| 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); |
| } | ||
|
|
||
| void Script::DumpAPI(DumpMode mode, const String& sourceTree) | ||
| void Script::DumpAPI(DumpMode mode, Path sourceTree) |
Contributor
|
The code didn't get any easier with the new class. You just added conversions from strings and back to strings. |
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.