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

Implement and expose OS.shell_show_in_file_manager() #69698

Merged

Conversation

Daylily-Zeleen
Copy link
Contributor

@Daylily-Zeleen Daylily-Zeleen commented Dec 7, 2022

Implement 'OS::shell_show_in_file_manager()' for FilesystemDock and expose to GDScript.

Currently only implement for Windows, because I only have a windows device.
On other platforms, it will fallback to origin action.

I hope someone can implement for other platforms.

QQ.20220916183403.mp4

Replace #67071.

Fixed #64568.

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/show_in_explorer branch 2 times, most recently from 1e23af2 to 4bf69e0 Compare December 7, 2022 04:04
@Daylily-Zeleen Daylily-Zeleen marked this pull request as ready for review December 7, 2022 04:21
@Daylily-Zeleen Daylily-Zeleen requested review from a team as code owners December 7, 2022 04:21
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/show_in_explorer branch 2 times, most recently from 1201d28 to 691ef15 Compare December 7, 2022 05:16
@Chaosus Chaosus added this to the 4.x milestone Dec 7, 2022
@KoBeWi
Copy link
Member

KoBeWi commented Dec 14, 2022

I investigated the focus issue I mentioned before and it's caused by the editor. If the editor window is not focused while the command is executed, the folder correctly opens in front instead in background.

@kleonc
Copy link
Member

kleonc commented Dec 22, 2022

Does this PR fix #64568?

editor/filesystem_dock.cpp Outdated Show resolved Hide resolved
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/show_in_explorer branch 5 times, most recently from 3c43cda to 036b106 Compare December 23, 2022 09:58
@Daylily-Zeleen
Copy link
Contributor Author

Does this PR fix #64568?

Now fixed.

Fix.mp4

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/show_in_explorer branch 2 times, most recently from e0c8b73 to 0e7c980 Compare December 23, 2022 10:14
Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

Not sure if p_open_folder parameter is needed, in the codebase it seems we always do want to open the folder. But it might be nice to have it exposed even if it's not used internally. Fine for me.

Comment on lines 1816 to 1891
if (fpath == "res://") {
// This case means that right click at the spcace area in FilesystemDock.
// Enter resource folder directly.
OS::get_singleton()->shell_show_in_explorer(dir, true);
} else {
OS::get_singleton()->shell_show_in_explorer(dir, false);
}
Copy link
Member

Choose a reason for hiding this comment

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

Note that in such case the menu entry is "Open in File Manager", it's expected to open the folder instead of just showing it:
https://github.com/godotengine/godot/blob/0e7c98061856a171838f4ae41e2ca925158e24aa/editor/filesystem_dock.cpp#L2604

I don't think it needs to be changed.

Suggested change
if (fpath == "res://") {
// This case means that right click at the spcace area in FilesystemDock.
// Enter resource folder directly.
OS::get_singleton()->shell_show_in_explorer(dir, true);
} else {
OS::get_singleton()->shell_show_in_explorer(dir, false);
}
OS::get_singleton()->shell_show_in_explorer(dir, true);

editor/editor_file_dialog.cpp Outdated Show resolved Hide resolved
@kleonc
Copy link
Member

kleonc commented Dec 23, 2022

Fixed #64568.

Only the Windows part? There also #64568 (comment) in there (could be done later/separately, asking just for confirmation).

@Daylily-Zeleen
Copy link
Contributor Author

Fixed #64568.

Only the Windows part? There also #64568 (comment) in there (could be done later/separately, asking just for confirmation).

Yes, I have not linux device, I can't be responsible for that。

@bruvzg
Copy link
Member

bruvzg commented Dec 23, 2022

Only the Windows part? There also #64568 (comment) in there (could be done later/separately, asking just for confirmation).

Also, macOS version for the reference - #65890 (comment)

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/show_in_explorer branch from 0e7c980 to b55a739 Compare January 21, 2023 16:54
doc/classes/OS.xml Outdated Show resolved Hide resolved
@YeldhamDev
Copy link
Member

YeldhamDev commented Apr 23, 2023

Wouldn't be better for the function's name to be shell_show_in_file_manager() instead? Since the editor references it as "File Manager", and it less vague/Windows-centric than just "explorer" (and yes, I know this PR only implements it for Windows for now, but I'm quite sure other OSes will be added by others in the future).

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/show_in_explorer branch from 49aab49 to c9fa01d Compare April 24, 2023 02:59
@Daylily-Zeleen
Copy link
Contributor Author

I tested this PR again and the problem with folder opening in background seems to be gone.

There is however another issue. When you right-click a file, "Show in File Manager" option shows. When you click it, it opens the containing folder and selects the file -> this is correct When you right-click a folder, "Open in File Manager" option shows. When you click it, it does the same as if it was file -> this is wrong.

"Open in File Manager" should open the folder, not the parent folder.

EDIT: Seems like kleonc already commented about it.

@KoBeWi @kleonc I noticed that my implement is the same as the VSCode's behavior of "show in file manager". However, UE editor's behavior is the same as your description.
It seems this different is not important, right?
But if your are sure to enter a folder directly is better, I will modify my code. Please give me a definite reply.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 24, 2023

Well, the option does say "Open in File Explorer" when you right-click directory. It has different text. So either it should be made the same or maybe both could be available.

But IMO it's better to keep the current behavior, i.e. opening the folder. Selecting a folder is much less common operation and also makes it impossible to navigate to empty folders.

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/show_in_explorer branch from c9fa01d to a3ee288 Compare April 24, 2023 12:22
@Daylily-Zeleen
Copy link
Contributor Author

Daylily-Zeleen commented Apr 24, 2023

Well, the option does say "Open in File Explorer" when you right-click directory. It has different text. So either it should be made the same or maybe both could be available.

But IMO it's better to keep the current behavior, i.e. opening the folder. Selecting a folder is much less common operation and also makes it impossible to navigate to empty folders.

I approve your opinion, my editor local language put the verb at the end of this option, and I never noticed the diffrenet between right-click file and folder.
Now I have changed the logic to open folder directly.

doc/classes/OS.xml Outdated Show resolved Hide resolved
doc/classes/OS.xml Outdated Show resolved Hide resolved
doc/classes/OS.xml Outdated Show resolved Hide resolved
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/show_in_explorer branch from a3ee288 to 8ed1147 Compare April 25, 2023 02:57
@YeldhamDev YeldhamDev changed the title Implement and expose OS::shell_show_in_explorer() Implement and expose OS::shell_show_in_file_manager() Apr 25, 2023
Copy link
Member

@YeldhamDev YeldhamDev left a comment

Choose a reason for hiding this comment

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

The commit message should be changed to reflect the function's current name.

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/show_in_explorer branch from 8ed1147 to b12ced0 Compare April 25, 2023 03:30
@akien-mga akien-mga merged commit 76d33d1 into godotengine:master Apr 25, 2023
@akien-mga
Copy link
Member

Thanks!

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.

"Show in File Manager" from context menu does nothing if path contains a # character
7 participants