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

JSON preview using jq shows files as empty #318

Closed
LeXofLeviafan opened this issue Oct 24, 2023 · 6 comments · Fixed by #320
Closed

JSON preview using jq shows files as empty #318

LeXofLeviafan opened this issue Oct 24, 2023 · 6 comments · Fixed by #320
Labels
bug Something isn't working waiting on op Waiting for more information from the original poster
Milestone

Comments

@LeXofLeviafan
Copy link

What system are you running Yazi on?

Linux X11

What terminal are you running Yazi in?

Konsole v23.08.1

Yazi version

yazi 0.1.5

Did you try the latest main branch to see if the problem has already been fixed?

Not tried, and I'll explain why below

Describe the bug

When tab_size is set to 8 (which is the default tab width in a Linux terminal and therefore the preferred one when looking at file diffs for example), JSON files are displayed as empty.

This is because --indent option of jq only allows values in range from -1 to 7 (0 stands for "no whitespace", and -1 stands for using tabs), and yazi is neither aware of that nor does it have any fallback behaviour; this applies to the latest commit in master as well:

.args(["-C", "--indent", &PREVIEW.tab_size.to_string(), "."])

(This arbitrary limit exists because for some inexplicable reason jq uses bitmask flags to store indent width…)

Note that I have 2 JSON files which are getting displayed in preview with tab_size set to 8; though considering they have a different colour scheme, I'm guessing jq is simply not applied to them (due to their huge size or perhaps the number of lines).

Expected Behavior

Preview of a JSON file should display the file contents – at least in plain text if the parser failed to process it.

To Reproduce

Set tab_size to 8 and navigate to a JSON file.

Configuration

In yazi.toml:

[preview]
tab_size = 8

Anything else?

When the problem is in the number of spaces, it's still possible to display the requested width by using requesting tabs and replacing each tab at the start with respective number of spaces (e.g. with regexps). Which is probably what the tab_size option is normally used for, anyway…

But either way, a fallback behaviour is necessary since it's perfectly possible for the file to contain invalid JSON (which would cause the parser to bail). …And likely not just for jq since I've seen similar problems with JS files that were apparently deemed invalid. (To compare: MC syntax colouring is primitive and warty, but at least it doesn't cause an empty screen when there's a misplaced comma in the file… or rather, it works just fine pretty much at all times, even if most of the file itself contains gibberish)

@sxyazi
Copy link
Owner

sxyazi commented Oct 25, 2023

Thank you for the detailed explanation of this issue. tab_size=8 causing empty is definitely a potential bug, and I hadn't noticed it – in fact, I never use an 8-tab indentation. I have fixed it in #320, let me know if it works for you.

As for the invalid JSON fallback, I never implemented it, so it's not a bug "but" an "enhancement". Anyway, I've also implemented it in that PR.

BTW, previously there was already a fallback to the built-in highlighter when jq was not installed.

@sxyazi sxyazi added the waiting on op Waiting for more information from the original poster label Oct 25, 2023
@LeXofLeviafan
Copy link
Author

let me know if it works for you

It does. Though I'd like to note that build.sh doesn't run on my system because of +stable (probably cuz I don't have rustup installed). Also I believe it's customary to include build instructions into the main README file (or at least a link to the respective doc).

I never implemented it, so it's not a bug "but" an "enhancement"

The sentiment is familiar but I disagree in this case: the base functionality of displaying file contents doesn't work when a parser fails, therefore it's buggy. Displaying the contents is the base behaviour of this functionality, and should work at least at the most basic level for any file (“the file is just way too big” might be a passable excuse for a failure to display its contents, but “the code in this file wouldn't compile” hardly qualifies as such). An “enhancement,” on the other hand, would be a new feature (or “sub-feature,” I guess) like highlighting of syntactically invalid files.

@sxyazi
Copy link
Owner

sxyazi commented Oct 25, 2023

I think our disagreement lies in the definition of "functionality".

Your definition is too "broad", such as "a preview feature", while I habitually break it down into smaller units like "JSON preview feature", "image preview feature", and "PDF preview feature". You can certainly refer to a "can't preview invalid JSON" as a "can't preview", but this doesn't provide any convenience for our maintenance.

GitHub's issue tracker is a place for developers to conveniently collect and categorize problems. We are used to addressing a specific and particular problem in a single "bug" or "enhancement" issue. I understand that you encountered an issue with build.sh during testing, but please raise it as a separate build-related issue. I just don't want to see a single issue getting cluttered with several "unrelated" problems being solved under it; it can be quite frustrating for me. I know you might argue that they are related: I need to build while testing previews! But please consider it from a software engineering perspective.

Finally thanks for the test, I will merge it now.

@LeXofLeviafan
Copy link
Author

Your definition is too "broad"

That's not correct: there is, in fact, a "preview" feature in your app, and it has a baseline expectation of "the file contents being displayed". A "JSON preview" (or an "image preview" for that matter) is an extension of it, which can be considered on its own, but doing so doesn't remove the concept of a "preview" as the base one. These two concepts both exist within the application, and they both have their own areas of concerns; wherein the correct one must be addressed. The lack of a general fallback that would display the file contents as plain text is not a "JSON preview issue", its a "preview issue"; it concerns the general handling of rendering previews, not just one specific extension thereof.

(…As for the build.sh thing, I wasn't trying to report it as a bug so much as I was indicating that most users would have more trouble following your suggestion of trying out this code than they'd care to deal with.)

@sxyazi
Copy link
Owner

sxyazi commented Oct 26, 2023

What you mentioned about the fallback when jq fails to parse invalid JSON, is specific to the JSON previewer. You will encounter this issue only when you depend on jq and use the JSON previewer. You can simply not install jq and use the built-in highlighter without any issues (It is listed as optional in the documentation).

This problem is not related to the basic preview functionality. You can look at the code of that PR; it only modifies the logic of the JSON previewer and is unrelated to the basic preview functionality.

Additionally, I disagree with your classification of this as a bug; this is incorrect. I would only consider something a bug in the following cases:

  • If a feature is described in the documentation, but doesn't work as expected.
  • If a feature is mentioned in the release logs, but doesn't work as expected.
  • If there is code in the implementation of a feature, but it doesn't work as expected.

In fact, "fallback when jq fails to parse JSON" doesn't fall into any of the above categories. Also, I don't think it falls under the category of "baseline". What's the likelihood of it happening? I use Yazi every day and have never encountered parsing failures. I'm not sure how you define "baseline", maybe:

  • I assume it should be considered baseline, because there is a possibility of parsing failure.
  • There is information about its fallback in MC, so consider MC as the baseline (because you mentioned MC, but I've never used it and don't know its specific workings and the probability of encountering this issue, whether it also detects file types through file, or whether it also relies on jq).

Regarding build.sh, it's just a script I made for my own convenience, not for users. It doesn't appear anywhere in the user documentation. On the contrary, the documentation explains how to build from the source: https://yazi-rs.github.io/usage/installation#build-from-source. Also, if you take a closer look at its contents (I assume you should have looked - don't run any scripts directly; it may harm your computer), it builds binary files for 4 platforms (macOS aarch64, macOS x86_64, Linux, and Windows). I'm not sure why you would need it.

@sxyazi sxyazi added this to the v0.1.6 milestone Nov 11, 2023
Copy link

I'm going to lock this issue because it has been closed for 30 days. ⏳ This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working waiting on op Waiting for more information from the original poster
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants