-
Notifications
You must be signed in to change notification settings - Fork 17
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 Re-enabling manual Style creation #47
Conversation
@fdncred To save you time spent reviewing: Added another commit which introduces test code for detecting similar breaking changes in the future. |
e8b5f60
to
b3f9ba6
Compare
This would take some work. I'd have to compile nushell with this PR, write a nushell command to respond to your new hyperlink command, and call it from my script. Is there maybe an easier way to do this? Maybe I can just tweak the example and call it?
I tried iterm2, wezterm, and terminal.app and couldn't get any of them to respond properly to cwd |
You could modify the hyperlink example to only output the hyperlink (no other content in the format string) and not sleep. Then, at least in a new bash shell (e.g. run bash from within your current nushell) you could do:
If you want to manually write the hyperlinks without nu-ansi-term here's how to try two variations. For the variation with the
Without the
and that will put a hyperlink in your prompt. I don't know how to make nushell invoke a program to get prompt content but that's essentially what you're looking for.
Odd. Well, maybe I ought to remove |
vtparse is the package I've been working on which could help test all kinds of Color, Style, and OSControl pieces while working within a lightweight CI pipeline. |
I think we should remove the Another option is to add a parameter to the function or a separate function to add the SOH/STX bookends. |
For most the OSC sequences this works well. However with the hyperlink they can't just wrap the whole string in
I think this makes more sense -- at least for the hyperlink. The separate function to turn it on for all escape sequences could be useful in packages that use nu-ansi-term. I can think of part of Statship specifically, at least, where it could replace a somewhat elaborate function that does add these wrappers around the escape sequences. which adds some string-copying work. I'll look into both options. On the testing side of things how are you feeling about the test code as it stands? Are the examples running in CI going to be enough or is more needed? |
I love the examples and don't mind running them manually from time to time. i haven't tried running them in ci. however, i've not been able to make cwd work at all.
Ok, I'm good with another param or separate function. I'll let you decide which is best. |
7666cb2
to
349465c
Compare
Almost there. Pushed some changes so you can see, in a separate commit rather than with the fix, the "wrapping" code. I do need to check the I've also played with wrapping all of the SGR sequences. The code is much more complicated, though, and I think that would have to be a separate PR. There's a TODO REVIEW in "Add optional OSC test combinatorial run" which I could use some help with; I don't know what the command line needs to be for Windows Terminal, WezTerm, etc. -- basically the non-Linux ones. I don't want to leave it as a TODO if I can avoid it. |
I took the The rest is looking good. |
349465c
to
7743564
Compare
Running the (Ignored) Combinatorial Tests
Because it doesn't consider absence of the terminal to be a failure. That said, "success" doesn't seem like the right response either. I looked into how to report a "skip" at runtime and everything I've seen suggests there's no real way to do so via test summary output -- either it's statically ignored, filtered out by the command line, or it produces a pass/fail result. So I modified the test to print useful output using
Of course, since it tries to run GUI terminals, that's not useful to put that in the CI runs. But if you've got Terminal.exe or Terminal.app it should open a new one for each OSC function. If it doesn't then the code for launching those commands isn't right and that's why it's still got the REVIEW comment there. Note: For some reason cargo test is no longer letting me run only the Combinatorial Test ReorganizationI've reorganized the combinatorial test code. Now there will be one test ok/fail output per combination of Dropping the Wrapping StuffLastly, and most importantly, I figured out the issue I was seeing with needing to output two |
7413f06
to
8ba9dfb
Compare
I think that's a good call
I was wondering why Terminal.app was failing on my Mac. I found out that it's because the path is wrong. I'm not sure if this has changed in MacOS or it's just a M1 and later change? This is the new path.
But it just hangs now and no terminal.app is shown. Not sure what's going on exactly. Oh, this is interesting. I get this.
But if i do a
I just never see the window. I have to
it runs fine here
|
OK, I've replaced the path in my local tree. However in light of the rest of your reply I've kept digging.
Interesting. I didn't see any command line arguments that could be supplied to potentially fix that. However I've found another reference which may be potentially better or even fix your issue with the terminals not showing up. By any chance does:
open a terminal? The How about:
This one replaces a path to the application with what I believe is a "bundle" reference of some kind.
I'm guessing the Terminal.app is open in the foreground when you run the tests, but if not then perhaps bringing it to the foreground or looking at its window list (in the menu bar if I recall) will show them / let you make them visible. It's supposed to wait for the terminals to close. Without that you'd never see the test output because it'd just open, run, complete, and close too quickly to see. That happened with a bunch of terminals on my machine and there wasn't even a perceptible flicker of a briefly-open terminal window (and my machine, especially my graphics card, is somewhat old at this point).
While I expected to see
It also |
This is what I get.
but using this launches a terminal but i can't tell that the --args command does anything because the terminal just remains open.
This works but leaves the terminal open.
They were 4 terminal.apps open in my dock (which was hidden). Good guess. If i go to them and exit and quit the app, the tests will proceed. However I can't tell that it's doing anything. i.e. the title isn't changed, there's no hyperlink, cwd doesn't seem to work, don't notice any icon. I'm know cwd works in terminal.app because I implemented it in nushell. So does the title. Not sure about icon. I don't think hyperlink works at all. We have a command in nushell called |
Yeah, that seems to be normal. As best I can tell
OK, so I managed to spend some time on a call with a friend to try variations on I think that means, for now, it's best to omit MacOS and Terminal.app from the I considered running it via Applescript but my friend had to end our call. So I can only speculate on what the script might look like based on this StackExchange. The script file (e.g.
Which could be run (from a command line or via the
In the next few days I'll polish the PR up some more and push hopefully the last changes before it's ready for you to run the tests again and, if that passes your standards, a decision on merging. Thanks for your time! |
I'm fine with running examples manually, but we need to be able to see them work. I can't see Running on WSL2 I have i haven't tried changing the code for these other terminal emulators but it's some i've heard of |
That makes sense. TL;DR / Summary:
I'm a little hesitant to remove
Yeah, I'm having trouble with eterm as well. If I can't get it working locally and soon I'll just remove it. I think we've got enough confidence from the existing coverage and adding more terminals (on Linux) begins to have diminishing returns.
I've tried Possible Origins of
|
ah, yes. I remember this. I run SunOS 4.1.3/4 for many years and remember this.
ya, let's remove cwd and icon please. Let me know when you're ready for more testing. |
OK, I've merged those changes in and simplified things a lot. I renamed the function
As for:
The inner double-quotes showing up inside the backticks suggests the double-quotes might be interfering with parsing the command line properly -- I'm guessing that Since I lack access to Windows, by any chance could I submit this PR and you could fix up the Windows bits? I'm happy to respond with guesses and possible solutions for problems you encounter. I just think we've got these longer cycle times and it might become error-prone to have me modifying the PR based on your descriptions each time. That might work better unless I'm mistaken and you're unable to modify the PR too.
... (I didn't see any additional promising options to use)
The
OK, that looks good and I think that's sort of the ideal result (though maybe 10 seconds is too long / too short for some folks -- you folks can modify the timeout as you see fit before merging). |
da9aadc
to
6b96ba2
Compare
OK, I removed I believe I've done everything I planned on doing for this PR. Let me know if you see more that needs to be fixed or I need to do something for the Mac and Windows paths in |
Move OSControl out of Style PR nushell#43 created a regression where code that manually instantiated a Style could no longer be created without using the update operator and Default trait on Style. Rather than place non-public functions and data in Style move it all into AnsiGenericString. This has the added benefit of greatly simplifying the code. Fixes nushell#46
PR nushell#43 introduced a pub(crate) field in Style which broke the intended API (See: Issue nushell#46). Introduce a new test which will fail in those cases since it won't be able to initialize pub(crate) fields. Inspired by: nushell#46 (comment)
6b96ba2
to
d5a0653
Compare
@mhelsley I'm reconsidering the value of the osc.rs test. I mean, it's cool but if we can't get it to work cross platform, i'm not sure we should leave it in. I think I'd probably land this now if you can remove it. What are your thoughts on it? |
27c618f
to
1041ed1
Compare
It makes sense that the testing should be cross platform. I've moved the code into it's own local branch for now. I also merged your "remove comments" change into the commit with the examples. |
Thanks. Let's move forward with this! |
The API for modifying a string to contain a hyperlink used in nushell#47 behaves different than most of our `Style`-driven APIs that basically all work through method chaining. As an alternative I propose we change the signature to take the `AnsiGenericString` by value and modify it in place. This feels more expressive for quickly building out a styled string with a link. One downside compared to a modifying setter API is that it takes a more tokens to do conditional modification with an `if` expression.
Should include the more friendly API breakage from nushell#47 Still a breaking change compared to 0.47 based on nushell#40
The API for modifying a string to contain a hyperlink used in #47 behaves different than most of our `Style`-driven APIs that basically all work through method chaining. As an alternative I propose we change the signature to take the `AnsiGenericString` by value and modify it in place. This feels more expressive for quickly building out a styled string with a link. One downside compared to a modifying setter API is that it takes a more tokens to do conditional modification with an `if` expression.
Fixes #46 by moving OSControl out of Style and into AnsiGenericString