WIP first attempt on line wrapping indents issue #34

asdf · 2019-09-04T13:01:57Z

asdf commented

2019-09-04 13:01:57 +00:00

OK this makes type i line wrapping look OK, but wrapping of links is not great. Mainly just sending this through to explain what I was working on and to promote some more ideas on this.

sloum reviewed 2019-09-04 15:24:56 +00:00

sloum left a comment

I will pull this and run the test locally as well as check out a build of it and get back to you asap. Thanks so much for putting this together :) I added a few comments, one a thanks and the other a minor golang efficiency thing (but it doesnt matter a whole ton for our purposes I dont think).

cui/cui.go Outdated

						
				@ -103,3 +111,3 @@

									out = append(out, subout.String())

									subout.Reset()

									subout.WriteString(wd)

									subout.WriteString(indent + wd)

sloum commented

2019-09-04 15:20:04 +00:00

I think it would be faster here to do two writes:

subout.WriteString(indent)
subout.WriteString(wd)

I think it would be faster here to do two writes: ``` subout.WriteString(indent) subout.WriteString(wd) ```

sloum commented

2019-09-04 15:40:20 +00:00

That said, I wouldn't hold up an approval over it, and I could be wrong. I just remember being told that writing to a bytes or string buffer is always faster that concating strings.

cui/cui_test.go

						
				@ -0,0 +2,4 @@

				import (

					"reflect"

					"testing"

sloum commented

2019-09-04 15:22:33 +00:00

Awesome. I have not used any testing in golang yet, so this will be a good learning experience for me. Thanks for setting this up!

sloum commented

2019-09-04 15:39:12 +00:00

Got the branch locally and ran it. The wrapping looks good. I see what you mean about the link wrapping. It seems to be an issue for lines that have no spaces and go longer than the available width. I'm glad it isnt a fatal error of some form, it just cuts it off (and maybe moves it down a line). I'm not sure if we want to consider that issue as in scope for this or not. What do you think?

To work with that we'd need to add in an if block to handle lines with no spaces (basically a wd inside the loop that is longer than the width of the terminal).

The i wrapping here and link wrapping where spaces are present both look solid to me. I'm happy to merge this in, but if you'd like to take a swing at the lines with no spaces issue please be my guest :)

Got the branch locally and ran it. The wrapping looks good. I see what you mean about the link wrapping. It seems to be an issue for lines that have no spaces and go longer than the available width. I'm glad it isnt a fatal error of some form, it just cuts it off (and maybe moves it down a line). I'm not sure if we want to consider that issue as in scope for this or not. What do you think? To work with that we'd need to add in an if block to handle lines with no spaces (basically a `wd` inside the loop that is longer than the width of the terminal). The `i` wrapping here and link wrapping where spaces are present both look solid to me. I'm happy to merge this in, but if you'd like to take a swing at the lines with no spaces issue please be my guest :)

asdf commented

2019-09-05 12:11:05 +00:00

Thanks for the feedback, I made the suggested change.

I agree regarding the scope, and would prefer more incremental changes to keep things moving.

I didn't get far today, but identified two issues that prevent this from being usable as it is:

Lines that contain lots of consecutive spaces will be really badly mangled
indents for gopher maps were fixed, but this broke non-indented text (like wrapping in text documents)

Lesser issues I've found are:

As you mentioned - long words don't wrap at all
Indents can have a space added after them sometimes

These issues are not resolved, but I've written more tests to cover most of these issues, and fixed issues in some of the existing tests.

Also there's a benchmark test for wraplines() that you can run using go test -bench=.

Thanks for the feedback, I made the suggested change. I agree regarding the scope, and would prefer more incremental changes to keep things moving. I didn't get far today, but identified two issues that prevent this from being usable as it is: - Lines that contain lots of consecutive spaces will be really badly mangled - indents for gopher maps were fixed, but this broke non-indented text (like wrapping in text documents) Lesser issues I've found are: - As you mentioned - long words don't wrap at all - Indents can have a space added after them sometimes These issues are not resolved, but I've written more tests to cover most of these issues, and fixed issues in some of the existing tests. Also there's a benchmark test for `wraplines()` that you can run using `go test -bench=.`

sloum commented

2019-09-05 15:55:52 +00:00

Thanks for setting up more tests! They look great!
I dont seem to be able to get the benchmark to run :-/
I potentially have a fix that might simplify things: Instead of using strings.Split(str, sep) we use strings.SplitAfter(str, sep). A quick check over at play.golang.org makes me think that this may work.

package main

import (
	"fmt"
	"strings"
)

// Using "&" as the split so that it can be more visible in the slice print
// a space should work the same (it will just be harder to see when printing)
func main() {
	fmt.Println(strings.SplitAfter("&&&&&&&This&is&&&a&test&&&","&"))
}

Running that at the aforementioned go playground shows how it could work. Docs can be found here.

If we do that we do not destroy any existing spacing. We wont have to manually add spaces after words anymore either... it would just use the existing spacing.

What do you think? Does this lead us in a good direction?

1. Thanks for setting up more tests! They look great! 2. I dont seem to be able to get the benchmark to run :-/ 3. I potentially have a fix that might simplify things: Instead of using `strings.Split(str, sep)` we use `strings.SplitAfter(str, sep)`. A quick check over at [play.golang.org](https://play.golang.org) makes me think that this may work. ``` package main import ( "fmt" "strings" ) // Using "&" as the split so that it can be more visible in the slice print // a space should work the same (it will just be harder to see when printing) func main() { fmt.Println(strings.SplitAfter("&&&&&&&This&is&&&a&test&&&","&")) } ``` Running that at the aforementioned go playground shows how it could work. Docs can be found [here](https://golang.org/pkg/strings/#SplitAfter). If we do that we do not destroy any existing spacing. We wont have to manually add spaces after words anymore either... it would just use the existing spacing. What do you think? Does this lead us in a good direction?

asdf commented

2019-09-07 13:24:47 +00:00

Using strings.SplitAfter simplifies this a fair bit. It wraps the line and doesn't mangle spaces. No need to reinsert spaces between words either.

Gopher maps now wrap like this throughout the entire document:

            **** bombadillo ****                                          
                                                                          
           bombadillo is a gopher client for the terminal. it functions   
as a pager                                                                
           with a "full screen" terminal user interface. keys are mapped  
similarly

While this does not indent wrapped lines, I prefer this over the previous behaviour because it seems like a more consistent and self-explanatory result.

I think these are the actions required to finalise this:

Clean up cui/cui.go and associated tests for the intended functionality
Create proposal to amend bombadillo-info document width to 69 characters, so it will fit in to an 80 character terminal without wrapping.
Create separate unit of work for wrapping of large words
If possible, create another unit of work for handling wrapping with an indent (wrapLines would have to be aware of the document type - if you have a suggestion on this let me know)

Using `strings.SplitAfter` simplifies this a fair bit. It wraps the line and doesn't mangle spaces. No need to reinsert spaces between words either. Gopher maps now wrap like this throughout the entire document: <pre> **** bombadillo **** bombadillo is a gopher client for the terminal. it functions as a pager with a "full screen" terminal user interface. keys are mapped similarly </pre> While this does not indent wrapped lines, I prefer this over the previous behaviour because it seems like a more consistent and self-explanatory result. I think these are the actions required to finalise this: 1. Clean up `cui/cui.go` and associated tests for the intended functionality 1. Create proposal to amend `bombadillo-info` document width to 69 characters, so it will fit in to an 80 character terminal without wrapping. 1. Create separate unit of work for wrapping of large words 1. If possible, create another unit of work for handling wrapping with an indent (`wrapLines` would have to be aware of the document type - if you have a suggestion on this let me know)

sloum commented

2019-09-07 14:35:50 +00:00

Awesome! That seems like a very acceptable result to me.

The path forward also seems clear. For number 4... I was having trouble figuring this one out. Given that the cui module has no knowledge of program state (beyond the elements that it defines) and it has no knowledge of the gopher module it is difficult to let it know what type of document it is dealing with at a given moment. We could pass it through... but we'd end up needing to pass through a bunch of other intermediate functions.

Changing the way things flow into wraplines could help, doing a larger change of how the whole application is structured could also help but seems maybe like overkill without other pressing needs pointing us in that direction. I'll try to see if I can find a good way to make wraplines document aware.

Awesome! That seems like a very acceptable result to me. The path forward also seems clear. For number 4... I was having trouble figuring this one out. Given that the cui module has no knowledge of program state (beyond the elements that it defines) and it has no knowledge of the gopher module it is difficult to let it know what type of document it is dealing with at a given moment. We could pass it through... but we'd end up needing to pass through a bunch of other intermediate functions. Changing the way things flow into wraplines could help, doing a larger change of how the whole application is structured could also help but seems maybe like overkill without other pressing needs pointing us in that direction. I'll try to see if I can find a good way to make wraplines document aware.

sloum commented

2019-09-07 14:36:30 +00:00

What currently happens with long lines with no spaces? I believe they just get truncated, yeah?

asdf commented

2019-09-10 10:28:26 +00:00

Yes, but, ACTUALLY... wrapLines returns the slice still containing that long line or long word, exceeding the console width. Something else must truncate it when it is displayed.

I have an idea on wrapping any long line or long word, but don't want to gunk this up with it.

This latest commit just focuses on simple line wrapping and associated tests. For this specific change, I'm not sure if there should be more tests. But, that's pretty much it I think.

Yes, but, ACTUALLY... `wrapLines` returns the slice still containing that long line or long word, exceeding the console width. Something else must truncate it when it is displayed. I have an idea on wrapping any long line or long word, but don't want to gunk this up with it. This latest commit just focuses on simple line wrapping and associated tests. For this specific change, I'm not sure if there should be more tests. But, that's pretty much it I think.

sloum commented

2019-09-10 15:12:14 +00:00

Awesome. This looks good to me. I'll merge in and another branch can get started for further work on the issue of wrapping for other cases.

sloum closed this pull request

2019-09-10 15:16:13 +00:00

Sign in to join this conversation.

No reviewers