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(#5): add copying to clipboard support #7

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

Conversation

AlejandroSuero
Copy link
Contributor

@AlejandroSuero AlejandroSuero commented May 3, 2024

  • Windows support
    • Using powershell script

powershell script demo from Windows11 VM:

Image pasted from Windows11 VM:
image from windows wm

  • MacOS support
    • Using osascript

osascript demo from MacOS:

osascript -e 'set the clipboard to (read (POSIX file "/Users/aome/dev/nvim_plugins/freeze.nvim/freeze.png") as JPEG picture)'

Image from the script:
image from MacOS script

  • Unix like support
    • Using xclip

I didn't test it yet in Ubuntu, but the script looks fine from what I use to copy images myself. If xclip is not the way to do it, I will appreciate some suggestions.

Closes #5

@AlejandroSuero AlejandroSuero force-pushed the feature/add-clipboard-support branch from d2ee219 to 9914dc7 Compare May 3, 2024 20:03
@ethanholz
Copy link
Owner

The unix implementation will likely fail in the near feature as xclip will not work on systems using Wayland. This issue might best be resolved by providing users with the ability to override this internal function with their own and we can provide a few defaults for the most common ones.

@AlejandroSuero
Copy link
Contributor Author

AlejandroSuero commented May 3, 2024

@ethanholz What would you think should be the best way to approach it?

This is what first comes to my mind.

local freeze = {
opts = {
dir = ".",
output = default_output,
theme = "default",
config = "base",
open = false,
},
output = nil,
}

Adding an option such as:

local freeze = {
  opts = {
    -- ...
    copy_cmd = ""
  }
  output = nil
}
-- ...
function freeze.copy(filename)
  if freeze.opts.copy_cmd ~= "" then
    -- use freeze.opts.copy_cmd
  end
  -- otherwise use the defaults?
end

I don't use Wayland so I was not familiar with those type of changes, sorry.

@ethanholz
Copy link
Owner

No worries on Wayland, I still appreciate the changes! let me think about implementation around the copy_cmd this weekend and I can get back to you on that.

For some reason the `copy` commands does not work properly when using
them on `vim.fn.jobstart` or `vim.loop.spawn`.

- Modified `freeze.dir` to be `vim.env.PWD` as default instead of `"."`
for `macos` copy command to work properly (needs full path).

- Moved `open` and `copy` actions to the `onExit` function so it works.
If not, it will open the image but not copy it.
@AlejandroSuero
Copy link
Contributor Author

@ethanholz I made some changes for the copy command to work properly.

For some reason it won't work unless outside a job or spawn, it will trigger it but ignore it. Since they are terminal commands I called them in os.execute and it works fine now.

Tested it with both open and copy actions active (true) and it will trigger both.

@AlejandroSuero
Copy link
Contributor Author

@ethanholz I added the Wayland support, but reading into Wayland here I discovered that echo $XDG_SESSION_TYPE could result in x11 if using Wayland under X11 system.

What do you think is the best way to show if using Wayland if this not enough to tell? Maybe using echo $WAYLAND_DISPLAY and check if not nil?

@ethanholz
Copy link
Owner

ethanholz commented May 22, 2024

What do you think is the best way to show if using Wayland if this not enough to tell? Maybe using echo $WAYLAND_DISPLAY and check if not nil?

I think this is good idea for a first pass and is good enough for review.

@AlejandroSuero
Copy link
Contributor Author

If they merge this PR charmbracelet/freeze#97 it could be much easier, don't know exactly how it will work on Wayland but is working on Windows, MacOS and the Manjaro that I have.

It is using gclip in case you want to test it on your machine.

@ethanholz
Copy link
Owner

If we can get this merged upstream I would much prefer to lean on that. Let's give it some time to see if your PR gets merged and if it does (it was reviewed so I'm hopeful), we can use that.

`os.execute` for `vim.fn.system` for better user experience. Sometimes
`os.execute` disturbs Neovim itself.

Fixed `copy_macos` since it had a syntax error.

If copy command fails, notify the user.
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.

Add support for copying to clipboard
2 participants