alphapapa / org-web-tools

View, capture, and archive Web pages in Org-mode
GNU General Public License v3.0
647 stars 33 forks source link

Fix clipboard issue for macOS and Windows #66

Closed askdkc closed 11 months ago

askdkc commented 11 months ago

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 https://github.com/alphapapa/org-web-tools/issues/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.

alphapapa commented 11 months ago

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 commented 11 months ago

@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

Linux

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

@alphapapa

Thanks for the review!

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

alphapapa commented 11 months ago

@askdkc Thanks for your work on this!

askdkc commented 11 months ago

@alphapapa

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

Thank you for creating this useful package!!