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

Feature request: option to not scroll cursor with iTerm2 images #1228

Closed
sxyazi opened this issue Aug 20, 2023 · 50 comments
Closed

Feature request: option to not scroll cursor with iTerm2 images #1228

sxyazi opened this issue Aug 20, 2023 · 50 comments

Comments

@sxyazi
Copy link

sxyazi commented Aug 20, 2023

Hi, I'm working on a terminal file manager, limited by ConPTY, in Windows it can only use the inline images protocol, the current behavior moves the cursor while the image is rendering, which can cause the TUI to scroll and get broken.

It would be great if a new parameter doNotMoveCursor=1 could be added to disable cursor movement, I've tested it in
WezTerm -- the only one I know of that implements this protocol besides iTerm2/Mintty, and it works fine, here's the proposal: wez/wezterm#1424

@mintty
Copy link
Owner

mintty commented Aug 21, 2023

Where "do not move cursor" means "do not scroll" or actually keep cursor where the image started so the next image or text will overlap it?
And wouldn't it be more convenient to have this as a switchable mode rather than a parameter for each image?

@sxyazi
Copy link
Author

sxyazi commented Aug 21, 2023

Where "do not move cursor" means "do not scroll" or actually keep cursor where the image started so the next image or text will overlap it?

Yep, some TUI apps, like Yazi, fix the right sidebar as the preview area and take over the clearing of all previewed content -- because it's not just images that are previewed, but also codes, directories, and so on. At this point the extra cursor movement is not only unnecessary, but also conflicts with the TUI itself.

And wouldn't it be more convenient to have this as a switchable mode rather than a parameter for each image?

I think it would be more flexible to be able to control it for each image, and if another image doesn't want to render in a fixed area, the scrolling may need to be restored. Another reason is that WezTerm is implemented in this way, that it would be easier for TUI devs to adapt if the format is kept the same, without having to take into account different standards for different terminals.

@mintty
Copy link
Owner

mintty commented Aug 23, 2023

I have a few concerns about a quick go:

  1. This is about the iTerm2 protocol, so how is the stance of iTerm2 about this meanwhile, after 2 years of the proposal?
  2. I do not see a clear definition of the protocol extension anywhere, is there a link?
  3. The semantics of "do not move cursor" and "do not scroll" is not quite the same. Which is desired, which is implemented, which will be implemented by iTerm2?

@sxyazi
Copy link
Author

sxyazi commented Aug 23, 2023

This is about the iTerm2 protocol, so how is the stance of iTerm2 about this meanwhile, after 2 years of the proposal?

I don't know much about the implementation behind iTerm2, but I tested at macOS that it works fine even without this parameter. But on Windows both WezTerm and mintty require this parameter, maybe it has something to do with the platform difference.

I do not see a clear definition of the protocol extension anywhere, is there a link?

Currently only WezTerm extends this parameter, this is its documentation https://github.com/wez/wezterm/blob/main/docs/imgcat.md

The semantics of "do not move cursor" and "do not scroll" is not quite the same. Which is desired, which is implemented, which will be implemented by iTerm2?

It's the literal "do not move cursor" for this parameter. When a TUI app moves the cursor at the same time as the terminal, it can lead to unexpected results, scrolling the screen being the most common one.

@mintty
Copy link
Owner

mintty commented Aug 27, 2023

I guess what you mainly need is to prevent scrolling by graphics output. Implementation is easy and I guess I'll make that a new private mode DECSET 7780.
About the actual cursor position, it should be easy to simply place the cursor whereever you need it after image output, and that's also portable. So I do not see the need for a "do not move cursor" attribute.

@sxyazi
Copy link
Author

sxyazi commented Aug 27, 2023

Sounds good! let me know if you need testers.

@mintty
Copy link
Owner

mintty commented Aug 27, 2023

The new feature is now in the repository.

@sxyazi
Copy link
Author

sxyazi commented Aug 27, 2023

Thanks for your work!

I tested the latest code TUI was corrupted, and could not receive stdin, after some tests finally found that it was caused by fa33000

Here is a reproduction video:

388.mp4

@mintty
Copy link
Owner

mintty commented Aug 28, 2023

What's wrong in the video?

@sxyazi
Copy link
Author

sxyazi commented Aug 28, 2023

When building with fa33000 (Aug 7, first 25 seconds of the video), the TUI is messed up, tabs float to the top left corner and no key input is recognized, on zoom the whole screen disappears.

Use its previous commit f61f9d5 (Aug 4) everything works fine.

It seems that the debug code messes up the TUI app, I'm not sure how to turn it off.

@mintty
Copy link
Owner

mintty commented Aug 28, 2023

The debug code isn't even active as it's #ifdef'ed out. There must be something else going on.

@sxyazi
Copy link
Author

sxyazi commented Aug 28, 2023

I noticed that write is placed inside the debug_pty block, is this expected behavior?

mintty/src/child.c

Lines 894 to 897 in fe4430d

#ifdef debug_pty
int n =
write(pty_fd, s, len);
#endif

@mintty
Copy link
Owner

mintty commented Aug 28, 2023

Oh! Quite embarassing. That happens if you work on multiple issues in one file. Fixed.

@sxyazi
Copy link
Author

sxyazi commented Aug 28, 2023

Thanks, all is well with TUI now.

I tested the new DECSET 7780 mode and it works fine, now my screen doesn't get scrolled.

But I also found another issue when adapting Mintty, there seems no way to get the iTerm2 image anchored at the current cursor position like Sixel, is there anything I missed?

@mintty
Copy link
Owner

mintty commented Aug 28, 2023

It works but it does not work? I don't understand.
Images are anchored at the current position except in "Sixel Display Mode" (DECSET 80).

@sxyazi
Copy link
Author

sxyazi commented Aug 28, 2023

For the clarification:

  • Due to the latest fix, the TUI is not broken now, it works
  • The new DECSET 7780 mode works correctly, and now images out of range don't cause the screen to scroll
  • iTerm2 image position doesn't work, the image is not anchored to the cursor position, but to the bottom right corner of the screen.

Here is the ANSI escape code to send:

\x1b7        save position
\x1b[2;99H   move 99,2
\x1b[?25h    show cursor
\x1b[?7780h  
\x1b]1337;File=inline=1;size=29413;width=400px;height=267px:/9j/4AAQ......Mmf/2Q==\x07
\x1b[?25l    hide cursor
\x1b8        restore position

Also have a video to explain it (note the bottom right corner):

133.mp4

@mintty
Copy link
Owner

mintty commented Aug 28, 2023

If I output an image with your escape sequences, it appears in the rightmost column of line 2 (near the top), properly corresponding to the 2;99 coordinates.

@sxyazi
Copy link
Author

sxyazi commented Aug 29, 2023

1111.mp4

I made a minimal replication and tested it in WezTerm and Mintty, WezTerm works fine (first 15 seconds of the video), but Mintty doesn't correctly respect the cursor position.

https://github.com/sxyazi/mintty-reproduction

@mintty
Copy link
Owner

mintty commented Aug 29, 2023

I have no rust. Please run > log to produce a text file that can be catted to demonstrate the issue. Also the mintty logging feature could be used.

@mintty
Copy link
Owner

mintty commented Aug 29, 2023

The test case img.txt confirms that positioning works properly.

@mintty
Copy link
Owner

mintty commented Aug 29, 2023

You used Save Cursor and Restore Cursor sequences to "keep" the cursor position. That could be a problem in the context of your application if it happens to be using them already, as these sequences cannot be nested.

@sxyazi
Copy link
Author

sxyazi commented Aug 29, 2023

Nope, the reproduction code above only has one Save Cursor and Restore Cursor, which doesn't work properly.

This log.txt is what I logged using mintty --log and do a cargo run, it seems that \x1b7 (save position), and \x1b[3;10H (move 10,3) were swallowed by mintty, and the order of the escape sequences changed.

I'm not sure if this is related to ConPTY, but the same code works without issues in WezTerm.

@mintty
Copy link
Owner

mintty commented Aug 29, 2023

Yes, there is no Save/Restore Cursor sequence in the log. Please report that to the cygwin mailing list.

@sxyazi
Copy link
Author

sxyazi commented Aug 30, 2023

I'm not familiar with Cygwin and Windows, how do I describe the issue?

Is the Save/Restore Cursor not visible to Mintty because Mintty is using Cygwin as a dependency library but Cygwin isn't providing Mintty with the correct escape code results?

@mintty
Copy link
Owner

mintty commented Aug 30, 2023

Your previous suspicion, ConPTY, is more likely. Anyway, I see in the log that the save/restore sequences do not arrive at mintty, so mintty behaviour is correct. So the issue should be taken to the cygwin mailing list.

@mintty
Copy link
Owner

mintty commented Aug 30, 2023

Can you replace the Save/Restore with absolute cursor placement in that application? It should be knowing where the next output is supposed to go. Or is "do not move cursor" behaviour needed in addition to "do not scroll"? And if so, does it make sense to combine them both in mode 7780?
Please consider some design options and also check how to get it portable, possibly also with iTerm2.

@sxyazi
Copy link
Author

sxyazi commented Aug 30, 2023

Can you replace the Save/Restore with absolute cursor placement in that application? It should be knowing where the next output is supposed to go.

I don't understand this. Do you mean that after displaying the image, the cursor position is no longer restored after the move and the TUI app uses the absolute position for the next time it draws? If so, that's hard to do, since the TUI library I'm using, ratatui, doesn't have a corresponding API to do this.

Are there any TUI projects that use Mintty with iTerm2 to display images? I'd like to use it as a reference to see how they are implemented.

Or is "do not move cursor" behaviour needed in addition to "do not scroll"? And if so, does it make sense to combine them both in mode 7780?

I need to do more tests to see if they make sense, at the moment Mintty doesn't display images properly, I found that cargo run > log.txt; cat log.txt can work, but cargo run directly doesn't, I need to dig into it deeper.

@mintty
Copy link
Owner

mintty commented Aug 30, 2023

Please be careful about how you describe the issue, as others reading this might get confused. Mintty does display images properly as I demonstrated. Your application with your library does not display properly in mintty. The reason is ESC 7/8 being filtered out, perhaps by ConPTY. So we can look for an alternative. If you display an image, using that library, don't you know your own current cursor position? If you do, you can simply set the same position to refresh it after image display.
That's my preferred proposal. If it does not work, I can offer another mode as said before.

@sxyazi
Copy link
Author

sxyazi commented Aug 31, 2023

Please be careful about how you describe the issue, as others reading this might get confused. Mintty does display images properly as I demonstrated. Your application with your library does not display properly in mintty.

Sorry, maybe I worded it wrong, I have to clarify:

The reproduction I provided above implements the iTerm2 image protocol correctly, but I can't get the correct result in Mintty, I don't think it can be called "Mintty does display images properly".

The reason is ESC 7/8 being filtered out, perhaps by ConPTY.

Yes, it may be a ConPTY issue, but WezTerm, which is also based on ConPTY and works fine, I'm not sure why.

If you do, you can simply set the same position to refresh it after image display

I didn't understand this, could you please give an escape code example like #1228 (comment), it would be helpful for both of us to explain the issue more clearly.

@BrianInglis
Copy link

I think what @mintty may mean is that the Cygwin console interface dealing with running native Windows programs via ConHost pseudo-console ConPTY adds layers which may insert or interpret escape sequences in their own way, as MS does not support a passthru interface, whereas Cygwin programs run under a Unix-like master/slave pseudo-tty interface which mintty is designed to handle in the same way as a Unix-like terminal program, but with a native Windows GUI, like its ancestor putty.
For comparison, I can just as easily and have run Cygwin programs under Cygwin lxterminal, u/rxvt, xterm, or terminal emulators based on VTE, under Cygwin X server, or "remotely" from or to a Linux VM or system running X.

@mintty
Copy link
Owner

mintty commented Aug 31, 2023

escape code example

\x1b[2;10H place cursor
\x1b[?7780h
\x1b]1337;File=inline=1;size=29413;width=400px;height=267px:/9j/4AAQ......Mmf/2Q==\x07
\x1b[2;10H place cursor again, to original position

@sxyazi
Copy link
Author

sxyazi commented Aug 31, 2023

It's hard to do that. The UI library I'm using, ratatui, provides a higher level of abstraction for components, and I can't predict its next cursor position, and it's not portable :(

@mintty
Copy link
Owner

mintty commented Aug 31, 2023

You are writing the application, right? So you are preparing an image for output and somehow get it written. Then you should be knowing where to place that image, right? That's how I understood the scenario but maybe it's different.
Who is writing the ESC 7 Save Cursor sequence in the first place, you or the library? If it's you, you could try another Save/Restore cursor sequence instead: ESC [ ? 1048h and ESC [ ? 1048l.

@j4james
Copy link

j4james commented Aug 31, 2023

\x1b[2;10H place cursor
\x1b[?7780h
\x1b]1337;File=inline=1;size=29413;width=400px;height=267px:/9j/4AAQ......Mmf/2Q==\x07
\x1b[2;10H place cursor again, to original position

Just FYI, something like that is almost certainly not going to work over conpty. When conhost sees the OSC 1337 it has no idea what it means or what effect it's going to have - it's just passing that through to mintty on the off chance that it does something meaningful. And when it receives the next CUP operation it's internal position is already at 2;10, so that's essentially a no-op. It's not going to send a position update to mintty because as far as it's concerned there's nothing to update.

In general, if you're using an escape sequence that conhost doesn't understand, there's a good chance it won't work correctly.

@mintty
Copy link
Owner

mintty commented Aug 31, 2023

And when it receives the next CUP operation it's internal position is already at 2;10, so that's essentially a no-op. It's not going to send a position update to mintty

Ah, that might be the problem. ConPTY apparently also transforms Save/Restore Cursor into absolute positioning (CUP). So I assume Restore Cursor is also dropped because of that. Maybe it would help to set the cursor explicitly to a wrong position, then back.

@BrianInglis
Copy link

Windows Rust crate ratatui library is designed to work with Windows Rust crate crossterm and a couple of other similar Windows Rust emulators that work on ConHost and/or pseudo-console ConPTY terminals.
It is using escape sequences assuming the behaviour of supported emulators and terminals, like having specific terminfo definitions baked in, not necessarily compatible with those of other emulators or terminals.
If it does not work as expected under some other emulators - have you tried comparing by running under Cygwin X Window programs lxterminal, u/rxvt, xterm?
Then do not use them - use the supported native Windows emulators - or a different TUI library that supports terminfo definitions for the emulators or terminals used, like mintty, to control features used and limit assumptions made.

@sxyazi
Copy link
Author

sxyazi commented Aug 31, 2023

@BrianInglis Hey, the reproduction I provided above just writes the escape sequences to stdout, without using libraries like ratatui, crossterm, to control the assumptions.

I've only tested in Mintty and WezTerm, which are the only 2 emulators I know of that support displaying images on Windows.

@j4james
Copy link

j4james commented Aug 31, 2023

Maybe it would help to set the cursor explicitly to a wrong position, then back.

@mintty If that works at all, it's likely to be flaky. Depending on the timing, conpty may only trigger a refresh after you've moved the cursor back to the original position. And in that case, it's again likely to think there's nothing to update.

You could try inserting a delay between the two moves, but even then conpty will sometimes optimize the cursor movement with relative operations, e.g. using CUF and CUB rather CUP if you're moving horizontally, so that can still leave you in the wrong position.

What I don't understand is why it would work any better on WezTerm, assuming it's also using conpty. It's possible they're bundling a more recent version, which could make a difference, but I'd still expect it to have issues.

@sxyazi
Copy link
Author

sxyazi commented Aug 31, 2023

It's possible they're bundling a more recent version, which could make a difference

Yes, WezTerm is using the more recent version from May 19, wez/wezterm@43a8b44

but I'd still expect it to have issues

FYI, I use the same code with doNotMoveCursor=1 to test it in WezTerm, and it almost has no issues.

234.mp4

@mintty
Copy link
Owner

mintty commented Aug 31, 2023

OK, so the clarifications suggest that you need more support from mintty. I had asked before, but let's get that clearer: there are a few options to deal with the two features "do not scroll" and "do not move cursor":

  • mode 7780 could imply also "do not move cursor", so enabling both at once
  • image format could adopt the doNotMoveCursor parameter, but by its name it would not imply "do not scroll"
  • image format could also add another parameter "doNotScroll"

Which of these (or other) options is preferable and most likely to be portable?

@sxyazi
Copy link
Author

sxyazi commented Sep 1, 2023

As I said, mode 7780 is already working as expected and the screen really doesn't get scrolled anymore.

Now the issue is that it's not possible to move the cursor to the specified position before the image is rendered, due to the \x1b[3;10H being missing, which may be beyond the scope of this issue, and since there is no an easy workaround so far, I think this issue should be closed.

Whether implementing mode 7780 as "do not move cursor" at the same time would help, depending on whether the missing \x1b[3;10H is related to a conflict with cursor movement during image rendering or not, I'm not sure, I'll only know after it's implemented and I'm testing.

Is it possible to create a git patch for "mode 7780 could imply also "do not move cursor", so enabling both at once", I'll provide further information after testing it out.

@mintty
Copy link
Owner

mintty commented Sep 1, 2023

\x1b[3;10H being missing

I guess you are misinterpreting something about this, maybe because you are writing multiple images in sequence?

Anyway, I've implemented "do not move cursor" for mode 7780 and uploaded it to the repository. Please test.

@sxyazi
Copy link
Author

sxyazi commented Sep 1, 2023

you are writing multiple images in sequence

Yes, I need to write multiple images. When starting the next image, I need to use \x1b[3;10H to reposition the cursor at the start of the image (right sidebar)


I have tested the new code and it behaves the same as the last version. But I finally found a way to get the image preview to work, which was prompted by @j4james, I added a sleep and everything works! I'm kind of incredulous 🤣

I'm not sure if there's a better way to do it, and while it works now, it's still unstable, and occasionally the picture gets washed out at the screen bottom.

1112.mp4

@mintty
Copy link
Owner

mintty commented Sep 1, 2023

I'm puzzled. If you write two subsequent images, and I assume you do not write them to the same position, there would be a positioning sequence (CUP) between them, with different coordinates. What would make ConPTY suppress the proper positioning for the second image?

@sxyazi
Copy link
Author

sxyazi commented Sep 1, 2023

Sorry I don't know more details behind ConPTY, but the issue can be reproduced using the following code:

fn main() -> anyhow::Result<()> {
    let img = fs::read(Path::new("./demo.jpg"))?;
    let mut stdout = std::io::stdout().lock();

    write!(stdout, "\x1b[?25h\x1b7\x1b[2;20H")?;
    stdout.flush()?;
    thread::sleep(Duration::from_millis(100));  // to switch this

    write!(
        stdout,
        "\x1b[?7780h\x1b]1337;File=inline=1;size={};width={}px;height={}px:{}\x07",
        img.len(),
        400,
        240,
        general_purpose::STANDARD.encode(&img)
    )?;

    write!(stdout, "\x1b8\x1b[?25l")?;

    write!(stdout, "\x1b[?25h\x1b7\x1b[20;20H")?;
    stdout.flush()?;
    thread::sleep(Duration::from_millis(100));  // to switch this

    write!(
        stdout,
        "\x1b[?7780h\x1b]1337;File=inline=1;size={};width={}px;height={}px:{}\x07",
        img.len(),
        400,
        240,
        general_purpose::STANDARD.encode(&img)
    )?;

    write!(stdout, "\x1b8\x1b[?25l")?;

    Ok(())
}

The video to demonstrate it:

111.mp4

And should this issue be closed? it seems gotten too far away from its original goal, and the sleep is kind of a workaround, even if it's not perfect.

@mintty
Copy link
Owner

mintty commented Sep 1, 2023

We should check whether the ...20;20 comes through to the terminal.
By the way, why do you switch on the cursor during image output and make it invisible otherwise? Feels like the wrong way.

@sxyazi
Copy link
Author

sxyazi commented Sep 1, 2023

\x1b[?25h is a WezTerm-specific behavior and is only needed in its Windows version, it's not needed in Mintty, and I added it just to test that it wouldn't be a problem in Mintty this way either.

Initially adapting WezTerm had the same issue as Mintty, the image didn't follow the cursor, no matter what the cursor was set to, after two nights of testing I realized that WezTerm has to show the cursor before drawing the image, and then hide it, which is very strange... I filed an issue with them and the answer I got was because of ConPTY, I'm not sure, it's just a huge black box for me, so agonizing...

@mintty
Copy link
Owner

mintty commented Sep 1, 2023

The new behaviour is now also supported case-by-case by image parameter doNotMoveCursor as you requested.
I think it makes no sense to split "do not move cursor" and "do not scroll" into two separate features so they are combined consistently, with either the new parameter or the mode 7780.

@sxyazi
Copy link
Author

sxyazi commented Sep 2, 2023

Thanks, it's now consistent with WezTerm behavior.

@mintty
Copy link
Owner

mintty commented Sep 3, 2023

Release 3.6.5.

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