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 support for OSC sequences #8

Open
bensadeh opened this issue Sep 26, 2020 · 14 comments · May be fixed by #9
Open

Add support for OSC sequences #8

bensadeh opened this issue Sep 26, 2020 · 14 comments · May be fixed by #9

Comments

@bensadeh
Copy link

Hi Michael,

First of all, I would like to say that I love this package. I am doing a lot of formatting with ANSI codes, and this package really takes a lot of the pain away from having to account for invisible codes. I am currently using other word-wrapping packages that do not account for the ANSI codes, and I'd like to migrate to go-term-text in full.

However, Wrap() currently hard breaks words at the word limit. This interferes with the ability to properly format hyperlinks in the terminal, because inserting a newline in the middle of the link messes up the output. I am attaching a screenshot to better explain the issue.

Screenshot 2020-09-26 at 14 41 37

The way I see it, the option not to break words that are longer than the line limit would solve the issue. Would it be possible to implement a wrap option to not break words that are longer than the line limit?

@MichaelMure
Copy link
Owner

Hi Ben,

I may be wrong but it seems to me that your problem is not the wrapping itself but that go-term-text doesn't support the OSC 8 type sequence required for links. Those sequences would not be detected which would throw off the accounting in the wrapping algorithm. Does the formatting works as you expect if you don't have this hyperlink thing ?

If my assumption is correct, this support would need to be implemented (disclaimer, I won't have the time to do it). There is a bunch of pattern like this in those algorithms:

	for _, r := range runes {
		if r == '\x1b' {
			escape = true
		}

               // do something ...

		if r == 'm' {
			escape = false
		}
	}

To support OSC8 and any new additional sequence type, I suppose it would be best to have a simple component to which you pass runes and that say if it's part of a sequence or not.

@bensadeh
Copy link
Author

The formatting does indeed work as I expect without the OSC 8 type sequence, and I understand that implementing support for this is non-trivial.

My hope with raising this issue was to check out whether it would be possible to implement an option to not hard-break words that are longer than the line limit, as this would fix my problem. But I don't know how common my use case is, and I don't want to suggest changes that aren't widely used or that are not in line with your package philosophy.

If it's too much work or not fitting for the package, I'll try and see whether I can implement it in a fork. If I'm able to implement it myself, I would be glad to send you a pull request for it.

@MichaelMure
Copy link
Owner

It looks in line with the idea of this package to me. It wouldn't be too hard to do either, especially as there is a high test coverage. Just search for the \x1b string and you should find all the relevant places.

Implementing an option to not hard-break words could also works but I'm not sure it would really solve your problem and I'm not convinced it would be simpler. What behavior you would want to see if it's not breaking words ?

@bensadeh
Copy link
Author

It's true that a hard-break wouldn't solve my problem directly (it's more of a quick fix at the moment). I am fairly new to golang and formatting text in the terminal, so it might be that I'm not approaching this from the right angle.

Thank you anyways for the pointers and quick replies. I will try to find a more appropriate solution and look where you suggested.

@MichaelMure
Copy link
Owner

Useful document explaining what kind of sequence exist: https://chromium.googlesource.com/apps/libapps/+/a5fb83c190aa9d74f4a9bca233dac6be2664e9e9/hterm/doc/ControlSequences.md#OSC

@MichaelMure MichaelMure linked a pull request Sep 26, 2020 that will close this issue
@MichaelMure
Copy link
Owner

Damn you, you got me intrigued ;)

Would #9 work ?

@bensadeh
Copy link
Author

Wow, that was quick! Thank you so much.

The pull request is good for me for now. I need to do some more testing to make sure it fits the project I'm working on, but I think this is a good solution.

Cheers, and hope you have a good rest of your weekend.

@MichaelMure
Copy link
Owner

Let me know if that actually works on your project. Then I can merge and make a proper release.

@bensadeh
Copy link
Author

Thank you. I will do some more checks and post my results here.

@bensadeh
Copy link
Author

bensadeh commented Oct 1, 2020

Hi again,

I did some more testing and for the project and these are my findings. It seems that there are some corner cases that at the moment are not handled. Here is a short snippet to show an example. The output is piped to less in order to switch to and from raw codes (type -r and then Enter when inside less to switch).

	hyperlink := "\u001B]8;;" + "https://www.google.com/" + "\u001B\\" + "Link test" + "\u001B]8;;\a"
	text := "abc def ghi jkl " + hyperlink + " mno pqr stu vwx"
	lineWidth := 9
	wrappedText, _ := term.Wrap(text,lineWidth, term.WrapIndent(""), term.WrapPad(""))

	cmd := exec.Command("less", "-r")
	cmd.Stdin = strings.NewReader(wrappedText)
	cmd.Stdout = os.Stdout

	err := cmd.Run()
	if err != nil {
		log.Fatal(err)
	}

For my example, lineWidth set to 9 does not properly output the link in the terminal. Setting lineWidth to 10 fixes the output, but actual wrapping happens at 17 characters. I can attach some screenshots to illustrate, but it is easier to run this code and toggling the raw output to see what happens.

This being said, the pull request is still an improvement because previously the wrap function would not handle the OSC codes at all.

@bensadeh bensadeh changed the title Option to not break words that are longer than the line limit Add support for OSC sequences Oct 1, 2020
@MichaelMure
Copy link
Owner

hyperlink := "\u001B]8;;" + "https://www.google.com/" + "\u001B\" + "Link test" + "\u001B]8;;\a"

Is that legal ? That's not the sequence I detect in there: https://github.com/MichaelMure/go-term-text/pull/9/files#diff-eb683c15127af2e8c14e17835b7813f4R115-R140

The chromium document say for OSC sequences:

This is a grab bag of various escape sequences. They are initiated with ESC+]. Arguments to the sequences are typically delimited by ;, and terminated with BEL. e.g. ESC+] 0 ; title BEL (0x1b 0x5d 0x30 0x3b ... 0x07).

@bensadeh
Copy link
Author

bensadeh commented Oct 1, 2020

Sorry Michael, I'm afraid I'm a bit out of my depth here.

I tried to follow the sequences outlined in this gist to the best of my ability, but there might be something obvious that I'm missing. In hindsight I could have spent more time researching on my own before opening up an Issue here.

@MichaelMure
Copy link
Owner

Yeah, it's tricky with all this escaping everywhere. I believe we are both wrong actually.

In https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda#the-escape-sequence, the sequence is described as:

  • starting with ESC and ], with ESC being \u001B (binary) or \x1b (hex)
  • end with either ESC and \, or BEL, with BEL being \x07 (hex)

The code in the PR only start a sequence with ESC and end with \x07 so the normal ESC + \ ending wont be detected.

The sequence you are using though looks incorrect as you are using ESC in the middle as well, and the ending looks incorrect as well.

@bensadeh
Copy link
Author

Hi again Michael!

Just wanted to drop in and say that I have abandoned the idea of incorporating these OSC sequences because they added a lot of complexity elsewhere in the project and they just weren't as useful as I had imagined them to be.

However, I am still using this text formatting package in the project with excellent results. Thanks again for looking into everything, but no need to keep this issue open on my account anymore 😊.

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 a pull request may close this issue.

2 participants