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

Add scrollback_fill_enlarged_window option #3371

Merged
merged 6 commits into from Mar 17, 2021

Conversation

elebow
Copy link
Contributor

@elebow elebow commented Mar 7, 2021

This is the beginning of a change to optionally fill the buffer with lines from the history when enlarging a window.

References #1206, #1972, #2398, #2531

This PR is unfinished, and it may take me some time to do so because I am unfamiliar with the codebase and out-of-practice with C. Any advice on the remaining work would be appreciated.

@kovidgoyal
Copy link
Owner

There are two files you will need to understand, screen.c and history.c. These maintain respectively the screen and history buffers. Currently there are functions to push a line from the top of the screen buffer to the bottom of the history buffer. You need to implement the reverse, namely pull a line from the bottom of the history buffer to the top of the screen buffer. It should not be difficult once you understand how the buffers work.

Then you need to spec how you want this to actually work. Should the number of blank lines at the bottom of the screen remain the same? Or should resizing the window mean that the cursor is moved to the last line? If only "new space" is filled does that mean this is triggerred only when the new window is taller than the old, not wider? If the window grows by n lines does that mean only at most N lines are filled?

Then you have to write tests to check that your code behaves as per the spec.

Feel free to ask if you are having difficulty anywhere. And when you have a spec, do please discuss it before spending time writing code.

@elebow
Copy link
Contributor Author

elebow commented Mar 8, 2021

Thank you for the advice.

Here is my thinking for the spec.

Increasing height

Lines move into the history by being "pushed up" beyond the top edge, so they should come out of history by being "pulled down" from the top edge. This supports the thought model that the screen buffer and history buffer are a single buffer, with only the lowest portion being visible.

The bottom edge of the screen is the origin, so the position of the cursor relative to the bottom edge should not change when increasing a window's height. The number of blank lines at the bottom should not be affected.

Increasing width

If rewrapping causes blank rows to become available, the space should be filled by "pulling down" from the window's top edge.

Case behaviors

  • Height increased, width unchanged → pull down lines to fill new space at the top
  • Height increased, width increased → rewrap, pull down
  • Height increased, width decreased → rewrap, pull down if possible
  • Height unchanged, width increased → rewrap, pull down if possible
  • Height decreased, width increased → rewrap, pull down if possible

@kovidgoyal
Copy link
Owner

Sounds ok to me, you will also have to deal with pulling down graphics from the graphics manager. Since those can scroll up as well.

@elebow elebow force-pushed the scrollback-fill-enlarged-window branch 3 times, most recently from ed8660c to 1f51ac6 Compare March 11, 2021 07:34
@elebow
Copy link
Contributor Author

elebow commented Mar 11, 2021

@kovidgoyal

I have pushed a partial implementation.

Other than the graphics manager which I have not looked at yet, I am having trouble figuring out how to add lines to the top of the screen's linebuffer in linebuf_add_line_to_top().

@kovidgoyal
Copy link
Owner

See the INDEX_UP macro, you need to do a similar thing, except instead of calling linebuf_index you call linebug_reverse_index

@elebow
Copy link
Contributor Author

elebow commented Mar 12, 2021

I see that HistoryBuf uses a ring buffer, and we can insert content by writing at location (self->start_of_data + self->count) % self->ynum.

But LineBuf seems to use some other data structure that I don't understand. What do line_map and scratch mean? Is there a straightforward way to write new content to a specific line number in the LineBuf?

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 12, 2021 via email

@elebow elebow force-pushed the scrollback-fill-enlarged-window branch 2 times, most recently from 2e535e1 to 21829c8 Compare March 15, 2021 03:40
@elebow
Copy link
Contributor Author

elebow commented Mar 15, 2021

I believe this PR is ready for review.

The tests for MacOS are failing, but I'm not sure why. None of the affected code seems like it would be platform-specific. I don't have convenient access to a real MacOS system to test further.

@elebow elebow changed the title [WIP] Add scrollback_fill_enlarged_window option Add scrollback_fill_enlarged_window option Mar 15, 2021
@elebow
Copy link
Contributor Author

elebow commented Mar 15, 2021

Also, I put forth an alternative name for the option, scrollback_window_anchor_bottom. Since the linebuffer and historybuffer appear to the user as a single buffer, it might be confusing to speak of "filling".

Copy link
Owner

@kovidgoyal kovidgoyal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe the option should be named fill_expanded_window_from_scrollback it makes it pretty clear what it does.

@@ -91,10 +91,15 @@ def set_options(self, options=None):
options = Options(merge_configs(defaults._asdict(), final_options))
set_options(options)

def create_screen(self, cols=5, lines=5, scrollback=5, cell_width=10, cell_height=20, options=None):
def create_screen(self, cols=5, lines=5, scrollback=5, cell_width=10, cell_height=20, options=None, content=[]):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use tuple not list for default as lists are mutable

@@ -303,6 +303,48 @@ def draw(text, end_line=True):
s.resize(s.lines - 1, s.columns)
self.ae(x_before, s.cursor.x)

def test_scrollback_fill_after_resize(self):
def prepare_screen(content=[]):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use tuple not list for default

@@ -252,6 +252,17 @@ historybuf_add_line(HistoryBuf *self, const Line *line, ANSIBuf *as_ansi_buf) {
*attrptr(self, idx) = (line->continued & CONTINUED_MASK) | (line->has_dirty_text ? TEXT_DIRTY_MASK : 0);
}

Line
historybuf_pop_line(HistoryBuf *self) {
index_type idx = (self->start_of_data + self->count-1) % self->ynum;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not handle count == 0

kitty/line-buf.c Outdated
@@ -326,6 +326,14 @@ is_continued(LineBuf *self, PyObject *val) {
Py_RETURN_FALSE;
}

unsigned int linebuf_continued_lines_count(LineBuf *self) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use index_type rather than unsigned int

kitty/screen.c Outdated
y++;
INDEX_GRAPHICS(1);

Line last_history_line = historybuf_pop_line(self->historybuf);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has unneccessary copies of the line object. Instead rewrite pop_line to take a pointer to the line object and return a bool indicating failure when count is 0. And there is a builtin line object in the LinueBuf class you can use, self->line



if (OPT(scrollback_fill_enlarged_window)) {
int lines_to_fill = (lines - self->main_linebuf->ynum) \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesnt handle the case of there being more free space because of the window becoming wider.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that not handled by the next line?

                          + (linebuf_continued_lines_count(self->main_linebuf) - linebuf_continued_lines_count(n));

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on what you mean by work. That counts continued lines both above and below the cursor, so if thre are continued lines below it will change cursor position w.r.t the bottom of the screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would something like this address that issue?

diff --git a/kitty/line-buf.c b/kitty/line-buf.c
index 7ac76e9b..b0372436 100644
--- a/kitty/line-buf.c
+++ b/kitty/line-buf.c
@@ -327,9 +327,9 @@ is_continued(LineBuf *self, PyObject *val) {
 }
 
 unsigned int
-linebuf_continued_lines_count(LineBuf *self) {
+linebuf_continued_lines_count(LineBuf *self, index_type stop_at_line) {
     unsigned int count = 0;
-    for (unsigned int i = 0; i < self->ynum; i++)
+    for (unsigned int i = 0; i < self->ynum && i < stop_at_line; i++)
       if (self->line_attrs[i] & CONTINUED_MASK) count++;
 
     return count;
diff --git a/kitty/screen.c b/kitty/screen.c
index bb2a0b13..e94ce538 100644
--- a/kitty/screen.c
+++ b/kitty/screen.c
@@ -249,7 +249,7 @@ screen_resize(Screen *self, unsigned int lines, unsigned int columns) {
 
     if (OPT(scrollback_fill_enlarged_window)) {
       int lines_to_fill = (lines - self->main_linebuf->ynum) \
-                          + (linebuf_continued_lines_count(self->main_linebuf) - linebuf_continued_lines_count(n));
+                          + (linebuf_continued_lines_count(self->main_linebuf, y) - linebuf_continued_lines_count(n, y));
 
       while (lines_to_fill > 0) {
         if (self->historybuf->count <= 0) break;

@elebow elebow force-pushed the scrollback-fill-enlarged-window branch from 21829c8 to 98c1513 Compare March 16, 2021 04:08
@elebow
Copy link
Contributor Author

elebow commented Mar 16, 2021

Pushed changes.

@kovidgoyal
Copy link
Owner

Here is a fix for add_line_to_top()

diff --git a/kitty/line-buf.c b/kitty/line-buf.c
index 5afba116..7783c8f0 100644
--- a/kitty/line-buf.c
+++ b/kitty/line-buf.c
@@ -407,10 +407,9 @@ delete_lines(LineBuf *self, PyObject *args) {
 
 void
 linebuf_add_line_to_top(LineBuf *self, Line *line) {
-    Line dest;
-    init_line(self, &dest, self->line_map[0]);
-    copy_line(line, &dest);
-    linebuf_mark_line_dirty(self, 0);
+    init_line(self, self->line, self->line_map[0]);
+    copy_line(line, self->line);
+    self->line_attrs[0] = TEXT_DIRTY_MASK | (line->continued ? CONTINUED_MASK : 0);
 }

There is no need to use a separate Line object on stack, and also the continued flag was not being copied.

@elebow elebow force-pushed the scrollback-fill-enlarged-window branch from 98c1513 to 0e10f6c Compare March 17, 2021 05:19
@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 17, 2021 via email

@elebow elebow force-pushed the scrollback-fill-enlarged-window branch from 0e10f6c to 9fff829 Compare March 17, 2021 06:28
@elebow
Copy link
Contributor Author

elebow commented Mar 17, 2021

Pushed changes.

@kovidgoyal kovidgoyal merged commit d61c4a9 into kovidgoyal:master Mar 17, 2021
@trygveaa
Copy link
Sponsor Contributor

Thanks for implementing this! I experience an issue though. When I increase the height of the window, scrollback is pulled from above, but instead of the previously visible scrollback being pushed down, it's overwritten instead, so the last output from the command is gone. See the attached video.

kitty-scrollback-resize-2021-03-17_15.53.11.mp4

@kovidgoyal
Copy link
Owner

I havent looked at it in detail but see if my recent changes to do the filling after cursor position is finalized work.

@trygveaa
Copy link
Sponsor Contributor

The same thing still happens with the current master.

Maybe it's easier to see what's happening if I don't return to the shell. The cursor remains at the original row from the top, it isn't moved down with the lines, see video:

kitty-scrollback-resize-2021-03-17_18.17.59.mp4

Also, I see that the cursor does move a bit sideways for some reason.

@elebow
Copy link
Contributor Author

elebow commented Mar 18, 2021

I see that same behavior with current master (4a996c1).

git-bisect says the bad behavior was introduced with 0719861.

@elebow
Copy link
Contributor Author

elebow commented Mar 18, 2021

This seems to solve it.

diff --git a/kitty/screen.c b/kitty/screen.c
index 473e34b7..094cdbcf 100644
--- a/kitty/screen.c
+++ b/kitty/screen.c
@@ -330,6 +330,7 @@ screen_resize(Screen *self, unsigned int lines, unsigned int columns) {
             if (!historybuf_pop_line(self->historybuf, self->alt_linebuf->line)) break;
             INDEX_DOWN;
             linebuf_copy_line_to(self->main_linebuf, self->alt_linebuf->line, 0);
+            self->cursor->y++;
         }
     }
     return true;

@kovidgoyal
Copy link
Owner

Should be fixed by my latest commit

@trygveaa
Copy link
Sponsor Contributor

Thanks, now it works great!

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

Successfully merging this pull request may close these issues.

None yet

3 participants