search

zellij-org / zellij

A terminal workspace with batteries included
https://zellij.dev
MIT License
21.84k stars 662 forks source link

Zellij replaces random chars with whitespace on the prompt #943

Closed jovandeginste closed 2 years ago

jovandeginste commented 2 years ago

Don't have much time to find a complete reproduction, apologies...

imsnif commented 2 years ago

Is this a regression? Does it happen with 0.22.1?

When you have the time, if you can follow the instructions for reporting a visual issue and attach the logs it'll be really helpful in debugging this.

jovandeginste commented 2 years ago

I'll do my best; I did not see this in 0.20.1, but I did not have that unicode character then. It's hard to "instantly" reproduce, it starts happening after working for a while. I'll see later (tomorrow?) if I can find a sure-fire set of steps, and refine on that.

jovandeginste commented 2 years ago

ok, what I did:

jovandeginste commented 2 years ago

If you need more, I can add debug tracing and/or screen recording tomorrow!

jovandeginste commented 2 years ago

Was this reproducible, or would you appreciate debug logging?

imsnif commented 2 years ago

Hey, sorry - I didn't get to this yet. Might need a few days. Debug logging would be very helpful if you can provide it!

jovandeginste commented 2 years ago

The debug logs; I redacted a small part from zellij-9.log, I hope this is still usable?

zellij.log zellij-9.log

jovandeginste commented 2 years ago

Screenshot: image

jovandeginste commented 2 years ago

I just tested with a "clean" prompt, and the issue is not there. I'll iterate over my current prompt (adding more elements) and see if I can pinpoint it

imsnif commented 2 years ago

Awesome, thanks! Could you also add the size of the terminal to the logs? You can get this with tput cols and tput lines. I'll need that to debug with them.

jovandeginste commented 2 years ago

cols: 117 lines: 57

jovandeginste commented 2 years ago

It's the unicode character

jovandeginste commented 2 years ago

Try with simply this:

export PS1="🏠"

Then do the steps in my first post.

jovandeginste commented 2 years ago

Don't even need the prompt:

🏠 xdef abc

Paste the house first on the prompt, then space, then abc, then ctrl+left, def+ space, then ctrl+left x -> d disappears. You need the space, but not all spaces...

jovandeginste commented 2 years ago

And for completeness' sake, you can replace ctrl+left with just hitting the left key until you are in the right spot.

JCallicoat commented 2 years ago

Don't even need the prompt:

🏠 xdef abc

Paste the house first on the prompt, then space, then abc, then ctrl+left, def+ space, then ctrl+left x -> d disappears. You need the space, but not all spaces...

What shell / terminal emulator are you using? I can't reproduce with zsh on xfce4-terminal.

jovandeginste commented 2 years ago

Bash inside tilix

jovandeginste commented 2 years ago

I just reproduced it with these steps:

jovandeginste commented 2 years ago

The more times I copy those UTF-chars, the further away the character is that gets wiped...

Cursor on the e: image

jovandeginste commented 2 years ago

Did anyone manage to reproduce this?

imsnif commented 2 years ago

I did not manage to get to this yet. These sorts of issues are high priority for me, but this was one of those weeks where everything was high priority :) I hope to get to this soon.

jovandeginste commented 2 years ago

I removed the clock as a workaround, so this is not breaking my work. Of course, this should be fixed (if anyone else can reproduce it). Otherwise I'm very curious what's special about me :-)

imsnif commented 2 years ago

99% it has to do with a combination of the wide-character and some cursor jump-to ANSI instruction (to hazard a guess). While we have wide-character handling in our interpreter, some edge-cases slip through sometime.

jovandeginste commented 2 years ago

Does just using arrow keys count as "jump-to"? Of maybe I have no idea what jump-to ANSI instructions are...

imsnif commented 2 years ago

Hey, so I found the issue. Thanks for the excellent reproduction steps, they really helped!

We track wide characters as one character with a width attribute of more than 1 (this is a trade-off because it makes the line wrapping much easier to handle - at least in my head). Because of this, when we do all sorts of operations on them we have to account for their width.

In most cases we handle this, but in this case we didn't. The case was: inserting a character in a line that contains one or more wide characters before it.

The reason you were seeing it when overriding the def part and not the abc part has to do with how bash handles these things (I guess! I do not know the bash internals, this is just the behaviour I saw when reproducing this).

When overriding a part of the line without a space after it, it prints the entire rest of the line to the terminal (this was when you were overriding the abc part with nothing after it, so when you were on the a and typed d, bash sent dabc to the terminal and advanced the cursor one forward, when you typed e, bash sent eabc and advanced the cursor one forward, etc.)

When you were typing the def part, it instead just inserted the character, and there we encountered our bug.

I was not able to reproduce this with fish, only with bash - so I guess that's why this was happening to you and not to @JCallicoat (I guess you're using a different shell?)

Anyway - I issued a fix for this which will be in main once the CI is happy. I want to hold off a little bit to release a version for this (since this isn't a regression) until we have some more stuff in main? I'm guessing it won't take very long. I hope you can be patient? (or run from main directly if you're brave :) ).

jovandeginste commented 2 years ago

@JCallicoat was using zsh according to his post @imsnif could you provide me with a musl-binary, so I can test it on my side (and report on any side effects if I would find any)?

imsnif commented 2 years ago

zellij-musl.tar.gz

Sure, here it is.

jovandeginste commented 2 years ago

zellij-musl.tar.gz

Sure, here it is.

Thx, testing it now (first check was ok: no more wipe-out)

jovandeginste commented 2 years ago

There is a new? different? bug now: when using backspace after adding the x character in the above steps, the wrong character is deleted.

jovandeginste commented 2 years ago

So, just to be clear, the bug is purely visual: the command is what it should be, but visually it's wrong. The exact steps are the same as before, but hit backspace after typing the x.

imsnif commented 2 years ago

Yes, another bug! Here's a musl with a fix, want to give it a try?

zellij-musl.tar.gz

jovandeginste commented 2 years ago

So far so good! Just wondering if it would be possible to attach zellij with a new version, to an older running version. What are the issues if that would be possible?

imsnif commented 2 years ago

Awesome! And sorry for the bit-by-bit approach here. This is one of the darker parts of the code and I look forward to refactoring it. I might try to go over stuff and fixing these wide char edge cases more thoroughly when I have the chance.

As for attaching to an existing session - the client part mostly handles STDIN/STDOUT, so even if this were possible it wouldn't help much with this bug (as the state, even the visual state, is kept in the server part - which is running the older version).

imsnif commented 2 years ago

And as a side note - it will probably work as long as we didn't change the protocol between the two parts, which we don't do very often. I plan on versioning it to make it clear when this is a no-go, but it's one of those TODO things.

jovandeginste commented 2 years ago

When I try this (attach to a newer version, as with these small bug fixes), I seem to get a new instance instead; ps aux also shows two "servers" running.

Don't worry about the bit-by-bit approach, this is perfectly reasonable for a 0.x version. It's also understandable that you can't test all the edge cases: you need a crowd of weird people for that...!

imsnif commented 2 years ago

When I try this (attach to a newer version, as with these small bug fixes), I seem to get a new instance instead; ps aux also shows two "servers" running.

Aha, yes! That's because the servers are themselves versioned. I forgot about that bit. There's one server process for each session, and their IPC session file is in a directory that includes the version. So, apparently the functionality is already there in a way.

jovandeginste commented 2 years ago

So far I had no whitespace issues, so this is fixed for me...

jovandeginste commented 2 years ago

I seem to have a variant of this issue:

it eats the next character (a in this example): 🕙 bc def

jovandeginste commented 2 years ago

Just updated to v0.26.0, can no longer reproduce. Probably the fix was not yet in 0.25, which I upgraded to earlier...

imsnif commented 2 years ago

Yes, I fixed this one a few days ago :)

jovandeginste commented 2 years ago

This can be closed, then?

Fabrice-Bernes commented 12 months ago

Although PS1 seems to work fine with wide characters, you can get this exact same problem by using 🏠 or whatever in your RIGHT prompt. Running export RPS1="🏠" on zsh should be enough to reproduce.

This is on zellij 0.39.1

Should this issue be reopened?

imsnif commented 12 months ago

Let's not conflate things. If you can reproduce this behavior, please open a new issue. But please be sure to follow the instructions for reporting a graphical issue and attach the relevant logs.