[GH-ISSUE #451] TextView: limiting the maximum number of lines #322

Closed
opened 2026-03-04 01:03:58 +03:00 by kerem · 23 comments
Owner

Originally created by @jpeletier on GitHub (May 31, 2020).
Original GitHub issue: https://github.com/rivo/tview/issues/451

Hi,

I am using TextView to display scrolling log output, but as soon as the text in the buffer becomes very big, CPU usage shoots up.

I propose solving it by adding a new SetMaxLines() method that makes sure the buffer slice never contains more lines than the set maximum. By default, it is set to infinite, therefore this would be a backward-compatible change.

Proposed PR: #452

Originally created by @jpeletier on GitHub (May 31, 2020). Original GitHub issue: https://github.com/rivo/tview/issues/451 Hi, I am using `TextView` to display scrolling log output, but as soon as the text in the buffer becomes very big, CPU usage shoots up. I propose solving it by adding a new `SetMaxLines()` method that makes sure the buffer slice never contains more lines than the set maximum. By default, it is set to infinite, therefore this would be a backward-compatible change. Proposed PR: #452
kerem closed this issue 2026-03-04 01:03:59 +03:00
Author
Owner

@rivo commented on GitHub (Sep 15, 2020):

Instead of adding a SetMaxLines() function, I would suggest that I add a GetLineCount() as well as a RemoveLines() function. This may help others solve problems that are not just specific to your situation.

Please let me know if this would work for you.

<!-- gh-comment-id:692627232 --> @rivo commented on GitHub (Sep 15, 2020): Instead of adding a `SetMaxLines()` function, I would suggest that I add a `GetLineCount()` as well as a `RemoveLines()` function. This may help others solve problems that are not just specific to your situation. Please let me know if this would work for you.
Author
Owner

@jpeletier commented on GitHub (Sep 15, 2020):

Hi @rivo and thank you for this project.

While this would not be a deal killer, I think it would be unnecessary for the following:

  • #452 is already implemented with tests, so it can be merged and we're done. You're suggesting to implement these two functions yourself, which puts a burden on you, and guessing what people may need as opposed to waiting for such need to come.
  • I don't see why would anybody call RemoveLines(n) without first calling GetLineCount() to do some calculation.
  • When you instantiate a buffer-like structure like TextView you as a developer user want to set some sort of limit on the resources it is going to be using, so SetMaxLines() or the like looks like a good design pattern to me.
  • TextView is an io.Writer, therefore with your suggested pattern I won't be able to just pass it around other parts of the program without wrapping it first in order to have the opportunity to call these two functions and have some way of limiting the memory usage of TextView. i.e., without this, TextView still needs "external management" in order to be stable. Remember that, as of today, writing to it indefinitely ends up stalling your application. There is no warning nor exception.
  • I think having SetMaxLines() is not incompatible with a future RemoveLines(n) + GetLineCount(), which can be promptly added if the need actually arises.
  • If we're going to go in the direction of RemoveLines(n) and GetLineCount(), I think I'd just rather make t.buffer public instead which would enable some text editing / subclassing. But again, that is trying to guess needs before they come.
  • The proposal here got a thumbs up emoji from at least two enthusiastic users 😉.

To summarize, I would ask you to reconsider #452, consider there must be at least some sort of memory management for TextView and let people propose and implement RemoveLines(n) and GetLineCount() when they actually need it.

Thanks
Javier

<!-- gh-comment-id:692967419 --> @jpeletier commented on GitHub (Sep 15, 2020): Hi @rivo and thank you for this project. While this would not be a deal killer, I think it would be unnecessary for the following: * #452 is already implemented with tests, so it can be merged and we're done. You're suggesting to implement these two functions yourself, which puts a burden on you, and guessing what people may need as opposed to waiting for such need to come. * I don't see why would anybody call `RemoveLines(n)` without first calling `GetLineCount()` to do some calculation. * When you instantiate a buffer-like structure like `TextView` you as a developer user want to set some sort of limit on the resources it is going to be using, so `SetMaxLines()` or the like looks like a good design pattern to me. * `TextView` is an `io.Writer`, therefore with your suggested pattern I won't be able to just pass it around other parts of the program without wrapping it first in order to have the opportunity to call these two functions and have some way of limiting the memory usage of `TextView`. i.e., without this, `TextView` still needs "external management" in order to be stable. Remember that, as of today, writing to it indefinitely ends up stalling your application. There is no warning nor exception. * I think having `SetMaxLines()` is not incompatible with a future `RemoveLines(n)` + `GetLineCount()`, which can be promptly added if the need actually arises. * If we're going to go in the direction of `RemoveLines(n)` and `GetLineCount()`, I think I'd just rather make `t.buffer` public instead which would enable some text editing / subclassing. But again, that is trying to guess needs before they come. * The proposal here got a thumbs up emoji from at least two enthusiastic users 😉. To summarize, I would ask you to reconsider #452, consider there must be at least some sort of memory management for `TextView` and let people propose and implement `RemoveLines(n)` and `GetLineCount()` when they actually need it. Thanks Javier
Author
Owner

@jpeletier commented on GitHub (Sep 15, 2020):

Update: I have just come across your podcast (12:37) whereupon you displicently dismiss my work on camera, patronizingly suggesting I am only thinking about myself.

I feel that was really unfair and I must say that I couldn't think more about others by the fact that I am sharing my work for anyone to use for free and as soon as possible. I am pretty much sure that I am not wrong by guessing that I am not the only one tailing logs and need a way to cap buffer accumulating. Proof: #266. I am sure @urandom and @foxcpp could use this feature out of the box. @tslocum already added it to cview. Tailing a log is a pretty common use case. All tools to open large text files must have a maximum buffer limit. You will never be able to contain the whole thing in memory. In @urandom 's use case, the log is just ongoing, infinite. No matter how much you optimize writing to an evergrowing buffer, at some point, it'll blow up.

Additionally, I most carefully reviewed your code to understand your patterns and naming in order to make sure that I write every single line of code and comments in the PR, including tests, respecting the "look and feel" of the original code. This you didn't even look at, and it is very small and concise. Instead, in the video you look at your own code randomly trying to find I don't know what and making some unrelated changes. (btw, you have a performance trap in GetText() due to an unnecesary copy of a potentially very large t.buffer, but since you don't want PRs, that leaves you alone I guess.). Please reconsider.

<!-- gh-comment-id:693028021 --> @jpeletier commented on GitHub (Sep 15, 2020): Update: I have just come across [your podcast (12:37)](https://www.twitch.tv/videos/741666357?t=00h12m37s) whereupon you displicently dismiss my work on camera, patronizingly suggesting I am only thinking about myself. I feel that was really unfair and I must say that I couldn't think more about others by the fact that I am sharing my work for anyone to use for free and as soon as possible. I am pretty much sure that I am not wrong by guessing that I am not the only one tailing logs and need a way to cap buffer accumulating. Proof: #266. I am sure @urandom and @foxcpp could use this feature out of the box. @tslocum already added it to [cview](https://gitlab.com/tslocum/cview/-/merge_requests/3). Tailing a log is a pretty common use case. All tools to open large text files must have a maximum buffer limit. You will never be able to contain the whole thing in memory. In @urandom 's use case, the log is just ongoing, infinite. No matter how much you optimize writing to an evergrowing buffer, at some point, it'll blow up. Additionally, I most carefully reviewed your code to understand your patterns and naming in order to make sure that I write every single line of code and comments in the PR, including tests, respecting the "look and feel" of the original code. This you didn't even look at, and it is very small and concise. Instead, in the video you look at your own code randomly trying to find I don't know what and making some unrelated changes. (btw, you have a performance trap in `GetText()` due to an unnecesary copy of a potentially very large `t.buffer`, but since you don't want PRs, that leaves you alone I guess.). Please reconsider.
Author
Owner

@rivo commented on GitHub (Jan 11, 2021):

I'm getting back to this now. I was going to check out what I said in the video but it's removed by now so unfortunately, I don't know anymore what was said. If it came across dismissive in any way, then please accept my sincere apologies. I'm quite sure it wasn't meant that way at all. It may have been that I wasn't going to get into it at the time of the video. This issue is still open, which means I'm still getting back to it and I'm taking it seriously.

I understand all of your points. The reason I haven't simply merged your PR was that I'm trying to figure out how much SetMaxLines() really only solves your problem specifically or a general problem that many people are having. What tends to happen is that, say, someone else comes along and says "SetMaxLines() always removes x lines at the top but I want to remove x lines in the middle somewhere." (or something like that). So I'm trying to give users something with a bit more control over what gets deleted and when.

There is also the issue of text that has no or only few newline characters. With wordWrap set to true (which is likely often the case),SetMaxLines() will not work as expected. How can we set a maximum for users who pipe very long lines into TextView? (As an example, I do this all the time with vim when examining JSON files.)

My thought was that even when you use TextView as a io.Writer, you can still use the changed event to listen to additions to your TextView. If you then have functions to clear out your buffer any way you need, I believe you should be fine. Of course, it's not the simple one-method solution you proposed. But it may help those other users with different requirements.

Love to hear your thoughts.

ps. Regarding GetText(), the idea wasn't to return t.buffer which is a string slice. And keeping the full string in parallel is a burden for those who never call GetText().

pps. In general, I didn't anticipate that people would use TextView for very large texts. It wasn't really built for that. (There are other GitHub issues about this topic.) But I guess I have to make the best out of the situation.

ppps. I do accept PRs.

<!-- gh-comment-id:757952581 --> @rivo commented on GitHub (Jan 11, 2021): I'm getting back to this now. I was going to check out what I said in the video but it's removed by now so unfortunately, I don't know anymore what was said. If it came across dismissive in any way, then please accept my sincere apologies. I'm quite sure it wasn't meant that way at all. It may have been that I wasn't going to get into it at the time of the video. This issue is still open, which means I'm still getting back to it and I'm taking it seriously. I understand all of your points. The reason I haven't simply merged your PR was that I'm trying to figure out how much `SetMaxLines()` really only solves your problem specifically or a general problem that many people are having. What tends to happen is that, say, someone else comes along and says "`SetMaxLines()` always removes x lines at the top but I want to remove x lines in the middle somewhere." (or something like that). So I'm trying to give users something with a bit more control over what gets deleted and when. There is also the issue of text that has no or only few newline characters. With `wordWrap` set to `true` (which is likely often the case),`SetMaxLines()` will not work as expected. How can we set a maximum for users who pipe very long lines into `TextView`? (As an example, I do this all the time with `vim` when examining JSON files.) My thought was that even when you use `TextView` as a `io.Writer`, you can still use the `changed` event to listen to additions to your `TextView`. If you then have functions to clear out your buffer any way you need, I believe you should be fine. Of course, it's not the simple one-method solution you proposed. But it may help those other users with different requirements. Love to hear your thoughts. ps. Regarding `GetText()`, the idea wasn't to return `t.buffer` which is a string slice. And keeping the full string in parallel is a burden for those who never call `GetText()`. pps. In general, I didn't anticipate that people would use `TextView` for very large texts. It wasn't really built for that. (There are other GitHub issues about this topic.) But I guess I have to make the best out of the situation. ppps. I do accept PRs.
Author
Owner

@jpeletier commented on GitHub (Jan 14, 2021):

Hi @rivo and thank you for your comments.

I ask that you please re-read my first reply as an answer to your last post.

We're stuck in a loop I think. You propose a different implementation that would work. The difference is that the one I am proposing is ready to use and people would benefit immediately.

btw, it also adds some memory management to TextView which becomes unstable beyond a certain text size. That should be addressed anyhow.

Again, your proposal would work (although it would have me wrap TextView into a new struct to watch for changes), but I cannot dedicate the effort to implement your proposal. I'd rather stick to my fork, which is a succint change and is already working flawlessly for the last months in production.

My suggestion: merge and see how the feature is used. Let's avoid guessing what "people" may need. Should the need to change buffer in different ways (changes in the middle, etc) appear, we can always go back and evolve the code to support it.

Otherwise issues and PRs will pile up and the community will scatter around their own forks (you've got a ton already) which is really bad if we want to have a solid library that is well maintained and evolves with the needs of you and the rest of us.

<!-- gh-comment-id:760188973 --> @jpeletier commented on GitHub (Jan 14, 2021): Hi @rivo and thank you for your comments. I ask that you please re-read my [first reply](https://github.com/rivo/tview/issues/451#issuecomment-692967419) as an answer to your last post. We're stuck in a loop I think. You propose a different implementation that would work. The difference is that the one I am proposing is ready to use and people would benefit immediately. btw, it also adds some memory management to `TextView` which becomes unstable beyond a certain text size. That should be addressed anyhow. Again, your proposal would work (although it would have me wrap `TextView` into a new struct to watch for changes), but I cannot dedicate the effort to implement your proposal. I'd rather stick to my fork, which is a succint change and is already working flawlessly for the last months in production. My suggestion: merge and see how the feature is used. Let's avoid guessing what "people" may need. Should the need to change buffer in different ways (changes in the middle, etc) appear, we can always go back and evolve the code to support it. Otherwise issues and PRs will pile up and the community will scatter around their own forks (you've got a ton already) which is really bad if we want to have a solid library that is well maintained and evolves with the needs of you and the rest of us.
Author
Owner

@rivo commented on GitHub (Jan 14, 2021):

I did re-read the entire conversation before I replied. I know what you wrote initially and I've seen your PR.

With my last comment, I was hoping that you would present a suggestion for TextView's memory management when wordWrap is set to true and we're dealing with content where lines break over (i.e. few newline characters). I know this goes beyond what you need for your application but that's exactly what I'm getting at: I would like for PR submissions to keep other users of the package in mind, too. (And this word-wrap issue is not "guessing what people may need" because as I said, I face this issue often myself.)

I'll happily admit that my first proposal does not solve this problem either so it's not a good solution. I also didn't ask you to implement that proposal.

Anyway, I'll think of a solution myself then.

<!-- gh-comment-id:760314235 --> @rivo commented on GitHub (Jan 14, 2021): I did re-read the entire conversation before I replied. I know what you wrote initially and I've seen your PR. With my last comment, I was hoping that you would present a suggestion for `TextView`'s memory management when `wordWrap` is set to `true` and we're dealing with content where lines break over (i.e. few newline characters). I know this goes beyond what you need for your application but that's exactly what I'm getting at: I would like for PR submissions to keep other users of the package in mind, too. (And this word-wrap issue is not "guessing what people may need" because as I said, I face this issue often myself.) I'll happily admit that my first proposal does not solve this problem either so it's not a good solution. I also didn't ask you to implement that proposal. Anyway, I'll think of a solution myself then.
Author
Owner

@jpeletier commented on GitHub (Jan 15, 2021):

I can think of the following:

  1. Define a configurabe high_water and low_water properties
  2. On every write, keep track of the total number of bytes currently in the array of strings. (total is always equal to the sum of the lengths of all the strings in the buffer)
  3. If the total goes above the high_water threshold, remove enough content from the buffer to make the total be equal or under low_water. Here we can have two options:
  • if "remove entire lines" is enabled, the cleanup algorithm will delete entire lines until the buffer goes under the low water level. This covers the tail logs use case
  • if "remove entire lines" is disabled, the cleanup algorithm will delete enough lines and the exact amount of content from the last line so that the new total becomes the low_watermark value. This covers the superlong lines use case.
  1. The cleanup algorithm should never remove content that is currently visible, that is, if the user selects a low water value that is smaller than the capacity of the viewport, then the capacity of the viewport becomes the effective low water value to use.

This is not ideal for me as I am just looking to tail logs (thus want entire lines), and dimensioning the capacity in terms of lines is a good enough estimation as lines are never infinitely long. Nevertheless I could have this work. Instead of SetMaxLines(x), I would set a low water value of x * average_line_length or something along these lines.

In any case, high_watermark becomes effectively the maximum memory consumption of the TextView

<!-- gh-comment-id:760893112 --> @jpeletier commented on GitHub (Jan 15, 2021): I can think of the following: 1. Define a configurabe `high_water` and `low_water` properties 2. On every write, keep track of the `total` number of bytes currently in the array of strings. (`total` is always equal to the sum of the lengths of all the strings in the buffer) 3. If the `total` goes above the `high_water` threshold, remove enough content from the buffer to make the `total` be equal or under `low_water`. Here we can have two options: * if "remove entire lines" is enabled, the cleanup algorithm will delete entire lines until the buffer goes under the low water level. This covers the tail logs use case * if "remove entire lines" is disabled, the cleanup algorithm will delete enough lines and the exact amount of content from the last line so that the new `total` becomes the `low_watermark` value. This covers the superlong lines use case. 4. The cleanup algorithm should never remove content that is currently visible, that is, if the user selects a low water value that is smaller than the capacity of the viewport, then the capacity of the viewport becomes the effective low water value to use. This is not ideal for me as I am just looking to tail logs (thus want entire lines), and dimensioning the capacity in terms of lines is a good enough estimation as lines are never infinitely long. Nevertheless I could have this work. Instead of `SetMaxLines(x)`, I would set a low water value of x * average_line_length or something along these lines. In any case, `high_watermark` becomes effectively the maximum memory consumption of the `TextView`
Author
Owner

@rivo commented on GitHub (Jan 17, 2021):

Maybe a much simpler idea: Everything above the containing box gets removed with each call to Draw(). That's just one simple flag. Draw() handles word wrapping anyway so there's no need to reimplement it elsewhere.

But this would result in a situation where you lose access to anything that scrolls out the top border. I don't know if you still want to retain some information there so the user can scroll up a bit.

May you (and others in this thread) can weigh in. Do you need to allow the user to navigate vertically in these "endless" text views? Or is it a non-interactive view for you anyway?

<!-- gh-comment-id:761872099 --> @rivo commented on GitHub (Jan 17, 2021): Maybe a much simpler idea: Everything above the containing box gets removed with each call to `Draw()`. That's just one simple flag. `Draw()` handles word wrapping anyway so there's no need to reimplement it elsewhere. But this would result in a situation where you lose access to anything that scrolls out the top border. I don't know if you still want to retain some information there so the user can scroll up a bit. May you (and others in this thread) can weigh in. Do you need to allow the user to navigate vertically in these "endless" text views? Or is it a non-interactive view for you anyway?
Author
Owner

@ripienaar commented on GitHub (Jan 17, 2021):

Need user to be able to resize the box at least - if we are going the “outside the box” route then box should mean full screen dimensions

I think though the n lines approach is fine. Even with very long lines it will be a huge improvement over now

<!-- gh-comment-id:761873037 --> @ripienaar commented on GitHub (Jan 17, 2021): Need user to be able to resize the box at least - if we are going the “outside the box” route then box should mean full screen dimensions I think though the n lines approach is fine. Even with very long lines it will be a huge improvement over now
Author
Owner

@jpeletier commented on GitHub (Jan 17, 2021):

Maybe a much simpler idea: Everything above the containing box gets removed with each call to Draw(). That's just one simple flag. Draw() handles word wrapping anyway so there's no need to reimplement it elsewhere.

Usually you need to be able to scroll back a number of lines. I usually put 100 more or less.

Also, in my use case, the window can be resized, imagine a text view as application root in a console box, then you resize the console making it bigger, it would look as there is missing text.

<!-- gh-comment-id:761893478 --> @jpeletier commented on GitHub (Jan 17, 2021): > Maybe a much simpler idea: Everything above the containing box gets removed with each call to Draw(). That's just one simple flag. Draw() handles word wrapping anyway so there's no need to reimplement it elsewhere. Usually you need to be able to scroll back a number of lines. I usually put 100 more or less. Also, in my use case, the window can be resized, imagine a text view as application root in a console box, then you resize the console making it bigger, it would look as there is missing text.
Author
Owner

@buzzdan commented on GitHub (Jan 18, 2021):

@rivo i use it to view logs, and yes i scroll up (usually with the mouse scroller or touchpad on my mac)

<!-- gh-comment-id:762269505 --> @buzzdan commented on GitHub (Jan 18, 2021): @rivo i use it to view logs, and yes i scroll up (usually with the mouse scroller or touchpad on my mac)
Author
Owner

@rivo commented on GitHub (Jan 25, 2021):

Ok, so I'm seeing two requirements here:

  • The minimum number of lines needs to be the number of lines on the screen.
  • Often times, you'd want to retain some buffer that can be scrolled back to.

With the solution of n lines, you need to know how large the console of the user is. That kind of information is not easily available to your application, though. So the best you could do in this case is guess.

How about setting it to a "multiple of the number of lines on screen" instead? If you set it to 1, everything that's beyond the number of lines available to your application will be discarded. If you set it to 2, twice as many are kept. And so on. (I realize that even if you set it to 1, the user could still resize their window. But that's something we cannot control at all. And after all, you can still set it to a higher number.)

Let me know if this would work for you.

<!-- gh-comment-id:766663060 --> @rivo commented on GitHub (Jan 25, 2021): Ok, so I'm seeing two requirements here: - The minimum number of lines needs to be the number of lines on the screen. - Often times, you'd want to retain some buffer that can be scrolled back to. With the solution of `n` lines, you need to know how large the console of the user is. That kind of information is not easily available to your application, though. So the best you could do in this case is guess. How about setting it to a "multiple of the number of lines on screen" instead? If you set it to `1`, everything that's beyond the number of lines available to your application will be discarded. If you set it to `2`, twice as many are kept. And so on. (I realize that even if you set it to `1`, the user could still resize their window. But that's something we cannot control at all. And after all, you can still set it to a higher number.) Let me know if this would work for you.
Author
Owner

@rivo commented on GitHub (Feb 14, 2021):

No thoughts anymore? I mean, if what I wrote in my last comment (i.e. not knowing how large the screen or the containing box is) is not an issue in your projects, maybe it's not very important?

<!-- gh-comment-id:778773225 --> @rivo commented on GitHub (Feb 14, 2021): No thoughts anymore? I mean, if what I wrote in my last comment (i.e. not knowing how large the screen or the containing box is) is not an issue in your projects, maybe it's not very important?
Author
Owner

@jpeletier commented on GitHub (Feb 14, 2021):

@rivo, I don't know about the others, but it is very hard to discuss in a loop about such a simple feature. During the 8 months this has been open, I'd rather have got the ball rolling with #452 and 10 further evolutionary PRs based on real feedback than one hyper-discussed issue that is going nowhere trying to guess what would work. In the meantime, people fork this project and leave.

Back to the subject, with your last proposal, for example, the user may reduce the window size, which would make TextView discard data accordingly. Thus, if the user then resizes the window back to the original size or bigger, the discarded data would then not be shown.

For your two detected requirements:

The minimum number of lines needs to be the number of lines on the screen.
Often times, you'd want to retain some buffer that can be scrolled back to.

The SetMaxLine() approach in #452 gets the job done, because what people are going to do is just set a very large buffer of, say, 500 lines or whatever and go on with their lives. Virtually all software that works with console buffers and, even some text editors, use lines as the unit of measure. Not perfect, not exact, but a good enough compromise.

After all this time, I think there is a solid consensus in this discussion around just setting a max lines auto-discard feature (approve #452) and close this issue.

Anyone in favor, please thumbs up the opening post.
Against, please thumbs down the opening post.

<!-- gh-comment-id:778832689 --> @jpeletier commented on GitHub (Feb 14, 2021): @rivo, I don't know about the others, but it is very hard to discuss in a loop about such a simple feature. During the 8 months this has been open, I'd rather have got the ball rolling with #452 and 10 further evolutionary PRs based on **real feedback** than one hyper-discussed issue that is going nowhere trying to guess what would work. In the meantime, people fork this project and leave. Back to the subject, with your last proposal, for example, the user may reduce the window size, which would make `TextView` discard data accordingly. Thus, if the user then resizes the window back to the original size or bigger, the discarded data would then not be shown. For your two detected requirements: > The minimum number of lines needs to be the number of lines on the screen. > Often times, you'd want to retain some buffer that can be scrolled back to. The `SetMaxLine()` approach in #452 gets the job done, because what people are going to do is just set a very large buffer of, say, 500 lines or whatever and go on with their lives. Virtually all software that works with console buffers and, even some text editors, use lines as the unit of measure. Not perfect, not exact, but a good enough compromise. After all this time, I think there is a solid consensus in this discussion around just setting a max lines auto-discard feature (approve #452) and close this issue. **Anyone in favor, please thumbs up the [opening post](https://github.com/rivo/tview/issues/451#issue-627945558)**. **Against, please thumbs down the [opening post](https://github.com/rivo/tview/issues/451#issue-627945558)**.
Author
Owner

@ripienaar commented on GitHub (Feb 14, 2021):

Agree this is way too much discussion for a basic thing. Less hassle to pick a different package and leave this.

SetMaxLine() would work just fine.

<!-- gh-comment-id:778833149 --> @ripienaar commented on GitHub (Feb 14, 2021): Agree this is way too much discussion for a basic thing. Less hassle to pick a different package and leave this. SetMaxLine() would work just fine.
Author
Owner

@joegrasse commented on GitHub (Feb 15, 2021):

@rivo I totally appreciate you trying to think through this problem and all the hard work you have done on this project. The best solution for me is like a scrollback buffer in a terminal. Set max number of lines (default unlimited) and the oldest rolls off as @jpeletier suggested. I have not looked at that implementation, so I can just agree to the idea.

<!-- gh-comment-id:779226049 --> @joegrasse commented on GitHub (Feb 15, 2021): @rivo I totally appreciate you trying to think through this problem and all the hard work you have done on this project. The best solution for me is like a scrollback buffer in a terminal. Set max number of lines (default unlimited) and the oldest rolls off as @jpeletier suggested. I have not looked at that implementation, so I can just agree to the idea.
Author
Owner

@rivo commented on GitHub (Feb 15, 2021):

Thanks for the feedback everyone. I will look into implementing this next. Although I will look at the PR again, I'm suspecting it will not be merged as it doesn't handle word-wrapping.

Commenting on two more points that were brought up:

  • This item having been open for a long time: I have commented on this in other threads already. Things move slowly in this project but they move. I've seen so many fast-paced projects that ended up as abandonware. I'm not planning on letting this happen to tview. But it's not my day job so some features will take longer. Yes, months in some cases.
  • Having a long discussion about this: There are two reasons for this. Firstly, it's my job to think of all the corner cases affected by the proposal. I've had very few PR submissions that handled all corner cases properly. (I just worked on one that seemed easy at first but ended up taking many hours and code changes to implement.) Secondly, this specific issue describes a situation I hadn't encountered myself before. So I'm asking questions to understand your requirements. Solutions are not requirements and while the solution proposed in a PR seems obvious to the submitter, it may not be the right way to meet the requirements of the users. Surely, if we were all in the same room, this could've been settled very quickly. But this is an offline conversation with long breaks between posts so it takes a while.

Anyway, I'll get back to this soon with an update.

<!-- gh-comment-id:779376623 --> @rivo commented on GitHub (Feb 15, 2021): Thanks for the feedback everyone. I will look into implementing this next. Although I will look at the PR again, I'm suspecting it will not be merged as it doesn't handle word-wrapping. Commenting on two more points that were brought up: - _This item having been open for a long time:_ I have commented on this in other threads already. Things move slowly in this project but they move. I've seen so many fast-paced projects that ended up as abandonware. I'm not planning on letting this happen to `tview`. But it's not my day job so some features will take longer. Yes, months in some cases. - _Having a long discussion about this:_ There are two reasons for this. Firstly, it's my job to think of all the corner cases affected by the proposal. I've had very few PR submissions that handled all corner cases properly. (I just worked on one that seemed easy at first but ended up taking many hours and code changes to implement.) Secondly, this specific issue describes a situation I hadn't encountered myself before. So I'm asking questions to understand your requirements. Solutions are not requirements and while the solution proposed in a PR seems obvious to the submitter, it may not be the right way to meet the requirements of the users. Surely, if we were all in the same room, this could've been settled very quickly. But this is an offline conversation with long breaks between posts so it takes a while. Anyway, I'll get back to this soon with an update.
Author
Owner

@jpeletier commented on GitHub (Feb 15, 2021):

I am intrigued, why do you need to handle word wrapping for this feature?
Please look at the PR in detail (an efficient and succint change).

Thanks

<!-- gh-comment-id:779382106 --> @jpeletier commented on GitHub (Feb 15, 2021): I am intrigued, why do you need to handle word wrapping for this feature? Please look at the PR in detail (an efficient and succint change). Thanks
Author
Owner

@rivo commented on GitHub (Feb 15, 2021):

Because I'm certain that I'm going to get requests like "I enabled word wrapping in TextView and it is very slow despite my 100-line limit." In fact, the PR I mentioned which I worked on today was about handling long lines which I hadn't anticipated in that context. We're at issue 567 as of today, i.e. I've seen my share of requests for this project. Some issues have become quite predictable. ("It doesn't work with Chinese characters or emojis" is another one that almost always pops up.)

It's ok, though, I already have an idea on how to do this.

(Btw, I have looked at the PR in detail.)

<!-- gh-comment-id:779404685 --> @rivo commented on GitHub (Feb 15, 2021): Because I'm certain that I'm going to get requests like "I enabled word wrapping in `TextView` and it is very slow despite my 100-line limit." In fact, the PR I mentioned which I worked on today was about handling long lines which I hadn't anticipated in that context. We're at issue 567 as of today, i.e. I've seen my share of requests for this project. Some issues have become quite predictable. ("It doesn't work with Chinese characters or emojis" is another one that almost always pops up.) It's ok, though, I already have an idea on how to do this. (Btw, I have looked at the PR in detail.)
Author
Owner

@rivo commented on GitHub (Feb 16, 2021):

The latest commit introduces TextView.SetMaxLines(). I'll keep this issue open for a while in case you find any bugs or if it doesn't work as one would expect.

This did in the end take a few hours to implement. It turns out there are more things to take care of than just word-wrapping. For example, if the cutoff point for removed lines comes after a colour tag, it must be reinserted at the beginning of the buffer or else there will be unexpected changes in colour. Same with region tags: If a region gets cut in half, the remaining half must still be highlightable. Finally, these side effects must also be documented properly (e.g. GetText() is affected by these changes).

<!-- gh-comment-id:780116886 --> @rivo commented on GitHub (Feb 16, 2021): The latest commit introduces `TextView.SetMaxLines()`. I'll keep this issue open for a while in case you find any bugs or if it doesn't work as one would expect. This did in the end take a few hours to implement. It turns out there are more things to take care of than just word-wrapping. For example, if the cutoff point for removed lines comes after a colour tag, it must be reinserted at the beginning of the buffer or else there will be unexpected changes in colour. Same with region tags: If a region gets cut in half, the remaining half must still be highlightable. Finally, these side effects must also be documented properly (e.g. `GetText()` is affected by these changes).
Author
Owner

@rivo commented on GitHub (Mar 11, 2021):

Has anybody tried this? Does it work as expected?

My impression was that this was a feature requested by many of you. It would be good to get some feedback here.

<!-- gh-comment-id:796912602 --> @rivo commented on GitHub (Mar 11, 2021): Has anybody tried this? Does it work as expected? My impression was that this was a feature requested by many of you. It would be good to get some feedback here.
Author
Owner

@joegrasse commented on GitHub (Mar 24, 2021):

I do plan on trying this. Still trying to work through getting my app to work with the new changes though (i.e. tcell v2,).

<!-- gh-comment-id:805991416 --> @joegrasse commented on GitHub (Mar 24, 2021): I do plan on trying this. Still trying to work through getting my app to work with the new changes though (i.e. tcell v2,).
Author
Owner

@rivo commented on GitHub (Apr 26, 2021):

I'm closing this now. If there are any problems with this, open a new issue.

<!-- gh-comment-id:827010297 --> @rivo commented on GitHub (Apr 26, 2021): I'm closing this now. If there are any problems with this, open a new issue.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
starred/tview#322
No description provided.