sloum/bombadillo
sloum
/
bombadillo
5
27
Fork 7
Code Issues 18 Pull Requests 1 Releases 9 Activity

Better word wrapping #214

Merged
sloum merged 4 commits from raidancampbell/bombadillo:master into release-2.4.0 2022-02-16 04:03:48 +00:00
Conversation 5 Commits 4 Files Changed 5 +29 -14
5 changed files with 29 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 984fb4eb2f - Show all commits

21
client.go
View File

@ -73,7 +73,8 @@ func (c *client) Draw() {
screen.WriteString("\033[0m")
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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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`
pageContent := c.PageState.Render(c.Height, c.Width-1, maxWidth, (c.Options["theme"] == "color"))
var re *regexp.Regexp
if c.Options["theme"] == "inverse" {
screen.WriteString("\033[7m")
@ -258,7 +259,8 @@ func (c *client) TakeControlInput() {
}
err = c.NextSearchItem(0)
if err != nil {
c.PageState.History[c.PageState.Position].WrapContent(c.Width-1,(c.Options["theme"] == "color"))
maxWidth, _ := strconv.Atoi(c.Options["maxwidth"])
c.PageState.History[c.PageState.Position].WrapContent(c.Width-1,maxWidth,(c.Options["theme"] == "color"))
c.Draw()
}
case ':', ' ':
@ -988,7 +990,8 @@ func (c *client) handleGopher(u Url) {
} else {
pg.FileType = "text"
}
pg.WrapContent(c.Width-1, (c.Options["theme"] == "color"))
maxWidth, _ := strconv.Atoi(c.Options["maxwidth"])
pg.WrapContent(c.Width-1, maxWidth, (c.Options["theme"] == "color"))
c.PageState.Add(pg)
c.SetPercentRead()
c.ClearMessage()
@ -1015,7 +1018,8 @@ func (c *client) handleGemini(u Url) {
u.Mime = capsule.MimeMin
pg := MakePage(u, capsule.Content, capsule.Links)
pg.FileType = capsule.MimeMaj
pg.WrapContent(c.Width-1, (c.Options["theme"] == "color"))
maxWidth, _ := strconv.Atoi(c.Options["maxwidth"])
pg.WrapContent(c.Width-1, maxWidth, (c.Options["theme"] == "color"))
c.PageState.Add(pg)
c.SetPercentRead()
c.ClearMessage()
@ -1081,7 +1085,8 @@ func (c *client) handleLocal(u Url) {
if ext == ".jpg" || ext == ".jpeg" || ext == ".gif" || ext == ".png" {
pg.FileType = "image"
}
pg.WrapContent(c.Width-1, (c.Options["theme"] == "color"))
maxWidth, _ := strconv.Atoi(c.Options["maxwidth"])
pg.WrapContent(c.Width-1, maxWidth, (c.Options["theme"] == "color"))
c.PageState.Add(pg)
c.SetPercentRead()
c.ClearMessage()
@ -1097,7 +1102,8 @@ func (c *client) handleFinger(u Url) {
return
}
pg := MakePage(u, content, []string{})
pg.WrapContent(c.Width-1, (c.Options["theme"] == "color"))
maxWidth, _ := strconv.Atoi(c.Options["maxwidth"])
pg.WrapContent(c.Width-1, maxWidth, (c.Options["theme"] == "color"))
c.PageState.Add(pg)
c.SetPercentRead()
c.ClearMessage()
@ -1117,7 +1123,8 @@ func (c *client) handleWeb(u Url) {
return
}
pg := MakePage(u, page.Content, page.Links)
pg.WrapContent(c.Width-1, (c.Options["theme"] == "color"))
maxWidth, _ := strconv.Atoi(c.Options["maxwidth"])
pg.WrapContent(c.Width-1, maxWidth, (c.Options["theme"] == "color"))
c.PageState.Add(pg)
c.SetPercentRead()
c.ClearMessage()

1
defaults.go
View File

@ -56,6 +56,7 @@ var defaultOptions = map[string]string{
"theme": "normal", // "normal", "inverted", "color"
"timeout": "15", // connection timeout for gopher/gemini in seconds
"webmode": "none", // "none", "gui", "lynx", "w3m", "elinks"
"maxwidth": "100",
}
// homePath will return the path to your home directory as a string

7
page.go
View File

@ -65,12 +65,15 @@ func (p *Page) RenderImage(width int) {
// width and updates the WrappedContent
// of the Page struct width a string slice
// of the wrapped data
func (p *Page) WrapContent(width int, color bool) {
func (p *Page) WrapContent(width, maxWidth int, color bool) {
if p.FileType == "image" {
p.RenderImage(width)
return
}
width = min(width, 100)
if maxWidth < 100 {
maxWidth = 100
sloum commented 2022-02-07 17:44:24 +00:00
Outdated
Review
Copy Link

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?

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
Outdated
Review
Copy Link

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
}
width = min(width, maxWidth)
counter := 0
spacer := ""
var content strings.Builder

10
page_test.go
View File

@ -20,8 +20,9 @@ func Test_WrapContent_Wrapped_Line_Length(t *testing.T) {
Color bool
}
type args struct {
width int
color bool
width int
maxWidth int
color bool
}
// create a Url for use by the MakePage function
@ -41,6 +42,7 @@ func Test_WrapContent_Wrapped_Line_Length(t *testing.T) {
"",
},
args{
10,
10,
false,
},
@ -57,6 +59,7 @@ func Test_WrapContent_Wrapped_Line_Length(t *testing.T) {
"",
},
args{
10,
10,
false,
},
@ -77,6 +80,7 @@ func Test_WrapContent_Wrapped_Line_Length(t *testing.T) {
"",
},
args{
10,
10,
false,
},
@ -85,7 +89,7 @@ func Test_WrapContent_Wrapped_Line_Length(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
p := MakePage(url, tt.input, []string{""})
p.WrapContent(tt.args.width-1, tt.args.color)
p.WrapContent(tt.args.width-1, tt.args.maxWidth, tt.args.color)
if !reflect.DeepEqual(p.WrappedContent, tt.expects) {
t.Errorf("Test failed - %s\nexpects %s\nactual %s", tt.name, tt.expects, p.WrappedContent)
}

4
pages.go
View File

@ -60,7 +60,7 @@ func (p *Pages) Add(pg Page) {
// Render wraps the content for the current page and returns
// the page content as a string slice
func (p *Pages) Render(termHeight, termWidth int, color bool) []string {
func (p *Pages) Render(termHeight, termWidth, maxWidth int, color bool) []string {
if p.Length < 1 {
return make([]string, 0)
}
@ -68,7 +68,7 @@ func (p *Pages) Render(termHeight, termWidth int, color bool) []string {
prev := len(p.History[p.Position].WrappedContent)
if termWidth != p.History[p.Position].WrapWidth || p.History[p.Position].Color != color {
p.History[p.Position].WrapContent(termWidth, color)
p.History[p.Position].WrapContent(termWidth, maxWidth, color)
}
now := len(p.History[p.Position].WrappedContent)