Line wrap correction #176
No reviewers
Labels
No Label
blocked
bug
build
documentation
duplicate
enhancement
finger
gemini
gopher
help wanted
http
in progress
invalid
local
needs-info
non-code
non-functional
non-urgent
question
release
rendering
suggestion
telnet
terminal
urgent
wontfix
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sloum/bombadillo#176
Loading…
Reference in New Issue
No description provided.
Delete Branch "line-wrap-correction"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Intended to fix #175
When a line is wrapped, the counter is set to 0 but not incremented for the character that is added. This change intends to correct this.
@ -130,6 +130,7 @@ func (p *Page) WrapContent(width int, color bool) {
counter += len(spacer)
}
content.WriteRune(ch)
counter++
The counter is set to 0 on line 127, we write the character on line 132 and then here we increment it.
Instead of incrementing the counter, we could just set it to 1. This might not be as easy to read though, and may require a comment to understand.
@sloum happy to do this in whatever style you prefer.
counter++
seems fine to meProvided unit tests used for debugging. The test is based on a template created using GoTests.
As noted in the issue, there is some difference in terminals that might add some complexity here. While this works with gnome-terminal, with st and rxvt-unicode the final character in a line appears blank. The window decorations and bookmarks bar are drawn normally though, this only appears to affect page content.
Awesome! Thanks for adding the unit tests. :)
I have yet to pull this down and try it out (it has been a busy weekend), but I will try to do so tonight. I think rather than going into
develop
we should make a minor release branch as I have a feature that is almost ready (improved command error messaging and in program help via:help [keyword]
. It would reduce the number of releases on the releases page of tildegit to do them together. or maybe it isnt a big deal and I'm overthinking it? What do you think?Also, do you think this is a big enough issue that it needs to make it to master? Or can this hang on develop for awhile?
I think this is minor enough to go in to a release branch and wait a bit before entering master. It's such a small change that it might even be best for you to push your changes to the release branch first? Just let me know how you would like to proceed.
Another update that seems to fix the issue where the final character of content was not being printed.
In client.go's
Draw()
, lines of content are written, followed by\033[0K
and then\n
. Reordering to write\033[0K
before the content seems to correct this.I don't really understand this but it seems to work.
That makes sense to me. It should be fine to clear first and then print. Have you noticed any change in render time when scrolling with j/k after making the change?
I'm sorry I have been lagging on addressing this PR. Life has gotten in the way a lot, and then I fogot that it was here :(
I can't see a performance difference through direct observation on my system as well as a remote server. It would be good to get more eyes on this though.
Benchmarking might be another option.
Draw()
could be executed on a client struct with some test data. Getting the struct to have good testing data, and understanding the benchmark output are the challenges. A quick try shows it is possible, and for this change there seems to be no difference in the results. I can share this if you want to see it.Also, no worries on the delay. You've been clear about what people can expect from your time on this project, and this has been a good learning opportunity for me.
I just pulled, built, and ran. This is a great fix. It felt snappy and fixed a really borked dispaly issue I was having on my enw site (
gemini://gemlog.blue
).I would like to hold off on merging for a bit to see if I can punt in a few other things into a single release (help documentation, configurable tcp request timeouts, and a timeout for gemini's tls connections). I'll post details on each soon.
If you want to try out the, incomplete, help system pull the branch
help-integration
. You will need to move the files by runningmake install-help
and then building the branch. Then you can run:help help
for the help on how to use the help system.WIP Line wrap correctionto Line wrap correctionOh looks like you can't change the merge target by editing the PR. I'll close this and raise a new one.
Pull request closed