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

Output from Br shell function in powershell doesn't print to stdout #888

Open
Alfamari opened this issue Jun 9, 2024 · 12 comments
Open
Labels
bug Something isn't working help wanted Extra attention is needed windows Windows specific problem

Comments

@Alfamari
Copy link

Alfamari commented Jun 9, 2024

EDIT: Nothing outputs to stdout. Including br --help, or execution: "echo {file}" (if from_shell: false).

Print Path doesn't print to stdout on windows with powershell br function, I've also even tried 2>&1 which I've had success with other programs that had issues printing to stdout like ffmpeg. This isn't really an issue in and of itself though because I easily created a workaround, but it leads to other problems I explain later:
`{

name: "Print to stdout"
invocation: "stdout"
external: "echo {file}"
from_shell: true

}`

I'm using bat as an example because I think it best illustrates the issue, though the issue is universal.

wezterm-gui_aCdnOuRt1K

wezterm-gui_lh4n4HZBD9

The issue stems from a couple of problems with staging multiple files.
One: "only verbs returning to broot on end can be executed on a multi-selection".
This eliminates the workaround as a possibility because the docs state "from_shell isn't compatible with leave_broot = false"
The workaround also doesn't work without from_shell = true.
Now print_path is able to be executed on multiple staged files, it just cannot be piped anywhere after.

Two: The default file operation commands operate sequentially which is substantially slower by spawning a brand new cmd instance each time rather than either putting all files on one line with cmd /c or using an array in powershell to let the shells handle the batch operations because there isn't a {staged_items} variable to contain multiple items yet. This has caused me to want to pipe for a speed increase and also to try and workaround some of the current limitations with staged items.
Like mentioned:
#695 (comment)
#266 (comment)

In the process of making this submission I found that it works properly with the broot executable, just not the br function.

wezterm-gui_qBxiWwymqX

Though the extra newline at the end can cause issues with pipes:

wezterm-gui_64PN91wWdP

I'm using Pwsh Core 7.4.2 on Windows 10 but the issue is also present on windows powershell 5.1.

Finally, thank you for this incredible masterpiece! I'll be taking it everywhere on all my machines going forward from now on (I even put this sucker on my steam deck lol). It's a shame it isn't mentioned nearly as much as LF, ranger, or vifm. You even created a very clever workaround to the cd issue previously thought to be impossible by other terminal file managers because of the limitations between parent and child processes. Because of your implementation, it's also allowed me to save items into current shell variables which couldn't be done with the other terminal file manager implementations and is a very crucial, almost mandatory feature for me.

@Alfamari Alfamari added the bug Something isn't working label Jun 9, 2024
@Alfamari Alfamari changed the title Print Path doesn't print to stdout on windows with Br function in powershell Output from Br shell function in powershell doesn't print to stdout Jun 9, 2024
@Canop Canop added help wanted Extra attention is needed windows Windows specific problem labels Jun 20, 2024
@AeliusSaionji
Copy link

AeliusSaionji commented Jul 7, 2024

Try this and tell me if it works for what you're trying to do

save the following snippet into a br.ps1, and source with
. br.ps1
then br | bat...

Function br {
  $args = $args -join ' '
  $cmd_file = New-TemporaryFile
  $out_file = New-TemporaryFile
  
  $process = Start-Process -FilePath 'broot.exe' `
                           -ArgumentList "--outcmd $($cmd_file.FullName) $args" `
                           -NoNewWindow -PassThru -WorkingDirectory $PWD -RedirectStandardOutput $out_file

  Wait-Process -InputObject $process # Faster than Start-Process -Wait
  If ($process.ExitCode -eq 0) {
    $cmd = Get-Content $cmd_file
    Get-Content $out_file # | Write-Output (is already implied)
    Remove-Item $cmd_file
    Remove-Item $out_file
    If ($cmd -ne $null) { Invoke-Expression -Command $cmd }
  } Else {
    Remove-Item $cmd_file
    Write-Host "`n" # Newline to tidy up broot unexpected termination
    Write-Error "broot.exe exited with error code $($process.ExitCode)"
  }
}

The current solution I came up with involves redirecting broot's stdout to a file, then reading the file back to shell stdout, because of current oddities in Start-Process that may or may not be addressed in the near future.

PowerShell/PowerShell#18026
PowerShell/PowerShell#5184
PowerShell/PowerShell#4332

@Alfamari
Copy link
Author

Alfamari commented Jul 8, 2024

@AeliusSaionji Yes! It works, thank you!

wezterm-gui_FWo68ddlpG

I'll just have to be wary of the new line at the end during my piping and try to have the foresight to filter it myself in the pipeline.
Though, I'm not sure if I should close the issue with this or not. While it has fixed my issue, not sure if I should leave it open until it's officially implemented.

@AeliusSaionji
Copy link

AeliusSaionji commented Jul 8, 2024

I'll make a PR and reference this issue, don't close until it's merged :)

I can reproduce the extra newline with printpath on linux with fish's br.fish, so @Canop might want to address this in broot itself.

However it's trivial to add here, so you can use this while you wait:

Function br {
  $args = $args -join ' '
  $cmd_file = New-TemporaryFile
  $out_file = New-TemporaryFile
  
  $process = Start-Process -FilePath 'broot.exe' `
                           -ArgumentList "--outcmd `"$cmd_file`" $args" `
                           -NoNewWindow -PassThru -WorkingDirectory $PWD `
                           -RedirectStandardOutput $out_file

  Wait-Process -InputObject $process # Faster than Start-Process -Wait
  If ($process.ExitCode -eq 0) {
    $cmd = Get-Content $cmd_file
    $out = Get-Content $out_file -Raw
    Remove-Item $cmd_file, $out_file
    If ($out -ne $null) { Write-Output $out.TrimEnd() } # print broot's stdout to shell stdout
    If ($cmd -ne $null) { Invoke-Expression -Command $cmd }
  } Else {
    Remove-Item $cmd_file, $out_file
    Write-Host "`n" # Newline to tidy up broot unexpected termination
    Write-Error "broot.exe exited with error code $($process.ExitCode)"
  }
}

@Canop
Copy link
Owner

Canop commented Jul 8, 2024

I clearly prefer the correct script to be installed by broot. But as I can't test myself on Windows, I let you create and check the PR.

@AeliusSaionji
Copy link

AeliusSaionji commented Jul 8, 2024

Maybe you misunderstood-

broot's printpath verb adds a newline at the end of its output, regardless of shell. Is this something that would be easy to suppress? Would it be missed if gone?

image

PowerShell pipes treat every line as an object, so it presents an issue when piping the output to powershell. If the issue is powershell specific and/or if it's desirable to keep the newline for other systems, then I'll PR the br.ps1 that trims newlines from the output.

@Canop
Copy link
Owner

Canop commented Jul 8, 2024

Instead of changing :print_path, which is too specific anyway, why not having a :write_stdout verb, similar to the current :write_output but writing to stdout instead of writing to the file provided with the --verb-output launch argument.

This would allow verbs like this:

    {
    invocation: wp
    cmd: ":write {file};:quit"
    }

But would be more generic.

I would also define a :writeln_stdout.

@Canop
Copy link
Owner

Canop commented Jul 8, 2024

@AeliusSaionji Can you have a look at #901 ?

@Alfamari
Copy link
Author

@AeliusSaionji Ahhh, it's so nice. Works beautifully. Thank you again! I'll try and be more mindful about trying to fix small things myself in the br.ps1 if I'm able.

@Alfamari
Copy link
Author

@Canop Excuse me if this is irrelevant, I'm not sure if this would be a problem, but the reason I'm currently using :print_path is because it's one of the only commands that's able to exit broot on multiple staged files, so I think that should be taken into consideration for when/how :writeln_stdout should be implemented.

@Canop
Copy link
Owner

Canop commented Jul 12, 2024

@Alfamari This is very relevant and something I didn't think about

@AeliusSaionji
Copy link

AeliusSaionji commented Jul 12, 2024

@AeliusSaionji Can you have a look at #901 ?

I apologize for the delay, and for the long essay I wrote you 😅


@Alfamari This is very relevant and something I didn't think about

And this short exchange effectively summarizes most of what I wrote :)

@Alfamari
Copy link
Author

Ok cool, I'm only a novice hobbyist so I don't fully know what's going on at all times or when something is self-evident. I also respect the self-less work you guys do so I don't want to pester about small things that are redundant or should have been easy to figure out on my own. I don't want to be that guy that where the answer could have been "read the docs", etc. 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed windows Windows specific problem
Projects
None yet
Development

No branches or pull requests

3 participants