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

Add OpenExternal, a function for opening files in the OS #4295

Merged
merged 1 commit into from
Mar 25, 2021

Conversation

ChrisJefferson
Copy link
Contributor

Add a function "OpenExternal", which opens files in other apps in the file system. While I like the names used by existing packages (like Splash), I also don't want to clash with those existing packages.

Rather than base this on the existing code used for help, I wrote a new function. This does remove the customizability, but also means we can open any file type. Also, in my experience, nowadays the "default" operating system method of opening files is what I want.

This also does not implement the functionality provided by (for example) semigroups, where files such as '.dot' and '.tex' files are compiled before being opened.

I'm very happy for suggestions on naming / functionality / etc. I thought having a PR would give a place to discuss such issues.

Discussed in #4272

Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

I like this feature, and I think OpenExternal is a decent name for it.

lib/streams.gd Outdated Show resolved Hide resolved
lib/streams.gd Outdated Show resolved Hide resolved
@wilfwilson wilfwilson added kind: new feature release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library labels Mar 16, 2021
@wilfwilson
Copy link
Member

I'd be happy to merge this, I just wanted to sanity check before doing so:

  • Have you tested the code yourself on the various operating systems? I've tried it on my Mac and I can confirm that it worked as expected.
  • Is it possible to add tests in GAP for OpenExternal? I presume such a test might be too brittle (and what would success even look like?) but if possible to do robustly, it could at least catch the case that open/explorer.exe/xdg-open... become renamed or whatever.

@fingolfin
Copy link
Member

I don't see a good way to test this automatically in our CI. So I'd just merge this as-is, but I'll wait to give a chance for more comments.

InstallGlobalFunction( OpenExternal, function(filename)
local file;
if ARCH_IS_MAC_OS_X() then
Exec(Concatenation("open \"",filename,"\""));
Copy link
Member

Choose a reason for hiding this comment

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

OK, I don't want to whine, but I have to repeat one of my pet mantras, namely that "Exec is a really bad function, we should never have added it like that". Besides not even allowing to check the return value, it also forces us to do silly quoting tricks like the above; and this is not even complete, a filename with a " in it will break it. We really should think about adding an alternative to Exec which is a bit better (doesn't go through a shell and hence has no quoting issues, and gives access to return value, and optionally allows for suppressing or redirecting output.

Copy link
Member

Choose a reason for hiding this comment

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

(But of course none of this is required for this PR!)

@wilfwilson wilfwilson merged commit 639f7b5 into gap-system:master Mar 25, 2021
@ChrisJefferson ChrisJefferson deleted the OpenExternal branch April 1, 2022 08:17
@fingolfin fingolfin changed the title Add OpenExternal, a function for opening files in the OS Add OpenExternal, a function for opening files in the OS Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: new feature release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants