You are once again using spaces instead of tabs, please set upclang-format to automate fixing this

@AThousandShips This is the new logic in which the environment variables are expanded. Is it acceptable?

static String _expand_windows_env_vars(const String &p_path) {String path = p_path;int from = 0;while ((from = path.find("%", from)) != -1) {int to = path.find("%", from + 1);if (to == -1) {break;}String var = path.substr(from + 1, to - from - 1);if (var.is_empty()) {from = to + 1;continue;}String value = OS::get_singleton()->get_environment(var);if (!value.is_empty()) {path = path.substr(0, from) + value + path.substr(to + 1);from += value.length();} else {from = to + 1;}}return path;}static bool _is_env_char(char32_t c) {return c == '_' || (c >= 'A' && c <= 'Z') ||       (c >= 'a' && c <= 'z') ||       (c >= '0' && c <= '9');}static String _expand_unix_env_vars(const String &p_path) {String path = p_path;int i = 0;while ((i = path.find("$", i)) != -1) {int start = i + 1;int end = start;while (end < path.length() && _is_env_char(path[end])) {end++;}if (end == start) {i++;continue;}String var = path.substr(start, end - start);String value = OS::get_singleton()->get_environment(var);if (!value.is_empty()) {path = path.substr(0, i) + value + path.substr(end);i += value.length();} else {i = end;}}return path;}String FileDialog::expand_path(const String &p_path) {    String path = p_path;    if (path.begins_with("~")) {#ifdef WINDOWS_ENABLED        String home = OS::get_singleton()->get_environment("USERPROFILE");#else        String home = OS::get_singleton()->get_environment("HOME");#endif        if (!home.is_empty()) {            path = home + path.substr(1);        }    }#ifdef WINDOWS_ENABLEDpath = _expand_windows_env_vars(path);#elsepath = _expand_unix_env_vars(path);#endifreturn path;}

Can't really speak to it, but easier to review if you just update the PR with it

You haven't declared it inos.h though, did you run this code or did you just forget to include that file in the push?

I forgot. Should I push that with the declaration inos.h or should I leave it to os_unix & os_windows?

The code won't work without it so you need to add it toos.h

The default method should probably just return the input string unchanged, but we'd need to check if any other platform needs it (that don't useOS_Unix as a base)

@AThousandShips Should I just declare inos.h & remove the declaration fromos_unix.h &os_windows.h? It may not cause any more errors?

No you should just addvirtual String expand_path(const String &p_path) const; toos.h and an implementation inos.cpp, orvirtual String expand_path(const String &p_path) const = 0; and implement it in all platforms where needed, or justvirtual String expand_path(const String &p_path) const { return p_path; } but I'd put it in the cpp file to make it easier to modify if needed

It should be separate for each OS as it's different in each, unless it doesn't need to be and both can useHOME I'm not familiar with this

No

I was thinking aboutvirtual String expand_path(const String &p_path) const override; as the main declaration inos.h because Windows & Unix are different (will still have to keep the logic apart, Linux usesHOME & Windows usesUSERPROFILE

I don't understand what you mean my suggestion is to addvirtual String expand_path(const String &p_path) const; toos.h andString OS::expand_path(const String &p_path) const { return p_path; } toos.cpp

Just add the code you forgot to include that works on your local machine and we can go from there

Please build the code locally to avoid pushing so many times with simple errors in the code and to make sure your PR actually works

Please build the code locally to avoid pushing so many times with simple errors in the code and to make sure your PR actually works

Is there a git command to push & not cause CI to run?

Yes you can add[no ci] to the commit message, but you really should test a PR, did you build this at all?

Yes you can add[no ci] to the commit message, but you really should test a PR, did you build this at all?

Yes, my build took some time but also failed. I'll start using[no ci] now.

Please just update your local branch, confirm it builds and fix any issues with it if it doesn't, and then push, no reason to push before you know it's actually correct

Please just update your local branch, confirm it builds and fix any issues with it if it doesn't, and then push, no reason to push before you know it's actually correct

Sorry, will do now.

This looks quite good! I have one concern though:
~ is, as far as I know, permitted in file names, on Windows it was used for trimmed names back in the 8.3 file name format restrictions, so for exampleFOOBARBAZ.TXT would becomeFOOBARB~.TXT, and from what I can find names can start with a~, and I can't find anything in POSIX about banning them, so I'd make the check check that it's a path starting with~/ (or~\\) to prevent~foo.txt being mangled

Leading~ is also regularly used for temporary lock files or backups etc.

Also are you still planning to add environment variable expansion support? Otherwise I'd remove it from the description and title

~ is, as far as I know, permitted in file names

It is, and not only on Windows (works on macOS as well). And it is permitted as the first (or only) character of the folder names. So it might cause issues with relative paths.

This looks quite good! I have one concern though:~ is, as far as I know, permitted in file names, on Windows it was used for trimmed names back in the 8.3 file name format restrictions, so for exampleFOOBARBAZ.TXT would becomeFOOBARB~.TXT, and from what I can find names can start with a~, and I can't find anything in POSIX about banning them, so I'd make the check check that it's a path starting with~/ (or~\\) to prevent~foo.txt being mangled

Leading~ is also regularly used for temporary lock files or backups etc.

Also are you still planning to add environment variable expansion support? Otherwise I'd remove it from the description and title

Thanks! Took some time but now works as intended.
Firstly, I didn't know that this could happen on Windows. I'll update the code to~/ &~\\ on Windows so it won't interfere with lockfiles, backups & etc (Can be done viaif (path.begins_with("~/") || path.begins_with("~\\")), I suppose? Unix will be updated to use~/ because~\\ is not supported)
Secondly, yes. Environment variables are on my mind so I think support will be added for them (If I'm positive I can't add support for them, I'll remove them)

@AThousandShips Should I also updateEditorFileDialog now?

Does that dialog need this? It is derived fromFileDialog so it should share the same code generally unless it's not in the shared code

Does that dialog need this? It is derived fromFileDialog so it should share the same code generally unless it's not in the shared code

No, you are correct. Same code is shared.

@bruvzg Can you test the logic on MacOS?

@AThousandShips If you are on Windows, can you test the logic?

I can try later when I have the time to build, but I don't think this should be ignored for CI unless you're going to update it more soon, assuming it compiles locally, assuming you are on Linux so you can test the Linux side

I can try later when I have the time to build, but I don't think this should be ignored for CI unless you're going to update it more soon, assuming it compiles locally, assuming you are on Linux so you can test the Linux side

I'll upload an MRP. Linux works fine, just have to confirm on Windows & MacOS. I'll not ignore the CI for the next change.

No MRP needed, I just need to build it and it'll take a little bit sinceos.h is changed, not serious but just gotta set aside the time

No MRP needed, I just need to build it and it'll take a little bit sinceos.h is changed, not serious but just gotta set aside the time

Alright, tell me if you encounter any problems/issues

Can you test the logic on MacOS?

Will check it tomorrow.