Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: Fix dragging and dropping files from compressed files into editor #97250

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

Garetonchick
Copy link
Contributor

Fixes #97128.

Use case described by @Maran23 in #97128 (comment) still doesn't work. I can look into it later.

@Garetonchick Garetonchick requested a review from a team as a code owner September 20, 2024 21:04
@Garetonchick Garetonchick force-pushed the windows-drag-and-drop-fix branch from cba9a5c to e8bac6d Compare September 20, 2024 21:46
@AThousandShips AThousandShips changed the title Windows drag and drop fix Windows: Fix dragging and dropping files from compressed files into editor Sep 21, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Sep 21, 2024
Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job, and congratulations on your first commit! I tested this and it does appear to work in regards to the issue I raised with 7zip. I'll approve this for now, but would you mind squashing the commits from this PR into a single commit? We generally prefer that convention when merging.

@Garetonchick Garetonchick force-pushed the windows-drag-and-drop-fix branch from 43ceb45 to 99840e9 Compare October 10, 2024 10:42
@Garetonchick Garetonchick requested a review from a team as a code owner October 10, 2024 10:42
@Garetonchick
Copy link
Contributor Author

Good job, and congratulations on your first commit! I tested this and it does appear to work in regards to the issue I raised with 7zip. I'll approve this for now, but would you mind squashing the commits from this PR into a single commit? We generally prefer that convention when merging.

Thank you! I have squashed the commits.

@bruvzg bruvzg requested review from bruvzg and removed request for RedMser October 11, 2024 12:19
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be working as expected.

Use case described by @Maran23 in #97128 (comment) still doesn't work. I can look into it later.

This likely requires handling CFSTR_FILECONTENTS and CFSTR_FILEDESCRIPTOR in addition to CF_HDROP.

@Garetonchick Garetonchick force-pushed the windows-drag-and-drop-fix branch from e7d32cf to 04140eb Compare October 16, 2024 11:20
@Garetonchick
Copy link
Contributor Author

Seems to be working as expected.

Use case described by @Maran23 in #97128 (comment) still doesn't work. I can look into it later.

This likely requires handling CFSTR_FILECONTENTS and CFSTR_FILEDESCRIPTOR in addition to CF_HDROP.

Added handling of CFSTR_FILEDESCRIPTOR and CFSTR_FILECONTENTS as you have suggested. Now drag-and-drop from inside the .zip file from Windows Explorer also works. Also I tested the same case with Total Commander and it works as well.

However, this time I had to do a bit of filesystem manipulations and couldn't find some functinality in Godot's source code. These are: joining filepaths with correct OS separator, creating temporary directory with random name, creating all subdirectories for a path and recursively removing directory with it's contents. For now I have written quick & dirty implementations of these, which are not general purpose, but happen to work here. So I would appreciate it if someone pointed me to the relevant existing functionality. Or if it doesn't exist, it probably should be added (maybe in a separate PR?) for general use.

@Garetonchick Garetonchick force-pushed the windows-drag-and-drop-fix branch 3 times, most recently from d8c29b9 to 490832a Compare October 16, 2024 19:26
@Garetonchick Garetonchick requested a review from bruvzg October 17, 2024 13:26
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (rebased on top of master 44fa552), it works as expected on Windows 11 23H2. Nested file structures work correctly, and so do file names containing Unicode characters.

Note that empty folders are not created if you drag a folder that contains empty folders to the Godot editor. (Even if you right-click it and use Show in File Manager, the folders won't be there.)

As mentioned in OP, using 7-zip's GUI (7ZFM.exe) will work, but using Windows Explorer won't. This applies regardless of the compression format.

@bruvzg
Copy link
Member

bruvzg commented Oct 22, 2024

These are: joining filepaths with correct OS separator, creating temporary directory with random name, creating all subdirectories for a path and recursively removing directory with it's contents.

There's DirAccess::erase_contents_recursive, DirAccess::make_dir_recursive and String::path_join (like all other public Godot IO functions, these use / separator, usually conversion is done internally right before passing path to the system API and never exposed to the user, see fix_path in OS_Windows).

static String fix_path(const String &p_path) {
String path = p_path;
if (p_path.is_relative_path()) {
Char16String current_dir_name;
size_t str_len = GetCurrentDirectoryW(0, nullptr);
current_dir_name.resize(str_len + 1);
GetCurrentDirectoryW(current_dir_name.size(), (LPWSTR)current_dir_name.ptrw());
path = String::utf16((const char16_t *)current_dir_name.get_data()).trim_prefix(R"(\\?\)").replace("\\", "/").path_join(path);
}
path = path.simplify_path();
path = path.replace("/", "\\");
if (path.size() >= MAX_PATH && !path.is_network_share_path() && !path.begins_with(R"(\\?\)")) {
path = R"(\\?\)" + path;
}
return path;
}

@Garetonchick Garetonchick force-pushed the windows-drag-and-drop-fix branch from 490832a to 2bd7599 Compare October 24, 2024 19:44
@Repiteo
Copy link
Contributor

Repiteo commented Nov 5, 2024

Do we want to wait and try to solve that edge-case, or is this ready to merge now in your eyes?

@Garetonchick
Copy link
Contributor Author

Do we want to wait and try to solve that edge-case, or is this ready to merge now in your eyes?

I think it's okay to merge. This edge case doesn't cause issues on my system (Windows 10), so I can't debug @Calinou's problem on Windows 11. I believe fixing it should be in a separate PR done by someone else.

@Repiteo Repiteo modified the milestones: 4.x, 4.4 Nov 6, 2024
@Repiteo Repiteo merged commit 6071c7c into godotengine:master Nov 6, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 6, 2024

Thanks! Congratulations on your first contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dragging and dropping files from 7zip into a Godot project fails.
7 participants