kamiyaa / joshuto

ranger-like terminal file manager written in Rust
https://crates.io/crates/joshuto
GNU Lesser General Public License v3.0
3.41k stars 151 forks source link

Cleanup preview.sh template #285

Closed DLFW closed 1 year ago

DLFW commented 1 year ago

Removed ranger residuals and dead code from the preview.sh script and the logic that invokes it.

DLFW commented 1 year ago

As a user recently pointed out, there was quite some misleading stuff in the preview.sh template script, shipped with Joshuto.

Changes

Intro Comment

The intro comment in the script has been rewritten. It was still describing the ranger usage.

Exit Codes

Joshuto seems to only check for exit code 1 / != 1. So, the table of exit codes in the script's intro comment has been cut down to the values 0 and 1. This should match the Joshuto functionality. All exit [>1] have been replaced by exit 0.

Remove --image-cache

The --image-cache parameter has been removed from both, the script and the Joshuto code that calls it. It was unused and was always zero. Also, the motivation was unclear to me.

Question

I thought about removing also the --x-coord and the --y-coord parameters. I don't see in what scenario they can be relevant for creating a text-preview. However, I first wanted to ask if that's ok.

Actually, also the --preview-width and the --preview-height parameter do not really make sense right now as Joshuto does not refresh the text-preview cache on geometry-change. (Or am I wrong here?) However, maybe it may still be helpful to have them.

Side Note

I think it would be a good idea to change the struct

pub struct FilePreview {
    pub status: std::process::ExitStatus,
    pub output: String,
    pub index: usize,
    pub modified: time::SystemTime,
}

so that the status is a meaningful enumeration. If I will ever pick up a feature that introduces a new exit-code logic, I would do that....

DLFW commented 1 year ago

Oh yeah, and I re-activated the preview for video/audio and for PDF. Not sure why these parts where commented out?

Also, the preview of epub now also uses bat for coloring the markdown translation.

kamiyaa commented 1 year ago

Intro Comment

The intro comment in the script has been rewritten. It was still describing the ranger usage.

Yeah, this was due for change 👍

Exit Codes

Joshuto seems to only check for exit code 1 / != 1. So, the table of exit codes in the script's intro comment has been cut down to the values 0 and 1. This should match the Joshuto functionality. All exit [>1] have been replaced by exit 0.

Yeah, I was thinking down the line we would implement the rest. But I guess we can cross that bridge once it comes.

Remove --image-cache

The --image-cache parameter has been removed from both, the script and the Joshuto code that calls it. It was unused and was always zero. Also, the motivation was unclear to me.

This was also residual from ranger. I think the feature itself is useful, but maybe theres a better way to do it than --image-cache

Question

I thought about removing also the --x-coord and the --y-coord parameters. I don't see in what scenario they can be relevant for creating a text-preview. However, I first wanted to ask if that's ok.

Yeah, I think it can be removed for text previews, but it is required for image previews.

Actually, also the --preview-width and the --preview-height parameter do not really make sense right now as Joshuto does not refresh the text-preview cache on geometry-change. (Or am I wrong here?) However, maybe it may still be helpful to have them.

Yes, you are correct here.

Side Note

I think it would be a good idea to change the struct

pub struct FilePreview {
    pub status: std::process::ExitStatus,
    pub output: String,
    pub index: usize,
    pub modified: time::SystemTime,
}

so that the status is a meaningful enumeration. If I will ever pick up a feature that introduces a new exit-code logic, I would do that....

Yeah that works 👍

Let me know if you have any other questions. As always thanks for the contributions! Once the PR is ready for review, I will review it 👍

DLFW commented 1 year ago

Hi @kamiyaa, this is ready for review, just in case it got missed. :wink:

kamiyaa commented 1 year ago

LGTM, thanks!