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

classlib: String:openOS escaping #5322

Merged
merged 1 commit into from
Mar 4, 2022
Merged

Conversation

elgiano
Copy link
Contributor

@elgiano elgiano commented Jan 15, 2021

Purpose and Motivation

Fixes #2297.
String:openOS calls systemCmd, resulting in:

This PR switches to SequenceableCollection:unixCmd, as suggested by @nhthn, fixing both problems at once.
I've changed implementations for Linux and macOS, but not for Windows, which I don't understand and can't test.

Types of changes

  • Bug fix
  • New feature
  • Breaking change?
    could anyone rely on openOS being blocking? I don't see it very likely, as openOS is not a file-chooser and doesn't return anything.

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review

@dyfer
Copy link
Member

dyfer commented Jan 15, 2021

In my experience open on macOS is not blocking, i.e. open itself returns right away. So should it still be changed?
(the escaping should be fixed of course)

@elgiano
Copy link
Contributor Author

elgiano commented Jan 15, 2021

IMO there are still two reasons to change it: macOS users would still gain better escaping that now, and SC codebase would be a bit more consistent

@jamshark70
Copy link
Contributor

Agreed that there is no reason to use systemCmd here -- but I have never seen openOS block the language until the window is closed on any platform (in multiple class sessions with students in Mac and Windows, and Linux on my own machine). The open, xdg-open and start processes return practically instantaneously.

AFAICS the format of the Windows implementation is not substantially different from the others -- it appears to require an additional title argument but otherwise similar. So I guess it could be fixed simply by ["start", "SuperCollider", this].unixCmd...? I could try that later.

@elgiano
Copy link
Contributor Author

elgiano commented Jan 16, 2021

but I have never seen openOS block the language until the window is closed on any platform

I don't know why, but on my system (running Arch Linux), xdg-open returns immediately if my file or web browser was already running. If it has to be started anew (i.e. I don't have any file browser already open), it blocks until that process exits.

This made me realize that if we go for unixCmd we loose the exit code, which we can get asynchronously with an action argument. It would make sense IMO, but what do you think?

I'm adding action and the windows implementation now :)

@dyfer
Copy link
Member

dyfer commented Feb 1, 2021

Hmmm...
I was toying with the idea of an action for openOS some time ago in my mind, with the assumption that the open etc commands do return immediately, so effectively the exit code would need to be obtained by wrapping a command in a script with some custom piping... or something like that. I thought that's too extensive and difficult to implement reliably and gave up.
What would be the use of an exit code from the open (et al) command?
IMO an action and exit code are useful only if they can be obtained somewhat reliably an all platforms, in all cases, and for the actual command being run, not for start/open/xdg-open. As it is now, on macOS (and Windows?) the command always returns immediately, and from your observations on Linux it does so under certain conditions...

@joshpar joshpar self-assigned this Feb 20, 2022
@joshpar joshpar added this to the 3.13.0 milestone Feb 20, 2022
@joshpar
Copy link
Member

joshpar commented Feb 25, 2022

@elgiano and @dyfer - I feel like the discussion here wasn't really resolved. Do either of you feel this should still be pulled in?

@dyfer
Copy link
Member

dyfer commented Feb 27, 2022

After looking at this again - I think that both the escaping fix and changing to .unixCmd is fine (I don't think the blocking was expected in any case), but I'm not sure we want to add the action, as it (most likely) has no connection to the actual command being run in most cases, outside of the singular case described by @elgiano

@elgiano
Copy link
Contributor Author

elgiano commented Mar 2, 2022

Looking at it again too, I also don't see it necessary to add an action. openOS is not meant to run a program, but rather as a shortcut to call the OS to open a file or URL. As such, I also agree it's better to leave it clean without unnecessary options, and redirect more advanced use cases to unixCmd itself.

I've edited and rebased the branch, we're ready to merge for me.

@dyfer
Copy link
Member

dyfer commented Mar 2, 2022

Thanks @elgiano !
I tested on macOS, it works as expected.
Just a small question about code style - is there a reason you included () after .unixCmd?

@elgiano
Copy link
Contributor Author

elgiano commented Mar 2, 2022

Just a small question about code style - is there a reason you included () after .unixCmd?

Not at all, just confused between code-styles :) thanks for catching that, I also added semicolons at the end

@dyfer
Copy link
Member

dyfer commented Mar 2, 2022

Did you happen to test on Windows? I don' have a Windows machine handy right at this moment.

@dyfer dyfer changed the title classlib: String:openOS escaping and non-blocking classlib: String:openOS escaping Mar 2, 2022
@dyfer
Copy link
Member

dyfer commented Mar 2, 2022

Also, I've changed the title to reflect the current state of the PR. Maybe it wouldn't hurt to remove/cross out the parts about blocking from the first comment so that it's clear if someone looks at this in the future.

@elgiano
Copy link
Contributor Author

elgiano commented Mar 2, 2022 via email

@dyfer
Copy link
Member

dyfer commented Mar 2, 2022

Sounds good, I'll test later today and will report back.

Copy link
Member

@dyfer dyfer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @elgiano !

@dyfer dyfer merged commit ec2dedf into supercollider:develop Mar 4, 2022
@elgiano elgiano deleted the topic/openOS branch March 4, 2022 11:19
@dyfer
Copy link
Member

dyfer commented May 19, 2022

Using an older build I just encountered an issue with openOS not escaping ' in paths. I'm glad this is now fixed!

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.

character "&" and .openOS works only in Windows
4 participants