chore!: change to use v3 of Tcell

[joelim-work]

• Closes Migrating to Tcell v3 #2284
• Fixes [Feature request] Support for zero-width joiner (ZWJ) U+200D (in preview and file name) (bug) #1134

Changes:

• Use the new v3 of Tcell (from v2).
• Support for SGR 8 (concealed text) is removed as Tcell has dropped support for it. This is a breaking change, however it is a very niche feature and I don't think users are interested in it anyway.
• Map <backspace2> keybindings to <backspace>, since Tcell does this internally. This is a breaking change, however it should be a welcome one as it unifies <backspace> and <backspace2>, which reduces confusion.
• ui.keyAcc and ui.keyCount are changed from []rune to string, since key events in Tcell now store a string instead of a rune. This makes the code easier to work with.
• Utility functions are added for converting between Tcell key events and their string representations (e.g. <space>/<c-a>).
• The key event handling code has largely been rewritten to make the logic clearer.
• Replace SetContent calls with PutStrStyled, this requires rewriting the win.print function to accommodate for the new API.

Anyway I will plan to leave this PR open for a while, and only merge it when it (and the new version of Tcell) becomes more stable.

Warning

Apparently keybindings for capital letters (e.g. G for bottom) don't work in v3 of Tcell. at least for me. I have raised an issue about it.
Fixed now, but I still plan to leave this PR open for a while regardless, not in a rush to merge.

[CatsDeservePets]

Uppercase letters get reported incorrectly for me as well.
I haven't encountered any other bug so far.

On a side note: I've noticed you've been using conventional commits more often lately. I try to align my commit messages to match the repos style. If you plan to stick with them going forward, I will use them as well.

[gdamore]

I fixed the upper case keys issue. Sorry about that!

[joelim-work]

On a side note: I've noticed you've been using conventional commits more often lately. I try to align my commit messages to match the repos style. If you plan to stick with them going forward, I will use them as well.

Yes, it's something I have decided to adopt from now on, since many other projects are using it too. I am uncertain about actually enforcing it though (e.g. via CI), or using it as a replacement for the existing breaking/new/fix labels. Just having some visual structure to the commit history is enough for me.

[CatsDeservePets]

On a side note: I've noticed you've been using conventional commits more often lately. I try to align my commit messages to match the repos style. If you plan to stick with them going forward, I will use them as well.

Yes, it's something I have decided to adopt from now on, since many other projects are using it too. I am uncertain about actually enforcing it though (e.g. via CI), or using it as a replacement for the existing breaking/new/fix labels. Just having some visual structure to the commit history is enough for me.

Cool, I am all for unification. I have been using them myself in privat projects (although I am using the Angular guidelines which famously dropped chore).

I guess using it for commit messages is good for now, we can always expand on that later.

[CatsDeservePets]

Without looking or digging into all the individual changes, I noticed the following while testing

• [Feature request] Support for zero-width joiner (ZWJ) U+200D (in preview and file name) (bug) #1134

Previews now seem to correctly display things like "👨‍💻". However, the width calculation is still off. Lets compare what happens when pasting this emoji (including the quotes) into lf's command line:

Before	After

The cursor still assumes the wider length. We might have to update printLength.

[CatsDeservePets]

I also noticed that MacOS Icon\r files now mess up the UI when visible in the preview pane.

What you see here is cursor misalignment causing the info timestamp to be printed at the wrong position (Who could have predicted that having carriage returns in your file names might cause issues?).
Note: These are MacOS specific files that are created when using custom icons for applications or files. This is not a user error of putting weird stuff in file names.

[joelim-work]

Without looking or digging into all the individual changes, I noticed the following while testing

• [Feature request] Support for zero-width joiner (ZWJ) U+200D (in preview and file name) (bug) #1134

Previews now seem to correctly display things like "👨‍💻". However, the width calculation is still off. Lets compare what happens when pasting this emoji (including the quotes) into lf's command line:

Before After

The cursor still assumes the wider length. We might have to update printLength.

So the reason is that in the current code, characters are printed individually using SetContent, but with this change entire strings are printed using PutStrStyled, which respects grapheme clusters. This is means that the []rune array containing U+1F468, U+200D, U+1F4BB is displayed as 👨‍💻 and not as 👨 + ZWJ + 💻.

Calculating the width is fairly simple using uniseg.StringWidth in https://github.com/rivo/uniseg, and it is probably much better than the current runeSliceWidth function, which sums the width of individual runes and cannot handle grapheme clusters.

By (unintentionally) supporting grapheme clusters in the display, this introduces other caveats - now commands like cmd-left and cmd-delete-back will operate on individual code points instead of entire grapheme clusters. I think it is a niche use-case and can be handled separately outside of the PR as it sounds like a non-trivial change. OK I made some fixes in 99a5864 but it is a bit hacky just because handling emojis is more complex than simple characters.

As a side note, I have been wondering whether it is possible to just replace https://github.com/mattn/go-runewidth entirely with https://github.com/rivo/uniseg - the latter is already a dependency of Tcell, and I don't think it makes sense to have two packages to work with Unicode.

I also noticed that MacOS Icon\r files now mess up the UI when visible in the preview pane. What you see here is cursor misalignment causing the info timestamp to be printed at the wrong position (Who could have predicted that having carriage returns in your file names might cause issues?). Note: These are MacOS specific files that are created when using custom icons for applications or files. This is not a user error of putting weird stuff in file names.

I'm not sure what you mean here, are you saying the filename contains a carriage return and lf prints it directly? Maybe it's possible to ignore it in the win.print function, similar to how there is special handling for tabs. If you send me a patch like below (untested) I can add it in.

diff --git a/ui.go b/ui.go
index f843acf..3e29314 100644
--- a/ui.go
+++ b/ui.go
@@ -90,7 +90,7 @@ func (win *win) print(screen tcell.Screen, x, y int, st tcell.Style, s string) t
 			for i := 0; i < w; i++ {
 				buf = append(buf, ' ')
 			}
-		} else {
+		} else if r != '\r' {
 			buf = append(buf, r)
 		}
 

[CatsDeservePets]

As a side note, I have been wondering whether it is possible to just replace https://github.com/mattn/go-runewidth entirely with https://github.com/rivo/uniseg - the latter is already a dependency of Tcell, and I don't think it makes sense to have two packages to work with Unicode.

Seems reasonable.

I'm not sure what you mean here, are you saying the filename contains a carriage return and lf prints it directly? Maybe it's possible to ignore it in the win.print function, similar to how there is special handling for tabs. If you send me a patch like below (untested) I can add it in.

You can find more information about these files here.

Your patch works perfectly fine!

Here is a screen recording for better understanding (in the end I am switching to a version with your patch applied):

out.mp4

[CatsDeservePets]

OK I made some fixes in 99a5864 but it is a bit hacky just because handling emojis is more complex than simple characters

I really don't like the code to be honest, but the outcome is good. I handles said emoji now better than yazi or ranger. joshuto seems to handle it without any problems as well.

[joelim-work]

I added the patch for ignoring carriage returns.

OK I made some fixes in 99a5864 but it is a bit hacky just because handling emojis is more complex than simple characters

I really don't like the code to be honest, but the outcome is good. I handles said emoji now better than yazi or ranger. joshuto seems to handle it without any problems as well.

I don't love the code either, but the problem is that a visible character on the screen can consist of multiple runes, so associating it with a single element of []rune is incorrect and leads to buggy behavior. Even in the current code, using cmd-delete-back on emojis like 👨‍💻 doesn't work properly because it deletes a single rune instead of a single grapheme cluster. I ended up extracting the logic into a helper function and added unit tests however.

I think this happens to solve #1134 so I'll add it to the PR description.

EDIT: BTW I also ended up replacing the runeSliceWidth functions the uniseg library, which means go-runewidth can be removed entirely - this is in a different branch which I plan to submit separately.

[CatsDeservePets]

I could not find any obvious regression, I will continue using this version before giving my ok on this.

[CatsDeservePets]

I wonder whether we should provide a list of all the available special keys in the docs.

[joelim-work]

There is already existing documentation for keybindings. I'm just not sure it's worth maintaining a list of every single possible key when the user can discover them interactively:

The easiest way to find out the name of a key combination and whether it will work on your system is to press the key while lf is running and read the name from the unknown mapping error.

If there's anything you would like to clarify in the documentation though, you're more than welcome to submit it in a separate PR.

[CatsDeservePets]

I think the biggest problem here is that lf doesn't fully match vims syntax. If you take a look at .vimrc files, you most likely see maps containing <C-..., <Esc> or <CR>. In vim, the casing of modifiers doesn't matter. Hence, it's the default string representation for keys used by :map or inside the help. Some also have aliases (e.g. <Enter>, <CR>, <Return>). There are more caveats like vim's <F1> vs lf's <f-1> or <M-...> being meta (same as alt aka <A-...> while being mouse in lf (vim uses syntax like <RightMouse> instead).
See also:

• https://neovim.io/doc/user/usr_40/#_key-mapping
• https://neovim.io/doc/user/intro/#_keycodes

Currently, a user coming from vim might assume that it works the same in lf (the docs aren't clear about most of this).
Even I fell into the trap of writing <Space> into my lfrc file and wondered why it did not work.

I think we should either sanitise these whenever possible or mention the differences explicitly.

[joelim-work]

I think the biggest problem here is that lf doesn't fully match vims syntax.

I acknowledge that there are differences in the syntax compared to Vim, but the names of these keys have existed from the beginning of the project and were not chosen by me. To me, it looks like there was a choice to either copy Vim's syntax or to 'improve upon it' (e.g. <CR> vs <enter>), and the latter was chosen.

I'm not particularly interested in adding a layer in the code that will translate user input like <CR> to <enter>, nor do I wish to change then names outright which would then result in a breaking change. I am fine if you wish to explicitly mention the differences in the documentation, but I consider it to be outside the scope of this PR. Since you are the one pushing for this, I will leave it up to you.

EDIT: Regarding name changes, I am probably fine with changing <f-1> to <f1> since f is not a modifier and function keys aren't used as much anyway. But again it is not relevant to this PR.

[CatsDeservePets]

I used this as my main version for the last few days and everything worked as expected. Regarding the key documentation, I will update the docs after this has been merged.

[joelim-work]

Thanks I will merge this now, if there are any regressions then I will address them later.

[ratijas]

Git bisect points to this 6ec60cf as the first bad commit, which causes #2563

Thanks I will merge this now, if there are any regressions then I will address them later.

🙃 hello there