kyoheiu / felix

tui file manager with vim-like key mapping
https://kyoheiu.dev/felix/
MIT License
712 stars 25 forks source link

Panic: Preview can't handle some chars when filetype is txt? #169

Closed Enayaaa closed 1 year ago

Enayaaa commented 1 year ago

Running fx with previewing on, I tried to scroll to a file having the following content:

████████████████████████████████

it seems that the filetype is also important, as this error only occured if my filetype was .txt as far as my testing showed.

fx panicks at such file content and prints the following.

Panic: byte index 94 is not a char boundary; it is inside '█' (bytes 93..96) of `████████████████████████████████
`
Error: felix panicked

The problem does not occur if I have previewing off (pressing v while fx is open somewhere).

To reproduce: place a file with the above content and .txt fileending in an empty directory and try to run fx in the directory.

kyoheiu commented 1 year ago

Could you tell me your felix version (fx --version) and OS?

Enayaaa commented 1 year ago

felix v2.2.2 Linux pop-os 6.0.12-76060006-generic #202212290932~1671652965~22.04~452ea9d SMP PREEMPT_DYNAMIC Wed D x86_64 x86_64 x86_64 GNU/Linux

kyoheiu commented 1 year ago

Hm, I tried to reproduce this. but couldn't.

To reproduce: place a file with the above content and .txt fileending in an empty directory and try to run fx in the directory.

screenshot

(Sorry for not providing the image as gif)

With the text you gave I created a .txt file and executed fx in the directory. As shown above, preview can show the text (although the positions of new lines are not correct, which is due to the need to handle the multibyte text as properly as this app can for now.)

Basically this char_boundary-related issue should have been fixed by the commit 7b1d5bf0565dedadc48a1d42cc2d22c28825dad5 .

How did you install felix? Maybe we can find another clue if you install from this repo (either main or develop). I'd like to know how this panic occurs, so it'd be great if you give it a try.

kyoheiu commented 1 year ago

FYI, felix detects file type by content_inspector::inspect, so the extension is not relevant. After that, the text content is split by:

  1. native function (when syntax-highlighting is off) or
  2. syntect::util::split_at (my fork version). So it may be useful to know whether the highlighting is on.
Enayaaa commented 1 year ago

I had previously installed via cargo cargo install felix and the version printed by it was what I sent in my comment previously. Running fx --version with that version of fx gave felix v2.2.2: Up to date.

I now cloned the repo and installed from main, seems like the issue is not there anymore.

With the version of fx on cargo I tested other fileendings and only .txt panicked.

kyoheiu commented 1 year ago

Happy to hear that the panic does not occur with this repository: As I understand, I should've set [patch] to tell crates.io to use my own fork version of syntect, not only specifying the url in the dependencies. Nah, it seems I have to publish the forked dependency to make crates.io use it. cf https://users.rust-lang.org/t/how-to-work-with-a-forked-dependency/13338 (The panic is due to the latest version of syntect panicking when splitting texts with multibyte chars: I created PR for this about a month ago)

Even when you install this app via cargo, if the syntax-highlighting is off the panic does not occur I think - because the syntect panic happens only if the highlighting is on.

In the next pubishing to the crate.io I'll fix this. Thank you for the report!

kyoheiu commented 1 year ago

v2.2.3 released, but this issue is still alive with cargo install felix due to syntect not updating yet. FYI, v2.2.3 fixes wide chars handling and the preview feature. If you'd like to use this latest version, please install from this repo.

kyoheiu commented 1 year ago

Updating syntect solved this.