-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[WIP] Add macOS Terminal mark to tty prompts #11516
Conversation
@@ -156,6 +162,10 @@ def msg(message, *args, **kwargs): | |||
|
|||
|
|||
def info(message, *args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should really combine tty.msg
and tty.info
. Does anyone know why they both exist?
lib/spack/llnl/util/tty/__init__.py
Outdated
@@ -24,6 +26,7 @@ | |||
_warn_enabled = True | |||
_error_enabled = True | |||
indent = " " | |||
_osascript = os.path.exists('/usr/bin/osascript') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this lazy (i.e., wrap it in a function call) so that it is not called unconditionally on module import. Doing lots of things in module scope slows down Spack startup.
@@ -156,6 +164,11 @@ def msg(message, *args, **kwargs): | |||
|
|||
|
|||
def info(message, *args, **kwargs): | |||
if _osascript: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap this whole thing in a function so the code's not duplicated across all the subprocess.call
invocations. And give the function a nice docstring to explain what's going on (I had to google to remember that osascript
is the thing that runs AppleScripts)
if _osascript: | ||
subprocess.call(['osascript', '-e', 'if app "Terminal" is frontmost ' | ||
'then tell app "System Events" to keystroke "u" ' | ||
'using command down']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three things:
-
I am not sure that
if app "Terminal" is frontmost
is the best condition here. What if terminal is in the background while the output is spewing, but you want toctrl-up
after the fact? What if you're in xTerm or iTerm, but Terminal is in front (for whatever reason)?What you really want to know is whether this
spack
instance was launched from the Terminal, and you can get that by checking theTERM_PROGRAM
env var. In Apple Terminal it is set like this:(spackbook):~$ env | grep TERM_PROGRAM TERM_PROGRAM=Apple_Terminal
Does iTerm support this too? In iTerm it is set like this:
(spackbook):~$ env | grep TERM_PROGRAM TERM_PROGRAM=iTerm.app
I think you can just check for those, then have the apple script unconditionally talk to "System Events".
-
The other thing that you likely need to factor in here is whether the output is going to
stdout
. Have you tried this withspack install
without-v
? I think you should maybe think about that, though maybe just doing it inmsg
andinfo
is enough. -
Are there markers that need to be suppressed with the terminal is not interactive? What if the user runs this and sends output to a file
spack install -v hdf5 > log
(people do this). You should probably only print out these markerssys.stdout.isatty()
returnsTrue
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for if app "Terminal" is frontmost
might be that it causes the Terminal to steal focus. I tried asking what the purpose was and got a response at the bottom of https://scriptingosx.com/2017/03/terminal-the-marks-the-spot/. I'll have to play around with this and confirm. I'll see what happens when the output is not verbose or is piped to a log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some observations:
- Even if I remove
if app "Terminal" is frontmost
, it still doesn't add mark characters to the Terminal if I'm not currently focused on the Terminal. Luckily, it doesn't steal focus, but it does sound the terminal bell, which is annoying... - This doesn't seem to be an issue. I believe the
osascript
command is still run, and you can see that it adds a mark before the next==>
is displayed, but it doesn't break anything or add duplicate marks. Probably worth checking if verbose though. - This doesn't seem to be an issue. No meta characters are added to the log, although it has the same minor side-effect as 2.
I'm going to see if I can find a way to fix 1. It would be great if we could always add a mark character even if the Terminal is in the background. I also want to disable that bell...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posted on StackOverflow: https://stackoverflow.com/questions/56262815/add-mark-to-terminal-in-background-using-applescript
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure that if you remove if app "Terminal" is frontmost then
(i.e. only have tell app "System Events" to keystroke "u" using command down
) then it will issue the keystroke to whatever app is currently active, causing who knows what. As a side note, I suspect that if one changes the shortcut then the AppleScript won't work—it might be more robust to script clicking the menu instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another important test case will be what happens when multiple Terminal windows are open.
@@ -26,7 +26,7 @@ | |||
_warn_enabled = True | |||
_error_enabled = True | |||
indent = " " | |||
_osascript = os.path.exists('/usr/bin/osascript') | |||
_osascript = subprocess.call(['which', 'osascript']) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much cleaner. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamjstewart @tldahlgren: only thing about this code is that it depends on which
, and I kind of want to avoid having external command dependencies in this code.
The rest of spack uses spack.util.executable.which
for things like this, but that returns a spack.util.executable.Executable
, which is (kind of) spack-specific.
In #11245 I pulled out which_string
from which()
, that searches the path and gives you a str
instead of an Executable
. How about we pull that in as llnl.util.filesystem.which
, and use it instead of shelling out to which
?
Alternately, spack.util.executable
is not THAT spack-specific. The only thing that you'd have to change to move it to llnl.util
would be to rethink ProcessError
, which extends SpackError
, which a lot of parts of Spack use for error handling. Probably not worth dealing with that now -- but it's something we could do later. Just using which_string
is probably fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use which_string
because spack.util.executable
imports llnl.util.tty
. We would either have to reimplement it in tty
or remove the tty
imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant move the function to llnl.util.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still thinking about merging llnl
and spack
? Also, did you have any opinions on merging tty.msg
and tty.info
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at the moment, though I have thought about making some things in llnl.util into proper Python packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can merge info
into msg
. I have no recollection at this point of why they're different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want to keep info
or msg
? I'm leaning towards info
since it's more inline with https://docs.python.org/3/library/logging.html#logging-levels. Also, should we have a command-line or config option to enable/disable marks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, tty.verbose
doesn't seem to be used anywhere? Is this correct?
Another possible way to do things is to create a separate tell application "System Events"
tell process "Terminal"
click menu item "Mark" of menu "Marks" of menu item "Marks" of menu "Edit" of menu bar item "Edit" of menu bar 1
end tell
end tell Then the script can be called like so: _script = os.path.join(os.path.dirname(__file__), 'mark.scpt')
subprocess.call(['osascript', _script]) This has the benefit that it doesn't send a keystroke to the current application, so it doesn't sound a bell when the application suddenly switches. However, it still doesn't add a mark if the Terminal is in the background. |
I just noticed that the |
That seems to be because of the "Automatically Mark Prompt Lines" option, which is enabled by default. I'm not sure what mechanism is used to detect prompt lines. |
Have you ever stared at the verbose output of a Spack build and lost track of where you were in the build process? Have you ever wished you could simply jump to the next stage in your terminal and see how the tests are progressing? With this PR, the future is now!
Simply use the macOS Terminal mark feature to jump between prompts using Cmd+Up/Down. See below .gif for a demonstration.
This works by calling the
osascript
command on macOS to insert a mark. Credits to Armin Briegel for figuring out how to do this.