Better word wrapping #214

raidancampbell · 2022-02-07T05:54:53Z

raidancampbell commented

2022-02-07 05:54:53 +00:00

WIP for me to test this locally for a few more days.

This PR makes a couple of changes around word wrapping.

984fb4eb2faa15142cde5482e558c28c8f178ec6: make max width configurable instead of hardcoded to wrap at 100 lines
dac13e1669071cea781226dffa0fd48c2710b015: fulfills #165 to soft wrap at whitespace boundaries instead of mid-word. I don't understand the full meaning of width in this context, as all callers effectively pass -1 from whatever they have, so the change here does have some +1 in the logic, which feels like an off-by-one error.

Are you interested in these changes, and if so how cleaned up are you looking for this to be?

WIP for me to test this locally for a few more days. This PR makes a couple of changes around word wrapping. - 984fb4eb2faa15142cde5482e558c28c8f178ec6: make max width configurable instead of hardcoded to wrap at 100 lines - dac13e1669071cea781226dffa0fd48c2710b015: fulfills #165 to soft wrap at whitespace boundaries instead of mid-word. I don't understand the full meaning of `width` in this context, as all callers effectively pass `-1` from whatever they have, so the change here does have some `+1` in the logic, which feels like an off-by-one error. Are you interested in these changes, and if so how cleaned up are you looking for this to be?

raidancampbell added 2 commits 2022-02-07 05:54:53 +00:00

984fb4eb2f

max width for softwraping now configured via maxwidth configuration item: default or invalid value falls back to previous 100 column

dac13e1669

soft wrap between words

sloum reviewed 2022-02-07 17:53:06 +00:00

sloum left a comment

I'm definitely interested in the changes. I have not had a chance to pull, build, and test... but would like to do so soon.

Thanks for taking this on! While breaking in a word has never really bothered me much, I know it bothers a lot of other folks, so this will be a nice improvement. I left a few comments on the PR from the look I've been able to do (but I have a toddler running around my feet trying to get my attention, so I'll do a better look when I get some time later).

Thanks again! It looks great so far. Thank you for not using anything outside of the standard lib as well! Do you know if there is anything in the unicode package that you are using that wouldnt have been present at the time of Go 1.12? I suppose it has been long enough that we can push the required version if need be, but I'd like to know if so, that way I can eventually mention it in release notes.

I'm definitely interested in the changes. I have not had a chance to pull, build, and test... but would like to do so soon. Thanks for taking this on! While breaking in a word has never really bothered me much, I know it bothers a lot of other folks, so this will be a nice improvement. I left a few comments on the PR from the look I've been able to do (but I have a toddler running around my feet trying to get my attention, so I'll do a better look when I get some time later). Thanks again! It looks great so far. Thank you for not using anything outside of the standard lib as well! Do you know if there is anything in the unicode package that you are using that wouldnt have been present at the time of Go 1.12? I suppose it has been long enough that we can push the required version if need be, but I'd like to know if so, that way I can eventually mention it in release notes.

client.go Outdated

						
				@ -74,3 +74,3 @@

					screen.WriteString(c.TopBar.Render(c.Width, c.Options["theme"]))

					screen.WriteString("\n")

					pageContent := c.PageState.Render(c.Height, c.Width-1, (c.Options["theme"] == "color"))

					maxWidth, _ := strconv.Atoi(c.Options["maxwidth"])

sloum commented

2022-02-07 17:49:30 +00:00

In general I think ignoring error should be avoided where possible. I think we can likely encapsule this in a function that can be called wherever we need it:

func getMaxWidth() int {
    out, err := strconv.Atoi(c.Options["maxwidth"])
    if err != nil {
        out = 100
    }
    if out < 10 {
        out = 10
    }
    return out
}

It makes sense that we could really just store the value in a module var:

var MaxWidth = getMaxWidth()

Then in the code for setting config values have a case for changing maxwidth and reset the var. Then we can just reference the var everywhere instead of recasting to int in a few places (that said, I have not gotten to do a deep dive into your changes, just a cursory look.... so I may be overthinking this).

In general I think ignoring error should be avoided where possible. I think we can likely encapsule this in a function that can be called wherever we need it: ``` go func getMaxWidth() int { out, err := strconv.Atoi(c.Options["maxwidth"]) if err != nil { out = 100 } if out < 10 { out = 10 } return out } ```` It makes sense that we could really just store the value in a module var: ``` go var MaxWidth = getMaxWidth() ``` Then in the code for setting config values have a case for changing maxwidth and reset the var. Then we can just reference the var everywhere instead of recasting to int in a few places (that said, I have not gotten to do a deep dive into your changes, just a cursory look.... so I may be overthinking this).

raidancampbell commented

2022-02-08 05:14:18 +00:00

070f7eb6ba261866395706f709d12dff3ead2e74 contains a first pass on this. Since c.Options is both a field on the struct and altered by reading in the configuration after instantiating the struct, it's a bit less clean.

Other potential options:

add a piece of state to hold a sync.Once (existed in 1.12) in getMaxWidth to avoid the checks and casts
have some hook on the client that gets called after the configuration file is read in
see if the configuration can be read in first, then passed to the client constructor, then maxWidth can be a field on the client alongside Width

`070f7eb6ba261866395706f709d12dff3ead2e74` contains a first pass on this. Since `c.Options` is both a field on the struct and altered by reading in the configuration after instantiating the struct, it's a bit less clean. Other potential options: - add a piece of state to hold a `sync.Once` (existed in 1.12) in `getMaxWidth` to avoid the checks and casts - have some hook on the client that gets called after the configuration file is read in - see if the configuration can be read in first, then passed to the client constructor, then `maxWidth` can be a field on the client alongside `Width`

page.go Outdated

						
				@ -72,2 +72,3 @@

					}

					width = min(width, 100)

					if maxWidth < 100 {

						maxWidth = 100

sloum commented

2022-02-07 17:44:24 +00:00

I'm not sure I follow why this is here. Since we are trying to wrap at maxwidth and remove hardcoded values in favor of the passed value that comes from the config, why are we hardcoding 100 here?

raidancampbell commented

2022-02-08 04:36:05 +00:00

The intent here was to catch malformed / missing configuration entries: your suggestion of wrapping the logic into a getMaxWidth function will solve this problem and remove the need here.

Much better, thanks!

The intent here was to catch malformed / missing configuration entries: your suggestion of wrapping the logic into a `getMaxWidth` function will solve this problem and remove the need here. Much better, thanks!

👍 1

raidancampbell commented

2022-02-08 04:44:15 +00:00

I'm definitely interested in the changes. I have not had a chance to pull, build, and test... but would like to do so soon.

Thanks for taking this on! While breaking in a word has never really bothered me much, I know it bothers a lot of other folks, so this will be a nice improvement. I left a few comments on the PR from the look I've been able to do (but I have a toddler running around my feet trying to get my attention, so I'll do a better look when I get some time later).

Thanks again! It looks great so far. Thank you for not using anything outside of the standard lib as well! Do you know if there is anything in the unicode package that you are using that wouldnt have been present at the time of Go 1.12? I suppose it has been long enough that we can push the required version if need be, but I'd like to know if so, that way I can eventually mention it in release notes.

Glad to help, it was a minor annoyance to me and a pretty easy change. No rush to review from my side, I'm happy using my minor fork for the time being.

I did a quick check on the unicode API: as of 1.12, the unicode.IsSpace function existed. At a cursory glance, it looks like the 1.12 implementation and 1.17 implementation are identical, so it should be safe.

EDIT: fix URL

> I'm definitely interested in the changes. I have not had a chance to pull, build, and test... but would like to do so soon. > > Thanks for taking this on! While breaking in a word has never really bothered me much, I know it bothers a lot of other folks, so this will be a nice improvement. I left a few comments on the PR from the look I've been able to do (but I have a toddler running around my feet trying to get my attention, so I'll do a better look when I get some time later). > > Thanks again! It looks great so far. Thank you for not using anything outside of the standard lib as well! Do you know if there is anything in the unicode package that you are using that wouldnt have been present at the time of Go 1.12? I suppose it has been long enough that we can push the required version if need be, but I'd like to know if so, that way I can eventually mention it in release notes. Glad to help, it was a minor annoyance to me and a pretty easy change. No rush to review from my side, I'm happy using my minor fork for the time being. I did a quick check on the unicode API: as of 1.12, the `unicode.IsSpace` function existed. At a cursory glance, it looks like the [1.12 implementation](https://cs.opensource.google/go/go/+/refs/tags/go1.12:src/unicode/graphic.go;l=126) and [1.17 implementation](https://cs.opensource.google/go/go/+/refs/tags/go1.17.6:src/unicode/graphic.go;l=126) are identical, so it should be safe. EDIT: fix URL

🎉 1

raidancampbell added 1 commit 2022-02-08 04:54:40 +00:00

070f7eb6ba

extract logic for reading maxwidth config into a function: now safely handles fallbacks for missing or malformed configuration

sloum changed target branch from master to release-2.4.0

2022-02-08 18:28:24 +00:00

sloum commented

2022-02-08 18:39:45 +00:00

Ok. I have built this. Looks great! Updating the width with :set maxwidth 60 or the like works well and the word wrapping is great!

I don't want to put more work on you, but if you wanted to eliminate the leading space that sometimes occurs from wrapping at word boundaries that would make it super slick. We'd want to be careful to only do so if the space was a result of word wrapping, not the result of a newline followed by a space in the source document. Again though: do not feel like you have to do this. It is just a suggestion. This is already such a bigger improvement than I thought it would be.

I really appreciate your work on this! Thank you so much!

Ok. I have built this. Looks great! Updating the width with `:set maxwidth 60` or the like works well and the word wrapping is great! I don't want to put more work on you, but if you wanted to eliminate the leading space that sometimes occurs from wrapping at word boundaries that would make it super slick. We'd want to be careful to only do so if the space was a result of word wrapping, not the result of a newline followed by a space in the source document. Again though: _do not feel like you have to do this_. It is just a suggestion. This is already such a bigger improvement than I thought it would be. I really appreciate your work on this! Thank you so much!

raidancampbell added 1 commit 2022-02-09 03:47:26 +00:00

1bef2e51a1

when wrapping, omit leading whitespace created if the wrap occurs at a space char

raidancampbell commented

2022-02-09 03:50:42 +00:00

Ok. I have built this. Looks great! Updating the width with :set maxwidth 60 or the like works well and the word wrapping is great!

I don't want to put more work on you, but if you wanted to eliminate the leading space that sometimes occurs from wrapping at word boundaries that would make it super slick. We'd want to be careful to only do so if the space was a result of word wrapping, not the result of a newline followed by a space in the source document. Again though: do not feel like you have to do this. It is just a suggestion. This is already such a bigger improvement than I thought it would be.

I really appreciate your work on this! Thank you so much!

heh, don't worry about it.

1bef2e51a1 takes the approach of "if I'm going to wrap and the first post-wrap character is a space or tab, eat it."

It required some test changes, but a new one was added to better describe what it's doing

> Ok. I have built this. Looks great! Updating the width with `:set maxwidth 60` or the like works well and the word wrapping is great! > > I don't want to put more work on you, but if you wanted to eliminate the leading space that sometimes occurs from wrapping at word boundaries that would make it super slick. We'd want to be careful to only do so if the space was a result of word wrapping, not the result of a newline followed by a space in the source document. Again though: _do not feel like you have to do this_. It is just a suggestion. This is already such a bigger improvement than I thought it would be. > > I really appreciate your work on this! Thank you so much! heh, don't worry about it. 1bef2e51a1705bf43f014b1aa7f0a4cbc7398f55 takes the approach of "if I'm going to wrap and the first post-wrap character is a space or tab, eat it." It required some test changes, but a new one was added to better describe what it's doing

raidancampbell changed title from ~~WIP: Better word wrapping~~ to Better word wrapping

2022-02-10 05:46:28 +00:00

raidancampbell commented

2022-02-10 05:53:23 +00:00

I got a chance to play around with this for a couple hours today and couldn't find any obvious issues. Code-wise it's not amazingly performant or clean: it gets the job done, but I'm happy to clean it up if you want.

For bookkeeping, this PR resolves #169 and #165.

Continue reviewing or merge at your leisure: be it days or months, I'm in no rush. Thanks for writing bombadillo and giving a fun project to hack on!

I got a chance to play around with this for a couple hours today and couldn't find any obvious issues. Code-wise it's not amazingly performant or clean: it gets the job done, but I'm happy to clean it up if you want. For bookkeeping, this PR resolves #169 and #165. Continue reviewing or merge at your leisure: be it days or months, I'm in no rush. Thanks for writing bombadillo and giving a fun project to hack on!

sloum commented

2022-02-10 23:58:38 +00:00

Sounds great! I probably wont get around to merging and working through the release until next week, but this is great!

I'm not too worried about amazingly clean/performant (you've seen the codebase, lol), it seems to work really well and I did not notice any performance change. If someone is on something really low specced then bombadillo probably isnt the right choice anyway and VF1, AV98, or any of the other more repl-like clients would be better choices.

Thanks again so much for your work on this! I'm going to keep the PR open until I can actually make time to do the release (I'll be merging in a few other things as well). I'll tag you in a comment on the release merge so you know when it is fully merged in as a release.

Sounds great! I probably wont get around to merging and working through the release until next week, but this is great! I'm not too worried about amazingly clean/performant (you've seen the codebase, lol), it seems to work really well and I did not notice any performance change. If someone is on something really low specced then bombadillo probably isnt the right choice anyway and VF1, AV98, or any of the other more repl-like clients would be better choices. Thanks again so much for your work on this! I'm going to keep the PR open until I can actually make time to do the release (I'll be merging in a few other things as well). I'll tag you in a comment on the release merge so you know when it is fully merged in as a release.

sloum merged commit 5a91f4ac94 into release-2.4.0

2022-02-16 04:03:48 +00:00

sloum referenced this issue from a commit

2022-02-16 04:03:49 +00:00

Merge pull request 'Better word wrapping' (#214) from raidancampbell/bombadillo:master into release-2.4.0

Sign in to join this conversation.

No reviewers