Automatically create folder in project manager create and import

[nathanfranke]

Edit:

2023-09-07 - I am busy, but once I have time this will be my top priority PR.
2023-10-17 - I am working on this slowly 🙂
2023-10-18 - Rebased and happy, but should wait for 4.3 😃

---

Supersedes #49602
Fixes #60332
Fixes #74678
Resolves godotengine/godot-proposals#4749
Resolves godotengine/godot-proposals#2754
Partly r3solves godotengine/godot-proposals#7978

Behavior:

• When create_dir is enabled, browse selects the containing/parent directory, project name is appended.
• When create_dir is disabled, whether before or after browsing, browse selects the exact project folder.
• Directory naming convention is configurable. kebab-case (default), snake_case, etc.

Potential improvements:

• Behavior of capitalize() means test5 turns into test-5/. Fixed, now test5 --> test5/
• Persist/remember if create_dir is pressed. Would we need an editor setting?
• Maybe bug: horizontal scroll position of target path resets on change

Video:

2023-02-18.19-51-22_.mp4

Creation:

Import:

[timothyqiu]

When the project name is "Test" and "Create Folder" is on, project path is /path/to/dir/Test. If I then change the path to /path/to/dir/DoesNotExist and click "Create & Edit". It creates and uses the "DoesNotExist" folder for me.

I'm not sure if this is expected. If yes, would it be better to place the button somewhere else because it's now not "that" related to the project name as before.

Also, should "Create Folder" use a CheckBox instead of a CheckButton? Because the control does not have immediate effect (the folder is not immediately created).

To summarize:

• What the "Create Folder" button does was clear: Create a folder with the project name at the current project path, and use that folder as the new project path.
• The "Create Folder" toggle is confusing:
  • When toggled off: It strips the last component from project path no matter if it's the same as project name.
    1. Turn on
    2. Set the project path to /path/to/something/you/typed/later
    3. Turn off
    4. And it strips /later
  • When toggled on: It appends the project name to the project path, even when the current path does not exist.
    1. Set the project path to /path/that/does/not/exist
    2. Turn on
    3. The project path becomes /path/that/does/not/exist/ProjectName
    4. It still reports error because the parent directory does not exist (and won't be created).
  • When "Create & Edit" is pressed: create the project folder if missing when the toggle is on.

[nathanfranke]

When the project name is "Test" and "Create Folder" is on, project path is /path/to/dir/Test. If I then change the path to /path/to/dir/DoesNotExist and click "Create & Edit". It creates and uses the "DoesNotExist" folder for me.

This is the behavior I intended. Some users might want to make the directory name different from the project name (e.g. Card Game project name and card-game directory)

It be better to place the button somewhere else because it's now not "that" related to the project name as before.

I agree that moving the button somewhere else would make sense, but the only other place I can think of would be below the name/path fields, taking more vertical space.

Also, should "Create Folder" use a CheckBox instead of a CheckButton? Because the control does not have immediate effect (the folder is not immediately created).

I think a CheckButton works better because the button immediately changes the path field.

• What the "Create Folder" button does was clear: Create a folder with the project name at the current project path, and use that folder as the new project path.

• The "Create Folder" toggle is confusing:

  • When toggled off: It strips the last component from project path no matter if it's the same as project name.

    1. Turn on
    2. Set the project path to /path/to/something/you/typed/later
    3. Turn off
    4. And it strips /later

  • When toggled on: It appends the project name to the project path, even when the current path does not exist.

    1. Set the project path to /path/that/does/not/exist
    2. Turn on
    3. The project path becomes /path/that/does/not/exist/ProjectName
    4. It still reports error because the parent directory does not exist (and won't be created).

  • When "Create & Edit" is pressed: create the project folder if missing when the toggle is on.

I tried my best to make it intuitive and straightforward for most use-cases.

I've already considered changing the text on the button, but I couldn't decide on what to change it to.

Edit: I see two potential improvements, adding them to the root PR.

[AaronRecord]

I agree that moving the button somewhere else would make sense, but the only other place I can think of would be below the name/path fields, taking more vertical space.

It could go on the right side of an hbox with the "Project Path:" label.

[nathanfranke]

🎉

[Calinou]

@nathanfranke Thanks for adding this 🙂

Does it prevent creating the project? It should, as users should never be able to brick their Godot editor installation entirely.

Also, I'd rephrase it slightly:

Creating a project in the engine's working directory or the home directory is not allowed, as it would prevent the editor from starting.

The check should be performed on both $CWD and $HOME regardless of the current path of the engine binary, in case you're starting the engine from $CWD but you intend to install it for use with $HOME later on.

[nathanfranke]

Does it prevent creating the project?

Yes

How do I get the home directory?

[Calinou]

How do I get the home directory?

Probably like this:

#ifdef WINDOWS_ENABLED
	const String home_dir = OS::get_singleton()->get_environment("USERPROFILE")
#else
	const String home_dir = OS::get_singleton()->get_environment("HOME")
#endif

	if (home_dir != "") {
		// Do check.
	}

OS.get_environment() returns an empty string if the environment variable doesn't exist, so make sure to only perform the check if the string is non-empty.

See also

godot/editor/editor_settings.cpp

Line 472 in 1e553e3

const String fs_dir_default_project_path = OS::get_singleton()->has_environment("HOME") ? OS::get_singleton()->get_environment("HOME") : OS::get_singleton()->get_system_dir(OS::SYSTEM_DIR_DOCUMENTS);

.

[nathanfranke]

This is working great on my end and ready for review. Behavior is intuitive for me for every edge case, with the exception of one: Typing the project name as .. will put that same text .. in the path, which will almost definitely cause oddities. I traced the issue back to OS.get_safe_dir_name and made a PR to fix that behavior: #65755

To rephrase, this PR is complete, but PR #65755 should be merged first, and this PR retested, before this PR should be merged!

[nathanfranke]

This is ready for 4.1 if desired

[AThousandShips]

Just a minor nit

[AThousandShips]

I'd keep this along with the other variables, also let's keep the naming consistent and remove the _, this file doesn't use that for member variables (even though some files do)

[YuriSizov]

The code is somewhat difficult to review due to amount of changes, but I did some testing and overall it works correctly.

I second KoBeWi's concerns here, by the way. I have yet to look into this PR in detail, but why exactly the changes are so extensive for such an isolated feature? Can the changeset be reduced somehow?

Full disclosure, I need to work on the project manager, and this PR is going to get in the way. So I either have to accept it or ignore it. Making sure this PR doesn't exceed its own scope would be a great help.

[nathanfranke]

I am reworking this to reduce the changes, will take a few days.

[YuriSizov]

I am reworking this to reduce the changes, will take a few days.

Thanks, I appreciate it! I took a look over the weekend and I think it all pretty much makes sense and at least changes are isolated to one class, for the most part. Renames for clarity are also very nice to have. So at a glance I'm not sure what specifically can be simplified, but an attempt at that is very much welcome.

KoBeWi mentioned that perhaps the folder naming scheme feature can be cut, and I agree that it is probably not very necessary, but since it's already implemented then it makes sense to keep it.

[akien-mga]

This can be rebased / redone after #87266.

[fire]

Is "kebab-case" (default) the default for filenames? I thought it was snake_case?

[nathanfranke]

Right now, the default is kebab-case and it replaces underscores: t e_s-t → t-e-s-t
It would probably be better to keep underscores instead: t e_s-t → t-e_s-t

Could be implemented like this: dir_name = project_name.to_lower().replace(" ", "-")

[nathanfranke]

The default (kebab-case) is now implemented as to_lower().replace(" ", "-"). So underscores are preserved: t e_s-t → t-e_s-t

[nathanfranke]

Thanks for the patience, rebased and ready.

[akien-mga]

Tested locally, looks pretty good! It's a great usability improvement, I really disliked how this worked until now.

The code changes are pretty hard to review in depth, for future PRs please try to keep refactoring separate from feature work. There's a lot of changes here which are just moving things around or slightly improving code quality which make the functional changes challenging to review. But overall it seems ok to me.

[akien-mga]

Thanks! And thanks for your patience and getting this PR to the finish line!