Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virtual Table view #question #248

Closed
pihentagy opened this issue Mar 11, 2019 · 31 comments
Closed

virtual Table view #question #248

pihentagy opened this issue Mar 11, 2019 · 31 comments

Comments

@pihentagy
Copy link

Does the library support virtual table?

I'd like to build a simple log viewer with this lib, but the log is quite huge, so I don't think I'd like to load the whole logfile to memory.

@rivo
Copy link
Owner

rivo commented Mar 16, 2019

At this point, no. I guess one could experiment with using a GetCell(row,column) *TableCell function that would allow you to return the cell content dynamically. But it would have to be random access. Right now, the user can jump to the beginning, to the end, page up/down, etc.. There are fixed rows/columns (at the beginning of the table). And we would need to know the total number of rows at any time. I'm not sure if you can provide that for your log. (If it's a simple file, it's probably difficult to implement without parsing it first and keeping it in memory.)

If you do have random access to your log, I could look into this. But it could take a while and I don't know yet if I might hit any major roadblocks. (I haven't looked at the code in detail from this perspective.)

@pihentagy
Copy link
Author

I'm thinking of parsing the logfile in the background and "index" it properly.

So I would take care of the GetCell implementation details, and telling about the number of rows/columns.

Don't get how the fixed columns/rows complicate the implementation.

But to recap the big picture: could you separate the Table model and the table view? Then I'd like to implement my own model, and pass it to your view.

@pihentagy
Copy link
Author

(If it's a simple file, it's probably difficult to implement without parsing it first and keeping it in memory.)

I was thinking about parsing the logfile to an sqlite database in the background. Either way, I would just need a model interface to implement and a way to notify the viewer about the data changes.

@rivo
Copy link
Owner

rivo commented May 13, 2019

Apologies for the late reply. I was away for a few weeks. Is this issue still relevant to you?

@pihentagy
Copy link
Author

Relevant, but I can postpone this development (as this is a half-hobby project). I haven't dealt with the problem in the past weeks, but not having a model-view separation can be a dealbreaker for me.

@pckhoi
Copy link

pckhoi commented Mar 20, 2021

What is your current appetite to see this implemented? I'm needing this and might try to implement this myself but what are the criteria you are looking for?

@pihentagy
Copy link
Author

To be frank I lost interest in this feature.

But the original need was to be able to lazily access a potentially huge number of items and have it rendered. So there should be a callback provided to retrieve the element specified in a parameter. It implies that the columns of the table should not depend on the actual length of the (huge) table.

@pckhoi
Copy link

pckhoi commented Mar 25, 2021

I went ahead and produced a VirtualTable that is to my liking but I'm not sure if @rivo would be interested in seeing my pull request because there are some breaking changes:

  • Decouple row/column selection from Table. In my opinion Table should be purely about visual and styling, and also my selection needs are different.
  • Add some public functions:
    • func (c *TableCell) SetPosition(x, y, width int) *TableCell
  • Make some function public:
    • printWithStyle should become PrintWithStyle
    • decomposeString => DecomposeString
    • iterateString => IterateString
    • stringWidth => StringWidth

This would imply perhaps remake the original Table primitive on top of VirtualTable primitive and all should be well.

@rivo
Copy link
Owner

rivo commented Apr 26, 2021

I'm not really sure I understand your suggestion. Maybe you can elaborate how this would work.

I looked at the code regarding implementing a virtual table and it doesn't look like there's a simple way. Tables are not read-only. There are all kinds of modifying functions like SetCell(), RemoveRow(), RemoveColumn(), InsertRow(), and InsertColumn(). If we wanted to back the table by some virtual structure, it would have to implement all of these functions.

Then, every time the table is redrawn, it would let the implementer know what rows can be displayed so it can adjust its mapping. While drawing, it would query the implementer for all cells in the visible row range. (If SetEvaluateAllRows() was called with true, even invisible cells of the table would be queried.)

Something like this:

type TableContent interface {
	GetCell(row, column int) *TableCell
	GetRowCount()
	GetColumnCount()

	SetCell(row, column int, cell *TableCell)
	RemoveRow(row int)
	RemoveColumn(column int)
	InsertRow(row int)
	InsertColumn(column int)
	Clear()

	UpdateView(fromRow, toRow int)
}

Looking forward to your thoughts.

@pihentagy
Copy link
Author

Yes that's it, separate the handling of table model (I'd use TableModel as a type) and the table view.

@pckhoi
Copy link

pckhoi commented Apr 27, 2021

Your proposal assumes that the TableContent interface want to accept TableCell as input. My table has to deal with 10s of million of rows and it doesn't even keep all the raw bytes of the data let alone the table cells. Instead it take in something that is close to io.ReadSeeker and io.ReaderAt - my point is VirtualTable shouldn't deal with how data is updated. Instead you can have a Table widget that wrap VirtualTable and work with the interface that you proposed.

My implementation of VirtualTable is complicated enough so I'll only discuss how it is different from the current Table:

  • It allows displaying sub-columns, so for any given pair of row/col index, a list of table cells can be displayed instead of just one and all the widths are added/divided in a pleasing way.
  • It has a SetGetCellsFunc(getCell func(row, column int) []*TableCell) method which allow you to feed the list of cells to display at a row/col index. It doesn't keep any TableCell of course.
  • It doesn't handle selection nor keep any selection state. My selection need is different so I have another widget that handle selection and control offsets/styling as needed.
  • Doesn't have SetEvaluateAllRows.

Some of my needs such as sub-columns maybe not relevant to most people but from this VirtualTable it's possible to build the exact same Table widget.

@rivo
Copy link
Owner

rivo commented May 29, 2021

I understand your use case and if we only looked at a subset of the current feature set of Table, it would certainly be possible to implement it this way. However, I cannot simply ignore everything else that is in there and the requirements of other people who are using this package. So it's important to at least have a plan what to do with the functions I mentioned.

Sure, you can choose not to implement functions like RemoveRow(row int), that's up to you. But for those users who need these functions, I need to know what to do internally to make it work. SetGetCellsFunc(getCell func(row, column int) []*TableCell) is not enough, which is why I proposed the TableContent interface.

Along with a function Table.SetTableContent(content TableContent), you can then define your own table (view). If this is not provided (probably in the majority of cases), the default implementation of TableContent is used, which is what the table is doing now, i.e. no virtualization.

Your proposal assumes that the TableContent interface want to accept TableCell as input.

Unless I misunderstood what you wrote, I don't think this is the case. In your case, you would implement GetCell(), GetRowCount(), and GetColumnCount(). Because it's read-only, you would ignore the other functions. Only GetCell() would return a TableCell for the requested cell. (It would be wise for you to cache these because the function is potentially called very often.)

Does this make sense to you?

This is quite a bit of work so I want to make sure that we're on the same page.

@pckhoi
Copy link

pckhoi commented Jun 1, 2021

I think you misunderstood some of my suggestions. Yes, my VirtualTable only access cells via SetGetCellsFunc however it doesn't mean that I can't remove row. It just means that I won't be doing that by interacting with VirtualTable directly. This is about separation of concerns. Like I said, VirtualTable should just worry about rendering cells, not how those cells contents can be changed. I mean look at Facebook's React, the store is separated from the component, if you want to update the component then you update the store, not the component directly. It absolutely doesn't mean that VirtualTable is read-only. As I said, VirtualTable does not keep any table cell but discard them all once it is done with them, but that doesn't mean that whatever is supplying those cells cannot cache them.

Again, SetGetCellsFunc's main difference compared to TableContent's methods such as GetCell and SetCell isn't so much that it is "read-only". It is about pulling updates versus pushing updates. When I want to make changes to the table, I make changes to the data directly, then I just let the VirtualTable know that it should now re-render. At which point it call the get cells func to get the cells that are currently in view. And it will result in cleaner code because all view-related logics are tucked into a few functions that deal with how to translate the object's state into the table cells whenever VirtualTable ask for them. Outside of those functions if I want to change the data I can change them directly, not having to sprinkle GetCell and SetCell all over my code.

My ideal version of TableContent interface only have get functions, and I hope you understand by now that doesn't mean it is read-only:

type TableContent interface {
	GetCells(row int, column int) []*TableCell
	GetRowCount() int
	GetColumnCount() int
}

Also I'm reiterating that I want to split the current Table widget into VirtualTable widget and a new Table widget which wrap VirtualTable and maintain current behavior. The point is to create a VirtualTable widget that only know how to display a table but otherwise stripped bare of any logic that are not strictly view-related. I was unsatisfied with some of the current Table's behaviors such as selection so this would provide an avenue to make my own table widget. Perhaps this is out of scope for this issue. If it is let me know I will open another issue.

@rivo
Copy link
Owner

rivo commented Jun 24, 2021

Let me ask you this way: Assuming I have a Table backed by your VirtualTable, as you described. In your opinion, what exactly is supposed to happen when I call this function?

@pckhoi
Copy link

pckhoi commented Jun 25, 2021

I assume you want the interface for Table remain unchanged. Which means the table need to keep all of its cells in a two-dimension matrix along with row and column count. Which is something that the current Table is already doing. The new function will be exactly the same as your current Table.RemoveColumn function. The difference is, this new Table type will not have any drawing logic whatsoever, it merely know how many rows there are, how many columns there are, what cell is at a certain position. The VirtualTable will take care of drawing, Table can tell it how many row and column there are via SetShape or even SetShapeGetter, doesn't matter. The Table can also provide it with a function that it can call whenever it needs a cell at a certain position via SetGetCellsFunc.

And then when the Table.Draw is called it can simply call VirtualTable.Draw because VirtualTable is embedded. VirtualTable calls its internal .getCells function (which was provided via SetGetCellsFunc) to get the cells that it need to draw. It will be very straight forward.

@marcelocantos
Copy link

Why not just implement VirtualTable as a new widget type distinct from Table? It could perhaps work by subclassing Table to leverage some of its code, but that would be an implementation detail. Table itself would remain as is.

@pckhoi
Copy link

pckhoi commented Jun 25, 2021

Because that means code duplication. Any drawing logic that Table possess VirtualTable will surely have it as well. By making Table relying on VirtualTable for drawing, any change to drawing logic only need to be made at one place.

@pckhoi
Copy link

pckhoi commented Jun 25, 2021

But if you are simply asking whether VirtualTable is a separate widget from Table then yes, it is a separate, lower-level widget.

@pckhoi
Copy link

pckhoi commented Jun 25, 2021

As for what type should be embedded in what type, I thought the answer should be VirtualTable as the embedded widget because it is lower-level.

@marcelocantos
Copy link

marcelocantos commented Jun 25, 2021

It would make sense to embed VirtualTable if Table wasn't already coded without it. As it stands, going the other way is cleaner, since it doesn't require changing an existing implementation that wasn't designed with virtualisation of its contents in mind.

I don't know the code base very well, but I suspect you could reuse most of the Table code by embedding it in VirtualTable.

@pckhoi
Copy link

pckhoi commented Jun 25, 2021

I was never aware that putting in the effort is one of the issues in discussion here. But if @rivo decides that he likes this direction then rest assure I'll open a PR with my own VirtualTable and Table implementation. I have already been using my own VirtualTable rewrite for a while now.

@rivo
Copy link
Owner

rivo commented Jun 25, 2021

I assume you want the interface for Table remain unchanged.

Definitely yes. This is one of the core principles of this package that whenever possible, backwards compatibility is guaranteed.

@pckhoi I've gone over your comments many times. Maybe I'm looking at the problem from a completely different angle but I have trouble understanding the details of your suggestion, especially how to handle write operations when the Table class itself doesn't have direct access to the data anymore (just via the read-only functions you suggested).

I have a slight hunch that you want the Table class to map visible cell positions (the ones on screen) to original cell positions (the ones in your own data structure). If that's the case, then for very large tables — and this is what the original request was about — Table would again create a very large matrix of position mappings. That wouldn't be desirable, in my opinion. Maybe you mean something else?

But since you already have an implementation, how about you post a link to it? No need to submit a PR, a link to your code will do. I'll have a look. Maybe that will help me understand where you're coming from.

@pckhoi
Copy link

pckhoi commented Jun 25, 2021

My table would not need to maintain a large matrix of TableCell, not even all the bytes are needed in memory. But it does maintain a cache of visible cells which get updated or removed depending on what positions are requested. In any case, look in this package there's a VirtualTable and other kinds of tables which all depend on VirtualTable for actual drawing.

@pckhoi
Copy link

pckhoi commented Jun 25, 2021

If you're asking about whether a new Table implementation with identical interface that embed VirtualTable, would be better at handling large matrix of cells then no. Because the old interface dictates that Table must keep all of its cells in memory, visible or not. The point of VirtualTable then is to enable other kinds of table that can actually handle infinite amount of data to be built on top of it.

@rivo
Copy link
Owner

rivo commented Jun 25, 2021

Thanks for providing a link to your implementation. It helped me understand what you've been talking about all along. (Maybe it's just me but from your descriptions alone, it was not clear that this is what you meant — too many questions unanswered.)

To be honest, though, I find this distinction between your VirtualTable and the Table not clear. Sure enough, VirtualTable implements a virtual table but only the read-only parts. You say that if someone also wants write access, they should just modify the underlying data structure. Well, this means they then have to expose their data structure in a separate way somehow because there are no write methods in VirtualTable. Or, alternatively, they could just use Table which implements them using a specific implementation of VirtualTable. But then they're stuck with Table which would be pretty much the way it is now and offers none of virtualization we want.

I also don't think this distinction between VirtualTable and Table would be clear to many people. And ultimately, it's not consistent with how extensibility is implemented in the tview package. Almost every tview widget provides specific functionality out of the box, and if you want to modify/extend that functionality, you "plug it in" with functions like SetInputCapture() or SetAutocompleteFunc(). There is no separate class VirtualInputField for extra InputField functionality.

So all in all, I think what I outlined before is what I'll be going for after all. You will still be able to handle changes to the data on the implementation side, i.e. your "separation of concerns". For example, if you want to display a log that is continuously being written to, you're free to let the table access only the trailing rows of your log. You don't need to call or even implement RemoveRow() to make that happen. But for those users who want to provide a Table with all of its functionality but with a different data structure, they can do it using the interface described above.

I understand that this is a very different direction from the one you've already taken and you may not be interested in this version. There are other people in this thread, though, that will hopefully benefit from this. (Also, I would say that things like sub-cells, the lack of the "evaluate all rows" flag, and a different selection functionality are out of scope for now anyway.)

Everyone, feel free to weigh in. I'm never quite sure if I missed a certain perspective that would make this a bad solution. But as of now, it looks like this will be the implementation.

@pckhoi
Copy link

pckhoi commented Jun 26, 2021 via email

@pckhoi
Copy link

pckhoi commented Jun 26, 2021

And regarding modifying data away from the table. Again, this is a pattern borrowed from React: separation of state management and view logic. But anyway if you feel like it's just coming from a React fanboy then feel free to ignore me.

@rivo
Copy link
Owner

rivo commented Jun 26, 2021

Sure, React has its merits, and it's currently undoubtedly popular. Whether or not its model is the best of its kind is debatable. I've been programming with UI toolkits for 25 years, including React, and honestly, I wouldn't say React is the best I've ever used. In fact, when I started using it, I found it quite difficult to get it right due to its variety of concepts (e.g. mounting, updating, unmounting pre/post phases, states vs. props, and many others). I wanted tview to be very easy to get started with.

What I didn't anticipate was that people would request a lot more complex functionality from tview than what it already does. So when people needed to customize their widgets, I added ways to do it here and there, and it ended up being the plug-in kind of architecture. We're here now. And in packages like this one, I value consistency more than going with the latest trend. (Which, incidentally, is why I love Go.)

As a side note, I don't see how my proposal mixes state management and view logic. Whatever your data structure is, a Table won't know about it and if you make changes to your data structure, Table will reflect that. The question whether there have to be two different classes (VirtualTable and Table) rather than one (Table) is a different one, in my opinion.

99% of all users are never expected to use the Box component, yet it is still exposed.

Oh, I would love to hide Box from users (and quite a few other classes/interfaces, by the way). But there are no abstract classes in Go. And I already mentioned elsewhere that I don't think replicating all methods in all classes is a good idea. (Also, I initially made the mistake of making too much public, and now I can't go back.)

@marcelocantos
Copy link

TableView is fundamentally a read-only object. Changes are expressed by modifying the source data and telling TableView that something changed so it can refresh the screen. Caching and localising the blast radius of a change (avoid reporting changes to a cell when it's not view) are key design questions.

This is why I disagree with the idea of introducing virtualisation to Table. It's a poor fit.

My original thought was that, if Table isn't easy to use as the backend for VirtualTable, then perhaps some additions to the API could make it so. On reflection, however, I suspect that that's not practical. The design considerations are quite different. For instance, a VirtualTable could in theory have a billion rows, each with its own size (Is size a per-row variable in Table?). You really don't want to create a billion placeholder rows, though. Moreover, you're much more likely to want to filter on a billion-row table than scroll through it (or at the very least filter before scrolling).

I think VirtualTable would be a fine addition to the library, but I strongly suspect that it'll require its own implementation distinct from Table. That said, once implemented, Table could be recast quite easily as a thin shim over VirtualTable.

@rivo
Copy link
Owner

rivo commented Aug 7, 2021

So this is implemented now. But for now, it's in a separate branch "virtualtable" (see PR #634). I also added an example with demos/table/virtualtable/main.go.

As discussed previously, Table's write functions are still there for compatibility reasons. But if you want to ignore them and instead modify the table on your end, i.e. making Table a read-only object, you're free to do so. The TableContentReadOnly struct will help you reduce your boilerplate in that case.

Give it a go and let me know how it works for you. Once it's stable, I'll merge the PR into the master branch.

@rivo
Copy link
Owner

rivo commented Sep 20, 2021

I've merged this branch into master since there were no complaints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants