Line wrap correction #176

asdf · 2020-06-21T04:29:48Z

asdf commented

2020-06-21 04:29:48 +00:00

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.

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.

asdf referenced this pull request

2020-06-21 04:32:36 +00:00

possible text wrapping issue with console width less than 102 characters. #175

asdf reviewed 2020-06-21 04:38:49 +00:00

page.go

						
				@ -130,6 +130,7 @@ func (p *Page) WrapContent(width int, color bool) {

									counter += len(spacer)

								}

								content.WriteRune(ch)

								counter++

asdf commented

2020-06-21 04:38:49 +00:00

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.

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.

sloum commented

2020-06-22 16:17:44 +00:00

counter++ seems fine to me

`counter++` seems fine to me

asdf commented

2020-06-21 05:02:26 +00:00

Provided unit tests used for debugging. The test is based on a template created using GoTests.

Provided unit tests used for debugging. The test is based on a template created using [GoTests](https://github.com/cweill/gotests).

asdf commented

2020-06-21 05:17:10 +00:00

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.

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.

sloum commented

2020-06-22 16:20:38 +00:00

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?

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?

asdf commented

2020-06-23 03:32:59 +00:00

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.

asdf commented

2020-06-23 05:26:15 +00:00

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.

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.

sloum commented

2020-06-27 21:15:29 +00:00

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 :(

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 :(

asdf commented

2020-06-28 04:44:31 +00:00

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 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.

sloum commented

2020-06-30 04:31:20 +00:00

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 running make install-help and then building the branch. Then you can run: help help for the help on how to use the help system.

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 running `make install-help` and then building the branch. Then you can run: `help help` for the help on how to use the help system.

🎉 1

asdf changed title from ~~WIP Line wrap correction~~ to Line wrap correction

2020-07-01 04:13:29 +00:00

asdf commented

2020-07-01 05:25:29 +00:00

Oh looks like you can't change the merge target by editing the PR. I'll close this and raise a new one.

👍 1

asdf closed this pull request

2020-07-01 05:25:31 +00:00

asdf referenced this pull request

2020-07-01 05:27:42 +00:00

Line wrap correction #179