mirror of
https://github.com/rivo/tview.git
synced 2026-04-26 21:35:54 +03:00
[GH-ISSUE #451] TextView: limiting the maximum number of lines #322
Labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/tview#322
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
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?
Originally created by @jpeletier on GitHub (May 31, 2020).
Original GitHub issue: https://github.com/rivo/tview/issues/451
Hi,
I am using
TextViewto 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
@rivo commented on GitHub (Sep 15, 2020):
Instead of adding a
SetMaxLines()function, I would suggest that I add aGetLineCount()as well as aRemoveLines()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.
@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:
RemoveLines(n)without first callingGetLineCount()to do some calculation.TextViewyou as a developer user want to set some sort of limit on the resources it is going to be using, soSetMaxLines()or the like looks like a good design pattern to me.TextViewis anio.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 ofTextView. i.e., without this,TextViewstill 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.SetMaxLines()is not incompatible with a futureRemoveLines(n)+GetLineCount(), which can be promptly added if the need actually arises.RemoveLines(n)andGetLineCount(), I think I'd just rather maket.bufferpublic instead which would enable some text editing / subclassing. But again, that is trying to guess needs before they come.To summarize, I would ask you to reconsider #452, consider there must be at least some sort of memory management for
TextViewand let people propose and implementRemoveLines(n)andGetLineCount()when they actually need it.Thanks
Javier
@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 larget.buffer, but since you don't want PRs, that leaves you alone I guess.). Please reconsider.@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
wordWrapset totrue(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 intoTextView? (As an example, I do this all the time withvimwhen examining JSON files.)My thought was that even when you use
TextViewas aio.Writer, you can still use thechangedevent to listen to additions to yourTextView. 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 returnt.bufferwhich is a string slice. And keeping the full string in parallel is a burden for those who never callGetText().pps. In general, I didn't anticipate that people would use
TextViewfor 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.
@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
TextViewwhich becomes unstable beyond a certain text size. That should be addressed anyhow.Again, your proposal would work (although it would have me wrap
TextViewinto 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.
@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 whenwordWrapis set totrueand 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.
@jpeletier commented on GitHub (Jan 15, 2021):
I can think of the following:
high_waterandlow_waterpropertiestotalnumber of bytes currently in the array of strings. (totalis always equal to the sum of the lengths of all the strings in the buffer)totalgoes above thehigh_waterthreshold, remove enough content from the buffer to make thetotalbe equal or underlow_water. Here we can have two options:totalbecomes thelow_watermarkvalue. This covers the superlong lines use case.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_watermarkbecomes effectively the maximum memory consumption of theTextView@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?
@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
@jpeletier commented on GitHub (Jan 17, 2021):
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.
@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)
@rivo commented on GitHub (Jan 25, 2021):
Ok, so I'm seeing two requirements here:
With the solution of
nlines, 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 to2, twice as many are kept. And so on. (I realize that even if you set it to1, 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.
@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?
@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
TextViewdiscard 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
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.
@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.
@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.
@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:
tview. But it's not my day job so some features will take longer. Yes, months in some cases.Anyway, I'll get back to this soon with an update.
@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
@rivo commented on GitHub (Feb 15, 2021):
Because I'm certain that I'm going to get requests like "I enabled word wrapping in
TextViewand 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.)
@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).@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.
@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,).
@rivo commented on GitHub (Apr 26, 2021):
I'm closing this now. If there are any problems with this, open a new issue.