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

Minor fixes to shell integation in repl. #5701

Merged
merged 1 commit into from Jun 2, 2022

Conversation

PerBothner
Copy link
Contributor

Added CMD_FINISHED_MARKER to be emitted when command finishes.

Also switched the names PRE_EXECUTE_MARKER and PRE_PROMPT_MARKER as the old names were confusing/wrong.

I've run cargo fmt and cargo tests.

Added CMD_FINISHED_MARKER to be emitted when command finishes.
Also switched the names PRE_EXECUTE_MARKER and PRE_PROMPT_MARKER
as the old names were confusing/wrong.
@fdncred
Copy link
Collaborator

fdncred commented Jun 2, 2022

What terminals have you tested these changes with? I'd like to maintain support with iterm2, kitty, and wezterm at a minimum.

@PerBothner
Copy link
Contributor Author

I've primarily tested on DomTerm because I think DomTerm's rich/html output would be a great match for NuShell. See issue #1570 - I'm looking into how to get rich output working.

I did a little bit of sanity test of Kitty and tried some things here. Right now I'm out in the boonies so I have no access to a Mac (hence no way to test iterm2) - and WezTerm is taking forever to download.

@fdncred
Copy link
Collaborator

fdncred commented Jun 2, 2022

I briefly tested all 3 and nothing jumped out at me so let's land this and pretend it's well tested. LOL.

Also, I'd love for you to walk me through setting up DomTerm sometime. It would be great if you could jump on Discord sometime.

Thanks for your contribution, and I agree I got the other MARKERS backwards when I implemented this. LOL, thanks for fixing it.

@fdncred fdncred merged commit 7a9bf06 into nushell:main Jun 2, 2022
@PerBothner
Copy link
Contributor Author

Actually, the iTerm2/FinalTerm shell integration protocol is rather limited. I've designed and implemented more full-featured protocol - see this article and the specification. For now I'm trying to just get the basics working using the existing NuShell support - I'll see if further changes are needed later.

@fdncred
Copy link
Collaborator

fdncred commented Jun 2, 2022

@PerBothner thanks. I've referred to that article while writing the first of these changes. That was you who authored that? wow. I'm impressed. :) Anything you could help with on improving this would be helpful.

@PerBothner PerBothner deleted the shell-integration-tweak branch June 2, 2022 23:14
@PerBothner
Copy link
Contributor Author

I'll be happy to help you set up DomTerm. (I just checked in some fixes to make Nushell work better.)
I try to keep accurate and detailed instructions here.
I mostly use and test Fedora, though I have build and run DomTerm on Ubuntu, MacOS, and WSL (using Ubuntu). However, instructions for non-Fedora platforms are more likely to incomplete or outdated.
There is an AppImage version of DomTerm which might be easier to use (on Linux or WSL), but it isn't as new, and you would probably want to use the fresh-from-GitHub sources anyway.
I just discovered I have a Discord account, but I don't remember creating or using it. So while I can use Discord, I can't promise to check it on a regular basis. I'm on old-school email-oriented guy: per@bothner.com.

fennewald pushed a commit to fennewald/nushell that referenced this pull request Jun 27, 2022
Added CMD_FINISHED_MARKER to be emitted when command finishes.
Also switched the names PRE_EXECUTE_MARKER and PRE_PROMPT_MARKER
as the old names were confusing/wrong.
@PerBothner
Copy link
Contributor Author

I've created simplified instructions for building DomTerm on macOS. Please let me know if they are unclear or you run into problems.

I've also created a page for macOS-specific issues. That is a suitable place to report or discuss mac-specific problems or blemishes.

I haven't actually tried NuShell on macOS yet - I've been focusing on simplfying and testing the build instructions and general sanity testing for DomTerm.

@fdncred
Copy link
Collaborator

fdncred commented Jul 2, 2022

Thanks. I got it compiled. I had to edit the configure script. Kind of a royal pain to get it going LOL. Then I launched it with -Bwry and couldn't move the window. Then I tried with -Bqt and couldn't move the window. Fonts weren't good. Looks like it prefers a light background. Had problems with openssl, qmake, Mac.

@PerBothner
Copy link
Contributor Author

Thanks for the feedback!

If you had to edit the configure script, please report it as a bug and I'll try to fix it.

I've been working today on issues relating to the titllebar and moving the window. It's almost ready to check in, but as a workaround you can start start domterm with the titlebar=system option.

I do notice fonts having line-spacing issues. There may be other problems.

You can specify style.dark=yes on the command-line to force dark mode. (There should be some logic to automatically select dark mode, but it may be broken.) I personally prefer light mode user interfaces, so "dark mode" could do with some polishing from someone who uses it.

@PerBothner
Copy link
Contributor Author

Somebody in the past reported a problem with openssl - you can see a possible workaround mentioned here. I didn't mention it since I didn't run into it, so I didn't realize it was still an issue. DomTerm only needs openssl when it is being run as a web server, which isn't the preferred way to use it remotely. (But that's a different discussion.

You also mentioned problems with "qmake, Mac [???]". It couldn't find a suitable qmake? I tested with homebrew, Qt 6, and Monterrey (on an old MacBook Air).

@fdncred
Copy link
Collaborator

fdncred commented Jul 2, 2022

oh, I got around the openssl issue by editing the configure script. It was looking for openssl in a native directory versus the home-brew directory. I had to do this to get it to compile after I changed the configure script CXX="g++ -std=c++11" ./configure --with-qt=/opt/homebrew/opt/qt/share/qt/libexec/. Then I ran make.

@PerBothner
Copy link
Contributor Author

Windows should now be movable (draggable) on all tested platforms (Electron, Qt, Wry).

However, dragging subwindow ("panes") only seems to work on Electron.
Electron also has proper (native) "traffic light" buttons, which makes it look a lot nicer.

@PerBothner
Copy link
Contributor Author

I changed the font order so "Menlo" is preferred over "Monospace". That seems to work better, at least when tested using "mc" (Midnight Commander).

@fdncred
Copy link
Collaborator

fdncred commented Jul 2, 2022

it looks like it's detecting my dark mode now, which is great. the window is draggable, which helps. This is a screenshot of the electron version. Not sure how to set the font but it's not that big of a deal. I'll probably stick with iterm2.

Screen Shot 2022-07-02 at 4 35 53 PM

@PerBothner
Copy link
Contributor Author

Have you tried a command like:

ls|to html --partial|bin/domterm hcat

What do you think? Note the "floating table header" (assuming output is more than one screenful), as mentioned on Discourse.
Any advice on how to set nu up to add |to html --partial|bin/domterm hcat automatically - or even better |to html --domterm automatically? Would to nu community be open to me adding a --domterm option to to html? It might tweak the html as well as wrapping in the needed escape sequences. While I'm relatively new to Rust, I could probably figure out how to do that. However, it's harder to figure out how to replace autoview (? I see this mentioned a few places) with to html - that I could use some help with.

I'm not expecting you to switch to DomTerm as your primary terminal - iterm2 is probably both more stable and has more features you're used to. But I am hoping I can (in time) make the integration so appealing that you'll want to!

@fdncred
Copy link
Collaborator

fdncred commented Jul 2, 2022

Have you tried a command like:

domterm: don't seem to be running under DomTerm - use --force to force

@PerBothner
Copy link
Contributor Author

"domterm: don't seem to be running under DomTerm - use --force to force"

Strange. Did it work if you try --force?

I just did a brew install nushell, started up nu, and then ls|to html --partial|bin/domterm hcat worked for me (on DomTerm/Electron on Monterrey) - like the snapshot in my Discourse message.

@fdncred
Copy link
Collaborator

fdncred commented Jul 3, 2022

I tried ls|to html --partial|bin/domterm hcat --force and got the same error message.

@PerBothner
Copy link
Contributor Author

Sorry, it looks like --force needs to be before the hcat because it's a general "domterm option" not a specific "hcat option".

That should probably be more more user-friendly.

You could also try commenting out these lines in the probe_domterm function in lws-term/utils.cc:

char *term_env = getenv("TERM");
char *domterm_env = getenv("DOMTERM");
if (! ((domterm_env && domterm_env[0])
       || term_env == NULL || term_env[0] == '\0'
       || strstr(term_env, "xterm") != NULL))
    return 0;

That would let us know it's an environment-variable issue.

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

Successfully merging this pull request may close these issues.

None yet

2 participants