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

Fix clipboard issue for macOS and Windows #66

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

askdkc
Copy link
Contributor

@askdkc askdkc commented Dec 9, 2023

This PR provides fix for issue #62

When using this package on macOS, org-web-tools--get-first-url needs to use pbpaste instead of org-web-tools--get-first-url, otherwise, it cannot get URL from clipboard.

Update: 2023-12-13
The Issue at #62 is likely just a DNS glitch.
However, the clipboard bug is causing problems for org-web-tools on macOS and Windows.
This pull request addresses and resolves both of these issues on these platforms.

@askdkc askdkc mentioned this pull request Dec 9, 2023
@alphapapa
Copy link
Owner

Hello,

I appreciate your trying to solve this, but I have doubts about this being the best solution. Emacs supports MacOS, so it seems like it should be able to get the clipboard contents without calling a third-party shell command. As well, there are several third-party builds of Emacs for Macs, some of which may have extra code for integrations like this.

I'd be grateful if you'd file a new issue with all the relevant information, and then we can research the most appropriate way to solve the problem on MacOS. Thanks.

@askdkc
Copy link
Contributor Author

askdkc commented Dec 10, 2023

@alphapapa

I see and I understand your concern.

I have macOS and Debian laptop so I tested these commands on both and here is the result:

Clipboard related command outputs differences on macOS and Linux

When text inside clipboard copied from the browser is : https://github.com/alphapapa/org-web-tools

macOS

  • gui-get-selection
  (gui-get-selection 'CLIPBOARD)
#+RESULTS:
  • shell-command-to-string
  (shell-command-to-string "pbpaste")
#+RESULTS:
: https://github.com/alphapapa/org-web-tools
  • current-kill
  (current-kill 0)
#+RESULTS:
: https://github.com/alphapapa/org-web-tools

Linux

  • gui-get-selection
  (gui-get-selection 'CLIPBOARD)
#+RESULTS:
: https://github.com/alphapapa/org-web-tools
  • shell-command-to-string
  (shell-command-to-string "pbpaste")
#+RESULTS:
: /bin/bash: Line 1: pbpaste: command not found
  • current-kill
  (current-kill 0)
#+RESULTS:
: https://github.com/alphapapa/org-web-tools

About (gui-get-selection 'CLIPBOARD) on macOS

(gui-get-selection 'CLIPBOARD) works on macOS ONLY IF user copies some texts inside Emacs on macOS.

So for example, inside my Emacs, I copy https://github.com/alphapapa/org-web-tools and then run

  (gui-get-selection 'CLIPBOARD)

It outputs https://github.com/alphapapa/org-web-tools

But those texts which copied from other application like Safari or FireFox, cannot be pasted by (gui-get-selection 'CLIPBOARD).

People use org-web-tools when they find out something interesting on their browser.
They copy the URL and paste it to Emacs using org-web-tools. But if they are using macOS, (gui-get-selection 'CLIPBOARD) returns nil so that org-web-tools--get-first-url cannot resolve DNS when it runs curl command.

Hence, "plz: plz: Curl error: "Curl error", #s(plz-error (6 . "Couldn't resolve host. The given remote host was not resolved.") nil nil)" occurs.

Using (current-kill 0) works on both platform without any issues.

@alphapapa
Copy link
Owner

Thanks for investigating that.

Using (current-kill 0) works on both platform without any issues.

That seems like the way to do it, then.

@askdkc
Copy link
Contributor Author

askdkc commented Dec 10, 2023

Thank you for the review.
I've updated the code to use current-kill on macOS.

This bug on macOS was killing me since I started using your package from last week.
I'm relatively new to Emacs, started only last month, and not very familiar with Elisp.

Any improvements you can suggest would be greatly appreciated.

@askdkc
Copy link
Contributor Author

askdkc commented Dec 10, 2023

I have a Windows machine also and out of my curiosity, I tried this package on WSL Ubuntu.
To my surprise, (gui-get-selection 'CLIPBOARD) didn't work on WSL either.

So I updated the code to only use (current-kill 0) in order to get the data from clipboard.
I think now it supports all Linux, macOS and Windows without any hiccups.

Hopefully this helps.

@alphapapa
Copy link
Owner

Thank you.

As a final adjustment, this PR could have two minor optimizations:

  1. Squash into a single commit and force-push to the PR's branch. This cleans up the commit history, since the earlier changes aren't needed.
  2. Instead of using append with a new list containing the current-kill, just use (cons (current-kill 0) kill-ring. This saves allocating another cell of memory, an optimization I should have made myself years ago.

Let me know if you want to do those yourself, in which case the changelog can be updated and the PR can be merged.

@askdkc
Copy link
Contributor Author

askdkc commented Dec 11, 2023

@alphapapa

Thanks for the review!

I cleaned up the commit history and fixed the code as you instructed.

@askdkc askdkc changed the title Use pbpaste when macOS Fix clipboard issue for macOS and Windows Dec 12, 2023
@alphapapa alphapapa self-assigned this Dec 13, 2023
@alphapapa alphapapa added this to the 1.3 milestone Dec 13, 2023
@alphapapa alphapapa merged commit ebeb297 into alphapapa:master Dec 13, 2023
0 of 5 checks passed
@alphapapa
Copy link
Owner

@askdkc Thanks for your work on this!

@askdkc askdkc deleted the fix-url-paste-mac-issue branch December 13, 2023 01:48
@askdkc
Copy link
Contributor Author

askdkc commented Dec 13, 2023

@alphapapa

You're welcome! Now I can insert links with titles into my org note files smoothly😄

Thank you for creating this useful package!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants