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

bat -p should also default --wrap never #289

Closed
wakemaster39 opened this issue Sep 7, 2018 · 12 comments · Fixed by #313
Closed

bat -p should also default --wrap never #289

wakemaster39 opened this issue Sep 7, 2018 · 12 comments · Fixed by #313
Labels
feature-request New feature or request

Comments

@wakemaster39
Copy link

When using cat on files containing long string like in id_rsa.pub, you are able to easily highlight and select the text and paste it anywhere and its copied verbatim. Using bat, since the default is to wrap output internally and not have the terminal do it, this causes additional line breaks in the copied text. This is extremely misleading when you are doing things like transferring text and using terminals of the same width so you do not see the added line breaks.

Adding the --wrap never along with -p is cumbersome and leads to falling back on cat instead.

Is this option configurable if I was to alter the theme?

@eth-p
Copy link
Collaborator

eth-p commented Sep 7, 2018

AFAIK, you can't configure it by altering the theme.

However, I agree that -p should also disable wrapping. Wrapping was meant to be used when the terminal's wrapping was inadequate (e.g. when the sidebar is there) and caused visual inconsistencies. When there isn't a sidebar, the terminal wrapping does a better job and it doesn't really make sense to have bat the wrap lines itself.

@sharkdp
Copy link
Owner

sharkdp commented Sep 7, 2018

@wakemaster39 Thank you for the feedback!

Absolutely agreed, wrapping should be disabled for the plain style.

@sharkdp sharkdp added the feature-request New feature or request label Sep 7, 2018
@sharkdp
Copy link
Owner

sharkdp commented Sep 8, 2018

Fixed via #291 by @eth-p

@sharkdp sharkdp closed this as completed Sep 8, 2018
@sharkdp
Copy link
Owner

sharkdp commented Sep 12, 2018

Released in v0.7.0.

@falzm
Copy link

falzm commented Sep 18, 2018

Hi, I've just upgraded to v0.7.0 and it doesn't seem to disable wrapping when invoked with --plain (still have to specify --wrap never). Am I missing something?

@sharkdp sharkdp reopened this Sep 18, 2018
@sharkdp
Copy link
Owner

sharkdp commented Sep 18, 2018

@falzm Thank you for the feedback!

I can confirm this - our implementation in #291 is wrong. I already have a fix.

@falzm
Copy link

falzm commented Sep 18, 2018

Thanks!

@sharkdp
Copy link
Owner

sharkdp commented Sep 23, 2018

Fixed in v0.7.1

@ernstki
Copy link

ernstki commented Jul 21, 2019

Hi, all. I'm using ranger, specifying bat as the previewer for Markdown files in scope.sh.

Within the ranger preview pane, tput cols does not report the correct viewport width, but they provide an environment variable that does, $PV_WIDTH (preview width).

Because of the outcome of this issue (-p disables wrapping) and the fact that wrapping settings are ignored anyway when stdout is not a terminal (app.rs line 190), I've been unable to coerce bat into giving me plain, wrapped output.

What I've resorted to is bat -p --color=always "$FILE_PATH" | fmt -s -w $PV_WIDTH, and I guess it's OK, but it seems like something bat could do (or used to be able to do).

I've made a screenshot to hopefully better illustrate everything I'm talking about above (large, so I'm not embedding it here).

Is there some other trick I'm missing to get --style plain and have the --terminal-width and --wrap options work together, to give me plain, wrapped output from bat? Also, is it just me, or does bat not do word wrapping (wrapping on word breaks, rather than in the middle of words)?

If the answer is "that's beyond the scope" or "it's not bat's job to do that" I understand. Thought I'd ask.

@eth-p
Copy link
Collaborator

eth-p commented Jul 22, 2019

@ernstki Good catch on it not being possible to wrap when stdout is not a terminal. That was done before --terminal-width became an available option, and correcting it should be a simple fix to your issue.

A solution would be to keep wrapping enabled if --terminal-width is present, even though it's not printing to a terminal:

- output_wrap: if !self.interactive_output {
+ output_wrap: if !self.interactive_output && !self.matches.is_present("terminal-width") {

You are correct about bat not having word wrapping. I was the one who added the wrapping feature, and I wasn't able to come up with a decent solution for how to do word wrapping at the time. The styles parsed by syntect weren't suitable as word boundaries, and I didn't want to implement word breaking that failed to take the language into consideration.

@ernstki
Copy link

ernstki commented Jul 22, 2019

OK, I understand that being more of a challenge with the embedded formatting from syntax highlighting.

I'm apprehensive about just implementing your suggestion in a new pull request, because

  1. is it really a bug?
  2. is it something that bat really needs to care about? (feeping creaturism, and all that)
  3. is --terminal-width really what you would want to call that option?
    • the desired width in that use case has nothing to do with the terminal, per se; it'd be more apt to call that just --width or --output-width
      • and again with the feature creep

I think if no one else is asking for this, maybe it's best if I just keep a patchfile around on my end.

@eth-p
Copy link
Collaborator

eth-p commented Jul 24, 2019

It's definitely not a bug, but it is an outdated behaviour that could be improved upon fairly easily.

I also wouldn't consider it to be feature creep. The --terminal-width option already exists, and has been "available" (i.e. not hidden) since dda27b2. All the patch would do is change the behaviour of --wrap to respect that the user wants wrapping even when stdout isn't a terminal (and when a width is provided with --terminal-width).

I do agree that the option would be better named as --width or --output-width, but that could be problematic for backwards compatibility with scripts that already use --terminal-width.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants