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

feat(#58): add copy to clipboard flag --copy #97

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

AlejandroSuero
Copy link
Contributor

@AlejandroSuero AlejandroSuero commented May 19, 2024

Changes made

With this addition the user can choose in interactive mode, or with the flag --copy, to copy the created image to their system's clipboard. This was achieved with the cross-platform clipboard package by golang-design.

Tests

  • Tested via make test:
    • In MacOS.
    • In Windows.
  • Manually tested via ./freeze --config user --lines 23,30 config.go --copy to get this image:
    image

Closes #58.

@Moulick
Copy link

Moulick commented May 25, 2024

@AlejandroSuero Not working, I cloned your fork, checked out your branch and compiled. --copy is not copying to the clipboard. Also make test fails. I am on MacOS (14.5 (23F79))

❯ make test
go test ./...
?   	github.com/charmbracelet/freeze/font	[no test files]
?   	github.com/charmbracelet/freeze/input	[no test files]
?   	github.com/charmbracelet/freeze/svg	[no test files]
?   	github.com/charmbracelet/freeze/test/input	[no test files]
--- FAIL: TestFreezeCopy (0.04s)
    freeze_test.go:81: clipboard is empty
FAIL
FAIL	github.com/charmbracelet/freeze	2.748s
FAIL
make: *** [test] Error 1

@AlejandroSuero
Copy link
Contributor Author

AlejandroSuero commented May 27, 2024

@Moulick I am using the same version of MacOS and it builds correctly with go build.

add-copy-to-clipboard-branch-demo.mp4
  • Result from using ./freeze --config user --lines 23,30 config.go --copy after being built:
    image

As for the test part, I committed a fix (eba42f9), apparently it was reading from the clipboard but not writing it, for example: If I have an image on the clipboard it will pass the test but it will fail otherwise.

  • Image from make test:
    image

@Moulick
Copy link

Moulick commented May 29, 2024

@AlejandroSuero now the test works, and the image generated by the make test appears in my clipboard. But running ./freeze --lines 23,30 config.go --copy still writes the image to the disk rather than clipboard. Attaching the compiled binary from my laptop

freeze.zip

@AlejandroSuero
Copy link
Contributor Author

After unzip freeze.zip then ./freeze --execute "exa -l" --copy I get this (recently powered up MacOS machine with nothing in the clipboard):

image

But running ./freeze --lines 23,30 config.go --copy still writes the image to the disk rather than clipboard

With these, are you referring to the fact that it writes the output image as well as copying it to the clipboard?

When running ./freeze --lines 23,30 config.go --copy with the downloaded freeze.zip I get the image freeze.png as well as it being copied to the clipboard:

image

@Moulick
Copy link

Moulick commented May 30, 2024

@AlejandroSuero not sure if there is something wrong with my machine but for me --copy is writing the freeze.png to disk and nothing to clipboard. Maybe we need someone else to try test

@AlejandroSuero
Copy link
Contributor Author

freezezip-just-powered-up-machine.mp4

@Moulick this is how it works for me just powering up the machine and having the clipboard empty.

png.go Outdated Show resolved Hide resolved
@AlejandroSuero
Copy link
Contributor Author

With the changes in c57172d, It still passes the tests and works the same way manually for me.

@maaslalani
Copy link
Contributor

Hey @AlejandroSuero, instead of introducing a new flag I wonder if we should simply allow --output clipboard or --output copy to copy to the clipboard, what do you think?

@meowgorithm
Copy link
Member

No opinions, but you could also introduce the option to print to stdout so you could freeze --output stdout | pbcopy.

@maaslalani
Copy link
Contributor

maaslalani commented May 31, 2024

No opinions, but you could also introduce the option to print to stdout so you could freeze --output stdout | pbcopy.

In that case I would rather simply detect if stdout is a tty and send to stdout if it's not a tty.

freeze | pbcopy

@AlejandroSuero
Copy link
Contributor Author

@maaslalani the problem I contemplated when I thought about it in the first place was that if for example you want to write an output name like artichoke_example.png and still you want to copy the image to the clipboard it won't be possible.

But I would like to hear your thoughts about that scenario.

@AlejandroSuero
Copy link
Contributor Author

@meowgorithm as for the stdout @Moulick mentioned this in the issue linked to this PR:

@Moulick
Copy link

Moulick commented May 31, 2024

  1. It looks like something is broken on my system, as I ran a new MacOS VM and it works fine there. I have no clue what's broken on my system but probably a myriad of third-party applications I am running that might be interfering with the clipboard. ¯\_(ツ)_/¯
  2. --output clipboard does sound like a better idea. I don't think anyone want to copy to clipboard and write to disk at the same time. Even on Android/iOS, the option when taking a screenshot is copy to clipboard and delete.
  3. pbcopy does not handle putting images into the clipboard correctly. So freeze | pbcopy cannot work.
    1. freeze --output stdout or detecting stdout tty should also be done, so we can use other programs (like image compression) in the workflow but probably in a separate PR.

@AlejandroSuero AlejandroSuero requested a review from Moulick June 7, 2024 11:27
With [gclip](https://github.com/golang-design/clipboard)
when not copying a png image `gclip -copy -f freeze.svg`
it will copy it as text, otherwise as an image
@AlejandroSuero
Copy link
Contributor Author

@Moulick I made some changes to work with librsvg and for non-png formats.

I added some comments on how gclip does not seem to work as an image when copying non-png formats. I wrote an issue about it in their github to figure it out.

@AlejandroSuero
Copy link
Contributor Author

As @maaslalani suggested, I made the changes so the user can freeze cut.go --output clipboard.

With these changes (9a169d5, 01d684a), the comment #97 (comment) is worked around so it will always use .png format to copy the image.

Test Results
> go test -v ./...
?       github.com/charmbracelet/freeze/font    [no test files]
?       github.com/charmbracelet/freeze/input   [no test files]
?       github.com/charmbracelet/freeze/test/input      [no test files]
?       github.com/charmbracelet/freeze/svg     [no test files]
=== RUN   TestConfig
--- PASS: TestConfig (0.00s)
=== RUN   TestCut
--- PASS: TestCut (0.00s)
=== RUN   TestFreeze
--- PASS: TestFreeze (0.27s)
=== RUN   TestFreezeOutput
--- PASS: TestFreezeOutput (0.04s)
=== RUN   TestFreezeCopy
--- PASS: TestFreezeCopy (0.50s)
=== RUN   TestFreezeHelp
--- PASS: TestFreezeHelp (0.01s)
=== RUN   TestFreezeErrorFileMissing
--- PASS: TestFreezeErrorFileMissing (0.01s)
=== RUN   TestFreezeConfigurations
=== RUN   TestFreezeConfigurations/artichoke-base
=== RUN   TestFreezeConfigurations/artichoke-full
=== RUN   TestFreezeConfigurations/eza
=== RUN   TestFreezeConfigurations/execute
=== RUN   TestFreezeConfigurations/bubbletea
=== RUN   TestFreezeConfigurations/bubbletea-copy
=== RUN   TestFreezeConfigurations/layout
=== RUN   TestFreezeConfigurations/haskell
=== RUN   TestFreezeConfigurations/dracula
=== RUN   TestFreezeConfigurations/border-radius
=== RUN   TestFreezeConfigurations/window
=== RUN   TestFreezeConfigurations/border-width
=== RUN   TestFreezeConfigurations/padding
=== RUN   TestFreezeConfigurations/margin
=== RUN   TestFreezeConfigurations/shadow
=== RUN   TestFreezeConfigurations/dimensions
=== RUN   TestFreezeConfigurations/dimensions-margin
=== RUN   TestFreezeConfigurations/dimensions-margin-line-numbers
=== RUN   TestFreezeConfigurations/dimensions-padding
=== RUN   TestFreezeConfigurations/dimensions-config
=== RUN   TestFreezeConfigurations/overflow
=== RUN   TestFreezeConfigurations/lines
=== RUN   TestFreezeConfigurations/font-size-28
=== RUN   TestFreezeConfigurations/font-size-14
=== RUN   TestFreezeConfigurations/line-height-2
=== RUN   TestFreezeConfigurations/overflow-line-numbers
=== RUN   TestFreezeConfigurations/helix
=== RUN   TestFreezeConfigurations/glow
=== RUN   TestFreezeConfigurations/tab
--- PASS: TestFreezeConfigurations (0.77s)
    --- PASS: TestFreezeConfigurations/artichoke-base (0.02s)
    --- PASS: TestFreezeConfigurations/artichoke-full (0.02s)
    --- PASS: TestFreezeConfigurations/eza (0.02s)
    --- PASS: TestFreezeConfigurations/execute (0.03s)
    --- PASS: TestFreezeConfigurations/bubbletea (0.03s)
    --- PASS: TestFreezeConfigurations/bubbletea-copy (0.03s)
    --- PASS: TestFreezeConfigurations/layout (0.03s)
    --- PASS: TestFreezeConfigurations/haskell (0.03s)
    --- PASS: TestFreezeConfigurations/dracula (0.03s)
    --- PASS: TestFreezeConfigurations/border-radius (0.03s)
    --- PASS: TestFreezeConfigurations/window (0.03s)
    --- PASS: TestFreezeConfigurations/border-width (0.03s)
    --- PASS: TestFreezeConfigurations/padding (0.03s)
    --- PASS: TestFreezeConfigurations/margin (0.03s)
    --- PASS: TestFreezeConfigurations/shadow (0.03s)
    --- PASS: TestFreezeConfigurations/dimensions (0.03s)
    --- PASS: TestFreezeConfigurations/dimensions-margin (0.03s)
    --- PASS: TestFreezeConfigurations/dimensions-margin-line-numbers (0.03s)
    --- PASS: TestFreezeConfigurations/dimensions-padding (0.03s)
    --- PASS: TestFreezeConfigurations/dimensions-config (0.03s)
    --- PASS: TestFreezeConfigurations/overflow (0.03s)
    --- PASS: TestFreezeConfigurations/lines (0.03s)
    --- PASS: TestFreezeConfigurations/font-size-28 (0.03s)
    --- PASS: TestFreezeConfigurations/font-size-14 (0.03s)
    --- PASS: TestFreezeConfigurations/line-height-2 (0.03s)
    --- PASS: TestFreezeConfigurations/overflow-line-numbers (0.03s)
    --- PASS: TestFreezeConfigurations/helix (0.03s)
    --- PASS: TestFreezeConfigurations/glow (0.02s)
    --- PASS: TestFreezeConfigurations/tab (0.02s)
PASS
ok      github.com/charmbracelet/freeze 3.405s

@bashbunni bashbunni self-assigned this Oct 11, 2024
@bashbunni
Copy link
Member

Hey just an update on this - I want to see how we can improve where this fits in the codebase. The feature works great, I just think we can probably refactor a bit to leave the codebase cleaner than we found it.

I also think --output clipboard is better nomenclature than --output copy so I removed that option in my update here. Sorry, this PR will sit a bit longer to figure out the cleanest solution 🙏 Thanks for your patience

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.

put result in clipboard
5 participants