-
Notifications
You must be signed in to change notification settings - Fork 46
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
Implemented View component for easy scrolling #82
Conversation
…ling - Implemented View as an easy way to use the existing Screen and Window APIs for rendering Cells that don't fit within a Window. Basically, a user renders the oversized Cell content to a View, then renders a part of that View to a Window. - Created the `view.zig` example.
- Added proper bounds checking to fit the View within the provided Window. - Added a simple x/y return Type to the `toWin()` function so users can more easily maintain scrolling bounds.
- Set up swapable Views for a Small and Large map. - Created a "Controls" Window at the top of the TUI. - Added better documentation for how Views work.
- Added the `header_align` and `col_align` to `TableContext` along with the corresponding `HorizontalAlignment` and `ColumnAlignment` Types. - Updated the `table.zig` example.
- Added the `header_border` and `col_border` to `TableContext`. Note, row borders don't work as well due to borders taking up an entire cell. - Updated the `table.zig` example.
src/View.zig
Outdated
}, | ||
unicode, | ||
); | ||
const window = try alloc.create(Window); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Window
shouldn't need to be allocated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. Windows are largely static bounds for rendering, but don't actually change. I'll fix that.
src/View.zig
Outdated
|
||
pub const Config = struct { | ||
max_width: usize = 10_000, | ||
max_height: usize = 10_000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the defaults for the width and height. Change the name to just width
and height
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
src/View.zig
Outdated
max_width: usize = 10_000, | ||
max_height: usize = 10_000, | ||
x_pixel: usize = 0, | ||
y_pixel: usize = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pixels shouldn't matter in this view? They only come into play later in rendering. Let's take them out of the Config struct and set them to 0 in init
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to ensure I understand, would users still be able to write images to a View if this is missing? The current implementation obviously doesn't handle this, but I was aiming to add it in the future so Images could be rendered in this style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, they would. The image is pretty much a flag on it's upper-left cell, and when that gets drawn to the real screen all of the data will be there to properly draw it.
Now...making a scrollable image would be much harder because you'd need to calculate it's extents and then use clip regions in the kitty protocol to get it to work. So you would need the pixel density to know how many cells the image takes up for this case.
src/View.zig
Outdated
y_pixel: usize = 0, | ||
}; | ||
pub fn init(alloc: mem.Allocator, unicode: *const Unicode, config: Config) !View { | ||
const screen = try alloc.create(Screen); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the Screen
need to be allocated? I think we could get away storing the value rather than a pointer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this depends entirely on how large the underlying Cell buffer gets, which is up to users since they'll no longer be bound by screen size.
A potential solution is making the Allocator an Optional, putting it in the Config, and setting a sane limit for stack allocation before throwing a comptime error that forces the use of the Allocator.
src/View.zig
Outdated
}; | ||
/// Render a portion of this View to the provided Window (`win`). | ||
/// This will return the bounded X (col), Y (row) coordinates based on the rendering. | ||
pub fn toWin(self: *View, win: *const Window, config: RenderConfig) !struct { usize, usize } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument win
should be type vaxis.Window
. No need to use a const pointer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this signature should be something like:
pub fn draw(self: *const View, win: vaxis.Window, x_off: usize, y_off: usize) !void
The width and height are defined by the window we pass in, we only need to define the origin of where we want to draw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer allowing users to render a smaller part of the View to a Window if they'd like. There's nothing stopping them from doing what you're saying (which is the default), but this offers simple flexibility.
src/View.zig
Outdated
|
||
/// Fills the View with the provided cell | ||
pub fn fill(self: View, cell: Cell) void { | ||
self.fill(cell); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo? This would infinitely recurse. Probably meant self.win.fill
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% a typo. Will fix!
src/View.zig
Outdated
return .{ x, y }; | ||
} | ||
|
||
/// Writes a cell to the location in the View |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to take all of these win
API wrappers and remove them. You can add a window
method which returns a root window of the backing screen and the full window
API is accessible from that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had two main reasons for this:
-
API simplicity. To me it makes more sense to remove an additional call from users if they're always going to have to make that call to use a View. Otherwise, users will always have to do
view.win().someFn()
instead of justview.someFn()
. This has the downside of maintaining API parity between Views and Windows, but that leads to the second point. -
The Window and View APIs are not 1-for-1. While the View API largely mirrors that of Window, there are a few functions that I don't think View should include. Namely
hasMouse()
andhideCursor()
(and possiblyscroll()
?) shouldn't be used from View. Those actions should be done from whatever Window the user pushes the View to. That list could also expand in the future if Window sees more changes.
src/View.zig
Outdated
/// Underlying Screen | ||
screen: *Screen, | ||
/// Underlying Window | ||
win: *Window, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to store a window at all here. They should be created on-the-fly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes back to API simplicity. View will always need a Window for its function calls, but it also only uses a subset of the Window API. This allows View to control its API without having to reinvent the wheel with the great functions available from Window.
The other option is to store the View's width
and height
directly to the View and create the Window during each function call. This would further remove the need for Screen allocation, but it would open up Views to having their size changed on the fly (which might not be desirable).
src/View.zig
Outdated
const Cell = @import("Cell.zig"); | ||
|
||
/// View Allocator | ||
alloc: mem.Allocator, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the allocator either. This isn't used except for deinit
, so let's just force callers to pass in their allocator for deinit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one other place it's used is to store the Screen as a whole. Which allows Views to be a little more locked down after initialization.
I'm also a bigger fan of Managed structs in this instance as they prevent users from accidentally passing the wrong Allocator back in for deallocation later. But that's largely subjective, and not a hill I'm willing to die on, haha
Added some initial comments. I think the main question I have is if this should be backed by With Or am I missing a use case here where it's beneficial to render the large area each frame and then only display a small portion? To me the big advantage of this widget is to prevent the render-every-frame...Or maybe the expectation is just that the user of a |
Hey, appreciate the initial comments. I'll reply to those individually. For the bigger picture, there is a case where View would still need to be rendered/updated more than just the initial time. For instance, if we're doing scrollable text (like a log) or if I rewrite the Table widget to use a backing View. In these cases, the data for the View is not static as it is in the example. That said, I think I can adjust the underlying Screen to simply calculate its required Cells based on the initial Config. |
Maybe I misunderstood - this |
My initial thought was that it wouldn't be dynamic in size, but its contents would be dynamic. A simple example is scrollable log text, where a library user can make a View of, say, 1000 rows and continually write their log entries to that View. Then they simply adjust where they're looking at in that View to let end users scroll through. |
- Removed Window allocation in `init()`. - Changed `max_width` and `max_height` to `width` and `height` in `Config`. - Removed `x_pixel` and `y_pixel` from `Config`. - Changed the `win` parameter of `toWin()` to Type `Window`. - Fixed a typo in `fill()` so that it's no longer a recursion trap. - Updated the example w/ corresponding fixes.
Updated the PR w/ the fixes outlined here: 6955482 Some of my Table Updates got caught up in the PR. I was trying to finish a few more items there, but the commits that are pushed are fully functional. |
- Removed Window allocation in `init()`. - Changed `max_width` and `max_height` to `width` and `height` in `Config`. - Removed `x_pixel` and `y_pixel` from `Config`. - Changed the `win` parameter of `toWin()` to Type `Window`. - Fixed a typo in `fill()` so that it's no longer a recursion trap. - Updated the example w/ corresponding fixes.
- Removed Window allocation in `init()`. - Changed `max_width` and `max_height` to `width` and `height` in `Config`. - Removed `x_pixel` and `y_pixel` from `Config`. - Changed the `win` parameter of `toWin()` to Type `Window`. - Fixed a typo in `fill()` so that it's no longer a recursion trap. - Updated the example w/ corresponding fixes.
Awesome work, thanks! |
The View component largely mirrors the Window API, but writes to a custom sized Screen buffer. This allows users to write to the View as they would a Window, then use
toWin()
to render a portion of the View to an actual Window.There's also a new
view.zig
example.