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

fix: mic_record script a bit for mac #949

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

swarnimarun
Copy link

Realised this demo was a bit broken while testing something irrelevant to the changes.

Also using all 3 names as iirc, OSX was what we used in 3.x. Though TBH not sure been a while since I used Godot 3. And I am not sure about the capitalisation part.

Realized this demo was a bit broken while testing something irrelevant to the changes.

Also using all 3 names as iirc, OSX was what we used in 3.x. Though TBH not sure been a while since I used Godot 3. And I am not sure about the capitalization part.
@@ -93,4 +93,8 @@ func _on_StereoCheckButton_toggled(button_pressed: bool) -> void:


func _on_open_user_folder_button_pressed():
OS.shell_open(ProjectSettings.globalize_path("user://"))
match OS.get_name():
"macOS", "MacOS", "OSX":
Copy link
Member

Choose a reason for hiding this comment

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

Only macOS is a valid name from OS here in 4.x AFAIK

@AThousandShips
Copy link
Member

What is the error you're seeing? Please provide some issue report for the reason behind this change

@swarnimarun
Copy link
Author

Aah, sorry, somehow thought it was obvious. So if you don't use file:// with shell open on mac you will get -50 perm error, attached an image example below.
image

@AThousandShips
Copy link
Member

AThousandShips commented Sep 17, 2023

That sounds like a bug to me, please open a report here

Also is this restricted only to macOS?

Comment on lines +96 to +100
match OS.get_name():
"macOS":
OS.shell_open("file://" + ProjectSettings.globalize_path("user://"))
_:
OS.shell_open(ProjectSettings.globalize_path("user://"))
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
match OS.get_name():
"macOS":
OS.shell_open("file://" + ProjectSettings.globalize_path("user://"))
_:
OS.shell_open(ProjectSettings.globalize_path("user://"))
if OS.get_name() == "macOS":
OS.shell_open("file://" + ProjectSettings.globalize_path("user://"))
else:
OS.shell_open(ProjectSettings.globalize_path("user://"))

Copy link
Author

Choose a reason for hiding this comment

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

@AThousandShips lmk if this would be better now that we only have one matcher

@swarnimarun
Copy link
Author

swarnimarun commented Sep 17, 2023

@AThousandShips yep this is only specific to MacOS where the resource URI is required for correct call.

And there is already an issue for it, godotengine/godot#52828, there was one even older than this(can't find that atm), I haven't been involved in Godot engine dev for a couple years so not sure why it hasn't been fixed but it should not be a big change, I could document a fix for engine there but I think the existing comments already make the issue quite clear.

@AThousandShips
Copy link
Member

AThousandShips commented Sep 17, 2023

Okay! I asked for an issue before and got no answer so that's why I was confused 🙃

However this should probably not be solved here but as suggested in the issue by fixing it locally

I'd say the behaviour of OS should be unform, you shouldn't have to make specific changes like these

@Calinou Calinou added the bug label Sep 17, 2023
@Calinou
Copy link
Member

Calinou commented Feb 7, 2024

@bruvzg Can we make it so we don't need to manually prefix file:// on macOS by modifying the engine's OS::shell_open() implementation?

This would prevent the need for this check in the project so the method can work in a cross-platform manner.

@bruvzg
Copy link
Member

bruvzg commented Feb 8, 2024

Can we make it so we don't need to manually prefix file:// on macOS by modifying the engine's OS::shell_open() implementation?

This would prevent the need for this check in the project so the method can work in a cross-platform manner.

What's the behavior on other platforms? It file:// is not required on Linux/Windows, I guess we can assume it's a file and auto add it if there's no URI schema in the shell_open argument.

@bruvzg
Copy link
Member

bruvzg commented Feb 9, 2024

Can we make it so we don't need to manually prefix file:// on macOS by modifying the engine's OS::shell_open() implementation?

godotengine/godot#88126

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

Successfully merging this pull request may close these issues.

None yet

4 participants