[GH-ISSUE #131] Feature: make some util methods visible #102

Closed
opened 2026-03-04 01:01:57 +03:00 by kerem · 6 comments
Owner

Originally created by @benweidig on GitHub (Jun 9, 2018).
Original GitHub issue: https://github.com/rivo/tview/issues/131

Hi,

I'm trying to implement more performant TextView, because want to dispIay syntax-highlighted/-colored code, und re-indexing takes too long for me with big data files.

So my general idea was to implement a custom TextView with "on-demand" indexing. Only problem is that some util methods are package-only.

IMO they could be useful too for tother complex widgets, so I would suggest to make them visible:

github.com/rivo/tview@398a6c2f77/util.go (L252)

github.com/rivo/tview@398a6c2f77/util.go (L167)

github.com/rivo/tview@398a6c2f77/util.go (L203)

I could prepare a PR for it if you like.

Originally created by @benweidig on GitHub (Jun 9, 2018). Original GitHub issue: https://github.com/rivo/tview/issues/131 Hi, I'm trying to implement more performant TextView, because want to dispIay syntax-highlighted/-colored code, und re-indexing takes too long for me with big data files. So my general idea was to implement a custom TextView with "on-demand" indexing. Only problem is that some util methods are package-only. IMO they could be useful too for tother complex widgets, so I would suggest to make them visible: https://github.com/rivo/tview/blob/398a6c2f773ae39d830456a92b25d79797181c06/util.go#L252 https://github.com/rivo/tview/blob/398a6c2f773ae39d830456a92b25d79797181c06/util.go#L167 https://github.com/rivo/tview/blob/398a6c2f773ae39d830456a92b25d79797181c06/util.go#L203 I could prepare a PR for it if you like.
kerem closed this issue 2026-03-04 01:01:57 +03:00
Author
Owner

@rivo commented on GitHub (Jun 9, 2018):

I'm a bit reluctant to open these up because in the past, I had to change these functions to accommodate new features and improvements to the package. (One example is Print() which should have been printWithStyle() all along but now still has the old interface because of backwards compatibility.) And you can see from their signatures that these functions are quite "ugly". Not really something I'd like to offer to the public.

Instead, I could probably make improvements to TextView's indexing, which is what you are looking for anyway. Could you explain, for your case, when the buffer is reindexed but when it shouldn't? Maybe that will help me understand where improvements could be made.

<!-- gh-comment-id:395961996 --> @rivo commented on GitHub (Jun 9, 2018): I'm a bit reluctant to open these up because in the past, I had to change these functions to accommodate new features and improvements to the package. (One example is `Print()` which should have been `printWithStyle()` all along but now still has the old interface because of backwards compatibility.) And you can see from their signatures that these functions are quite "ugly". Not really something I'd like to offer to the public. Instead, I could probably make improvements to `TextView`'s indexing, which is what you are looking for anyway. Could you explain, for your case, when the buffer is reindexed but when it shouldn't? Maybe that will help me understand where improvements could be made.
Author
Owner

@benweidig commented on GitHub (Jun 9, 2018):

My app is a JSON viewer: You select nodes from a list/tree, and the TextView is used to render the current node pretty-printed and with syntax-hightlighting/-colors.

On Draw(tcell.Screen) the call t.redinxBuffer(int) ensures we got an up-to-date index for the current width. Building the index is only done if necessary, because it's a costly operation due to regexp/string manipulation. My problem is that the whole text will be indexed, and not just the visible range. If my content is huge it will block the event loop, so I can't even terminate it with SIGTERM.

My idea: Partial indexing.

Instead of indexing all the lines we just index the current visible lines (maybe more) and remember the last state (colors etc.), so indexing could be continued later if scrolled.

I'm not sure how well this will work with regions etc., that's a feature I haven't used/read the code so far.
Because of the complexity of the problem and me maybe being the only one using it in this way I wanted to implement it myself and only needed the 3 funcs. But if it works it might be a nice addition to the TextView.

It won't be an easy feature to add, but I'll try to add it the current TextView to test its viability and share my findings.

<!-- gh-comment-id:395965074 --> @benweidig commented on GitHub (Jun 9, 2018): My app is a JSON viewer: You select nodes from a list/tree, and the TextView is used to render the current node pretty-printed and with syntax-hightlighting/-colors. On `Draw(tcell.Screen) ` the call `t.redinxBuffer(int)` ensures we got an up-to-date index for the current width. Building the index is only done if necessary, because it's a costly operation due to regexp/string manipulation. My problem is that the whole text will be indexed, and not just the visible range. If my content is _huge_ it will block the event loop, so I can't even terminate it with SIGTERM. My idea: Partial indexing. Instead of indexing all the lines we just index the current visible lines (maybe more) and remember the last state (colors etc.), so indexing could be continued later if scrolled. I'm not sure how well this will work with regions etc., that's a feature I haven't used/read the code so far. Because of the complexity of the problem and me maybe being the only one using it in this way I wanted to implement it myself and only needed the 3 funcs. But if it works it might be a nice addition to the TextView. It won't be an easy feature to add, but I'll try to add it the current TextView to test its viability and share my findings.
Author
Owner

@rivo commented on GitHub (Jun 9, 2018):

Well, currently, reindexBuffer() only does something if it really needs to. This is the case when the TextView's width changes (because line breaks may appear in different places) and when some other attributes change (e.g. toggling the use of region tags).

It also happens when you write to the TextView but I guess that part could be optimized by indexing only the newly added text. Also, the width change criterion could be applied only if lines are wrapped (I think I'm going to add this now, it's an easy fix).

But if line wrapping is turned on and the width actually changes, I don't see any way to apply "partial indexing". You have to know where the top currently visible line starts and that requires the analysis of the preceding text.

But maybe you have an idea that I haven't thought of yet. Let me know in that case.

<!-- gh-comment-id:395996897 --> @rivo commented on GitHub (Jun 9, 2018): Well, currently, `reindexBuffer()` only does something if it really needs to. This is the case when the `TextView`'s width changes (because line breaks may appear in different places) and when some other attributes change (e.g. toggling the use of region tags). It also happens when you write to the `TextView` but I guess that part could be optimized by indexing only the newly added text. Also, the `width` change criterion could be applied only if lines are wrapped (I think I'm going to add this now, it's an easy fix). But if line wrapping is turned on and the width actually changes, I don't see any way to apply "partial indexing". You have to know where the top currently visible line starts and that requires the analysis of the preceding text. But maybe you have an idea that I haven't thought of yet. Let me know in that case.
Author
Owner

@benweidig commented on GitHub (Jun 10, 2018):

You're right, it might not be possible to implement my idea on the TextView, that's why I thought about building my own with reduced and fixed features that will just satisfy my needs.

I didn't want to take too much of your time for my special edge-case, it's even an edge-case in my app, not only in general.

Indexing only newly written text instead of everything sounds like a good way to reduce the work needed for all text lengths. Right now I use SetText() but I could change it to use Write(). Additionally I would need a callback on scrolling events to write new text if necessary.

Thx for all the explanation, you helped me a lot thinking about my problem. I'll give it a try.

<!-- gh-comment-id:396028175 --> @benweidig commented on GitHub (Jun 10, 2018): You're right, it might not be possible to implement my idea on the `TextView`, that's why I thought about building my own with reduced and fixed features that will just satisfy my needs. I didn't want to take too much of your time for my _special_ edge-case, it's even an edge-case in my app, not only in general. Indexing only newly written text instead of everything sounds like a good way to reduce the work needed for all text lengths. Right now I use `SetText()` but I could change it to use `Write()`. Additionally I would need a callback on scrolling events to write new text if necessary. Thx for all the explanation, you helped me a lot thinking about my problem. I'll give it a try.
Author
Owner

@rivo commented on GitHub (Jun 10, 2018):

I don't know how difficult it is at the moment to reindex only new text. But I can look into it.

Regarding scroll events, you can always intercept key events by using SetInputCapture().

<!-- gh-comment-id:396033747 --> @rivo commented on GitHub (Jun 10, 2018): I don't know how difficult it is at the moment to reindex only new text. But I can look into it. Regarding scroll events, you can always intercept key events by using [`SetInputCapture()`](https://godoc.org/github.com/rivo/tview#Box.SetInputCapture).
Author
Owner

@benweidig commented on GitHub (Jun 10, 2018):

Thx, but like I said, it's just an edge-case for me, and I'm sure I can optimize my side of the code first for a little more performance.

Hijacking scrolling via SetInputCapture() is a nice idea, I'll give it a try.

<!-- gh-comment-id:396034748 --> @benweidig commented on GitHub (Jun 10, 2018): Thx, but like I said, it's just an edge-case for me, and I'm sure I can optimize my side of the code first for a little more performance. Hijacking scrolling via `SetInputCapture()` is a nice idea, I'll give it a try.
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#102
No description provided.