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

New get-text extent: last_non_empty_output #4973

Closed
wants to merge 6 commits into from

Conversation

karlb
Copy link

@karlb karlb commented Apr 15, 2022

This allows running kitty @ get-text --extent last_complete_output to
get the previous command's output in the same window.

See #4962 .

I'll try this out over the next days but wanted to push it as a draft right away to get CI feedback and maybe even some early feedback on my code. A hint on how to best fix the test would be very welcome!

This allows running `kitty @ get-text --extent last_complete_output` to
get the previous command's output in the same window.

See kovidgoyal#4962 .
@kovidgoyal
Copy link
Owner

@page-down this modifies find_command_output() which is your code, you have any comments?

@kovidgoyal
Copy link
Owner

The failing test is because bash on macOS is ancient and does not work. You should run it with zsh or bash based on bash_ok() and shutil.which('zsh')

@karlb karlb force-pushed the last-complete-command branch 2 times, most recently from af7f556 to bb81b52 Compare April 15, 2022 15:26
@page-down
Copy link
Contributor

You should test it in kitty_test/screen.py, I personally don't recommend doing it in shell_integration.py.

Adding a new option should not break last_cmd_output.
When I am running a command with continuous output, I expect to see the part that has been output so far in the pager when I perform the show last command output action.

I haven't checked the other details yet.

@karlb
Copy link
Author

karlb commented Apr 15, 2022

When I am running a command with continuous output, I expect to see the part that has been output so far in the pager when I perform the show last command output action.

I fully agree and don't intend to break it. AFAICS, the test is the problem, not my change. I didn't manage to get a passing test for last_cmd_output even without the addition of last_complete_output, that's why I assume I am not faking the incomplete command output correctly.

@page-down
Copy link
Contributor

page-down commented Apr 15, 2022

... I didn't manage to get a passing test for last_cmd_output even without the addition ...

You can use test.py to execute the tests in screen.py alone. I believe they all pass the test.

I suggest the option name last_complete_cmd_output, and CommandOutput.last_complete.

I think a fully more accurate complete command output (EDIT: It's just the text that ended up on the screen, not the actual command output.) would require parsing the OSC 133 D marking, which is not currently available. If there is no prompt in the screen, then last_complete_cmd_output will not work.

Also when the beginning of the command output is outside the history buffer, is it considered complete command output? If you treat it as complete, then it does not match what the option name actually means.

EDIT:
Sorry, what does complete mean in this context? Does it mean the output of the last completely executed command, or the entire output of the command?

I see you added a -2 for find_cmd_output(..., int direction, ...), when there are so many argument options, I think enum or something like that is needed.

Thanks for the PR, it's obviously useful, but it does need some polishing.

@kovidgoyal
Copy link
Owner

May be better to look for latest non-empty command output. That should work
in all situations and not be misleading.

@karlb
Copy link
Author

karlb commented Apr 18, 2022

Thanks for the feedback. I'll work on it.

You should test it in kitty_test/screen.py, I personally don't recommend doing it in shell_integration.py.

I assume I should skip the PTY/shell and write to correct bytes to the screen directly in that case, right? What's the best way to do that? screen.draw seems to be for visible characters only and screen.callbacks.write only puts the bytes in wtcbuf, so they won't show um in the screen. And I need a screen object to call cmd_output on it.

@page-down
Copy link
Contributor

... What's the best way to do that? ...

Tests related to the command output are in kitty_test/screen.py test_prompt_marking.
You can see the existing cmd_output call test cases.

@karlb
Copy link
Author

karlb commented Apr 18, 2022

Sorry, what does complete mean in this context? Does it mean the output of the last completely executed command, or the entire output of the command?

I meant the former. Maybe last_completed_cmd_output would be clearer.

May be better to look for latest non-empty command output. That should work in all situations and not be misleading.

Not strictly in all cases (echo foo && kitty @ get-text --extent last-non-empty), but probably in all that matter and there are cases where it will be even more useful than last completed command output. And I also think that it is easier to explain and understand. I'll probably give it a try.

@page-down
Copy link
Contributor

I meant the former. Maybe last_completed_cmd_output would be clearer.

Sorry, if a command is fully executed and there is no output, then by your definition, this should get empty output.
Otherwise it would be confusing, at least for me.
(You can't say that a command that has no output is not fully executed.)

May be better to look for latest non-empty command output. ...

... I also think that it is easier to explain and understand. I'll probably give it a try.

This one does seem to be the most appropriate for now.
It can cover most of the interactive usage scenarios.

@karlb karlb changed the title Last complete command New get-text extent: last_non_empty_output Apr 20, 2022
@karlb karlb marked this pull request as ready for review April 24, 2022 13:23
@karlb
Copy link
Author

karlb commented May 4, 2022

The current state seems to work fine for my usecase. Is the enum refactoring as desired? If there are any other changes required to get this merged, let me know.

@kovidgoyal
Copy link
Owner

If you are going to use an enum then change find_cmd_output() to accept an enum parameter instead of int. @page-down find_cmd_putput() is your code so please review the changes to it. If it looks OK to you I will go ahead.

@karlb
Copy link
Author

karlb commented May 4, 2022

If you are going to use an enum then change find_cmd_output() to accept an enum parameter instead of int.

Yes, of course. I haven't written much C for many years and that show, I'm afraid. Should be better now.

@page-down
Copy link
Contributor

page-down commented May 5, 2022

find_cmd_output is used to find the command output around the specified line. For the current needs, I suggest just providing the appropriate arguments to this function, and not putting every named rule into it.

For trying to get the last command output that is not empty, I expect to at least check if the last command output (or the current cursor line) is empty.

Simply check (marked as output start + empty + the previous line is prompt) and skip the cursor line, start from the previous line. Of course there is also the possibility of outputting some and then moving the cursor, we can't cover all cases, just most of the common ones.

All previous empty output, as the output start marker will be overwritten by the prompt marker.

@kovidgoyal
Copy link
Owner

Here is a patch to sort out various other minor issues:

diff --git a/kitty/fast_data_types.pyi b/kitty/fast_data_types.pyi
index 871ce4c3..e53f0506 100644
--- a/kitty/fast_data_types.pyi
+++ b/kitty/fast_data_types.pyi
@@ -20,6 +20,10 @@ MOUSE_SELECTION_WORD: int
 MOUSE_SELECTION_RECTANGLE: int
 MOUSE_SELECTION_LINE_FROM_POINT: int
 MOUSE_SELECTION_MOVE_END: int
+LAST_RUN_CMD: int
+FIRST_ON_SCREEN: int
+LAST_VISITED_CMD: int
+LAST_NON_EMPTY_CMD: int
 KITTY_VCS_REV: str
 NO_CLOSE_REQUESTED: int
 IMPERATIVE_CLOSE_REQUESTED: int
diff --git a/kitty/screen.c b/kitty/screen.c
index 92102eb0..aec98569 100644
--- a/kitty/screen.c
+++ b/kitty/screen.c
@@ -7,6 +7,10 @@
 
 #define EXTRA_INIT { \
     PyModule_AddIntMacro(module, SCROLL_LINE); PyModule_AddIntMacro(module, SCROLL_PAGE); PyModule_AddIntMacro(module, SCROLL_FULL); \
+    PyModule_AddIntMacro(module, LAST_RUN_CMD); \
+    PyModule_AddIntMacro(module, FIRST_ON_SCREEN); \
+    PyModule_AddIntMacro(module, LAST_VISITED_CMD); \
+    PyModule_AddIntMacro(module, LAST_NON_EMPTY_CMD); \
     if (PyModule_AddFunctions(module, module_methods) != 0) return false; \
 }
 
@@ -2762,7 +2766,7 @@ typedef enum CommandOutputs
 {
     LAST_RUN_CMD = 0,
     FIRST_ON_SCREEN = 1,
-    LAST_VISISTED_CMD = 2,
+    LAST_VISITED_CMD = 2,
     LAST_NON_EMPTY_CMD = 3,
 } CommandOutput;
 
@@ -2777,7 +2781,7 @@ find_cmd_output(Screen *self, OutputOffset *oo, index_type start_screen_y, unsig
     Line *line = NULL;
 
     // find around
-    if (direction == LAST_VISISTED_CMD) {
+    if (direction == LAST_VISITED_CMD) {
         line = checked_range_line(self, y1);
         if (line && line->attrs.prompt_kind == PROMPT_START) {
             found_prompt = true;
@@ -2798,7 +2802,7 @@ find_cmd_output(Screen *self, OutputOffset *oo, index_type start_screen_y, unsig
         while (y1 >= upward_limit) {
             line = checked_range_line(self, y1);
             if (line && line->attrs.prompt_kind == PROMPT_START && !line->attrs.continued) {
-                if (direction == LAST_VISISTED_CMD) {
+                if (direction == LAST_VISITED_CMD) {
                     // find around: stop at prompt start
                     start = y1 + 1;
                     break;
@@ -2826,7 +2830,7 @@ find_cmd_output(Screen *self, OutputOffset *oo, index_type start_screen_y, unsig
     }
 
     // find downwards
-    if (direction == LAST_VISISTED_CMD || direction == FIRST_ON_SCREEN) {
+    if (direction == LAST_VISITED_CMD || direction == FIRST_ON_SCREEN) {
         while (y2 <= downward_limit) {
             if (on_screen_only && !found_output && y2 > screen_limit) break;
             line = checked_range_line(self, y2);
@@ -2873,9 +2877,9 @@ cmd_output(Screen *self, PyObject *args) {
         case FIRST_ON_SCREEN: // first on screen
             found = find_cmd_output(self, &oo, 0, self->scrolled_by, FIRST_ON_SCREEN, true);
             break;
-        case LAST_VISISTED_CMD: // last visited cmd
+        case LAST_VISITED_CMD: // last visited cmd
             if (self->last_visited_prompt.scrolled_by <= self->historybuf->count && self->last_visited_prompt.is_set) {
-                found = find_cmd_output(self, &oo, self->last_visited_prompt.y, self->last_visited_prompt.scrolled_by, LAST_VISISTED_CMD, false);
+                found = find_cmd_output(self, &oo, self->last_visited_prompt.y, self->last_visited_prompt.scrolled_by, LAST_VISITED_CMD, false);
             } break;
         case LAST_NON_EMPTY_CMD: // last non-empty cmd output
             // When scrolled, the starting point of the search for the last command output
diff --git a/kitty/window.py b/kitty/window.py
index 397bd00c..aefef38a 100644
--- a/kitty/window.py
+++ b/kitty/window.py
@@ -25,8 +25,9 @@
 from .fast_data_types import (
     BGIMAGE_PROGRAM, BLIT_PROGRAM, CELL_BG_PROGRAM, CELL_FG_PROGRAM,
     CELL_PROGRAM, CELL_SPECIAL_PROGRAM, CURSOR_BEAM, CURSOR_BLOCK,
-    CURSOR_UNDERLINE, DCS, DECORATION, DECORATION_MASK, DIM, GLFW_MOD_CONTROL,
-    GRAPHICS_ALPHA_MASK_PROGRAM, GRAPHICS_PREMULT_PROGRAM, GRAPHICS_PROGRAM,
+    CURSOR_UNDERLINE, DCS, DECORATION, DECORATION_MASK, DIM, FIRST_ON_SCREEN,
+    GLFW_MOD_CONTROL, GRAPHICS_ALPHA_MASK_PROGRAM, GRAPHICS_PREMULT_PROGRAM,
+    GRAPHICS_PROGRAM, LAST_NON_EMPTY_CMD, LAST_RUN_CMD, LAST_VISITED_CMD,
     MARK, MARK_MASK, NO_CURSOR_SHAPE, NUM_UNDERLINE_STYLES, OSC, REVERSE,
     SCROLL_FULL, SCROLL_LINE, SCROLL_PAGE, STRIKETHROUGH, TINT_PROGRAM, Color,
     KeyEvent, Screen, add_timer, add_window, cell_size_for_window,
@@ -163,9 +164,8 @@ class DynamicColor(IntEnum):
     default_fg, default_bg, cursor_color, highlight_fg, highlight_bg = range(1, 6)
 
 
-# Matches kitty/screen.c/CommandOutput
 class CommandOutput(IntEnum):
-    last_run, first_on_screen, last_visited, last_non_empty_run = 0, 1, 2, 3
+    last_run, first_on_screen, last_visited, last_non_empty_run = LAST_RUN_CMD, FIRST_ON_SCREEN, LAST_VISITED_CMD, LAST_NON_EMPTY_CMD
 
 
 DYNAMIC_COLOR_CODES = {

@kovidgoyal
Copy link
Owner

Note that I implemented this without bothering to use find_cmd_output or an enum, seems a lot clearer to me.

@karlb
Copy link
Author

karlb commented May 28, 2022

Cool, thanks!

@page-down
Copy link
Contributor

Thank you for all your efforts. That's a very straightforward and clear implementation. I can't wait to try it out.

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