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

Cleanup preview.sh template #285

Merged
merged 2 commits into from
May 7, 2023
Merged

Cleanup preview.sh template #285

merged 2 commits into from
May 7, 2023

Conversation

DLFW
Copy link
Contributor

@DLFW DLFW commented Apr 23, 2023

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

@DLFW
Copy link
Contributor Author

DLFW commented Apr 23, 2023

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....

Removed ranger residuals and dead code from the preview.sh script and
the logic that invokes it.
@DLFW
Copy link
Contributor Author

DLFW commented Apr 23, 2023

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
Copy link
Owner

kamiyaa commented Apr 25, 2023

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 DLFW marked this pull request as ready for review April 27, 2023 20:43
@DLFW
Copy link
Contributor Author

DLFW commented May 7, 2023

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

@kamiyaa
Copy link
Owner

kamiyaa commented May 7, 2023

LGTM, thanks!

@kamiyaa kamiyaa merged commit 95d304f into kamiyaa:main May 7, 2023
3 checks passed
@DLFW DLFW deleted the fix-preview.sh branch May 12, 2023 20:47
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

Successfully merging this pull request may close these issues.

None yet

2 participants